[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #17799 [Core Tor/Tor]: Hash All PRNG output before use
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs