[tor-bugs] #17799 [Core Tor/Tor]: Hash All PRNG output before use

[Tor Bug Tracker & Wiki]

#17799: Hash All PRNG output before use
-------------------------------+----------------------------------------
 Reporter:  teor               |          Owner:  nickm
     Type:  defect             |         Status:  needs_revision
 Priority:  Medium             |      Milestone:  Tor: 0.2.9.x-final
Component:  Core Tor/Tor       |        Version:  Tor: unspecified
 Severity:  Normal             |     Resolution:
 Keywords:  TorCoreTeam201604  |  Actual Points:
Parent ID:                     |         Points:  small/medium-remaining
 Reviewer:  asn                |        Sponsor:
-------------------------------+----------------------------------------

Comment (by nickm):

 Changes incorporated, I believe, except as noted below.

 {{{
 +    // cpunk: What happens here on "coverage" builds?
 }}}

 I'm assuming you mean "coverage" builds that also set --disable-asserts-
 in-tests.  They break; --disable-asserts-in-tests makes your tor broken.
 I'm adding a ticket (#18888) to make them warn even more loudly, in case
 somebody thinks it's a good idea to actually use that in production.

 {{{
  shake_prng_getbytes_large(shake_prng_t *prng, uint8_t *out, size_t n)
 +// cpunk: So, this function has a different behaviour, compared to
 _raw(), in
 +// that it pulls in less external entropy (openssl, libc) for the same
 amount
 +// of bytes.  Is this significant?  Does it change its cryptographic
 profile?
 +// Compare the following two:
 +//
 +//   1)
 +//     crypto_rand(buf, desired_size)
 +//
 +//   2)
 +//     not_large_size = 128
 +//     for (i = 0; i < desired_size/not_large_size; ++i)
 +//       crypto_rand(buf + i*not_large_size, not_large_size)
 +//     crypto_rand(buf + i*not_large_size, desired_size -
 i*not_large_size)
 +//
 +// Where desired_size is large enough to cause refill(s).
 }}}

 So, the entire "reseed periodically after a certain amount of data"
 business is a bit voodoo; I've added notes to the check_reseed function.
 I've added a bit more commentary to the getbytes_large function itself
 too.

 {{{
    crypto_teardown_shake_prng(); /* ???? */
 +                                // -> cpunk: What is it?
 }}}

 I think I was wondering whether it causes a redundant call to
 crypto_teardown_shake_prng().


 {{{
    RAND_set_rand_method(&ossl_shake_method);
 +  // cpunk: From
 https://www.openssl.org/docs/manmaster/crypto/RAND_set_rand_method.html
 +  //
 +  //   RAND_set_default_method() makes meth the method for PRNG use. NB:
 This
 +  //   is true only whilst no ENGINE has been set as a default for RAND,
 so
 +  //   this function is no longer recommended.
 +  //
 +  // And later (RAND_METHOD is a typedef for rand_meth_st):
 +  //
 +  //   RAND_METHOD implementations are grouped together with other
 algorithmic
 +  //   APIs (eg. RSA_METHOD, EVP_CIPHER, etc) in ENGINE modules.  If a
 default
 +  //   ENGINE is specified for RAND functionality using an ENGINE API
 function,
 +  //   that will override any RAND defaults set using the RAND API (ie.
 +  //   RAND_set_rand_method()). For this reason, the ENGINE API is the
 +  //   recommended way to control default implementations for use in RAND
 and
 +  //   other cryptographic algorithms.
 +  //
 +  // IIUC, openssl seems to be saying this should be implemented as an
 engine
 +  // instead; or maybe it should be compiled with OPENSSL_NO_ENGINE to
 make
 +  // sure?  Just a thought.
  }
 }}}

 I've left this comment in-place while I investigate.  When I last looked
 at the openssl code, it appeared to be the case that OpenSSL's engines
 overrode any previous calls to set_rand_method, but that subsequent calls
 to set_rand_method overrode them.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17799#comment:37>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online