Incorrect log message in token_check_object
[00:49:06] <wanoskarnet> token_check_object(): "Wrong size on key for %s: %d bits". crypto_pk_keysize() counts a bytes not a bits.Activity
changed milestone to %Tor: 0.2.2.x-final
Replying to nickm:
Well, if we really care that not all 128-byte keys will really have 1024 bits of modulus, then fixing the log message alone isn't enough: we should fix the check too.
I care about never lying to the user. We can't check the key size in bits because OpenSSL's documentation says it can generate keys shorter than 1024 bits when it is told to generate a 1024-bit key.
Replying to rransom:
Replying to nickm:
Well, if we really care that not all 128-byte keys will really have 1024 bits of modulus, then fixing the log message alone isn't enough: we should fix the check too.
I care about never lying to the user. We can't check the key size in bits because OpenSSL's documentation says it can generate keys shorter than 1024 bits when it is told to generate a 1024-bit key.
Can you point me to the place in the openssl documentation that says this? If so, it would seem to be a flaw in openssl that we should work around. But I just took a safari through the openssl 1.0.0d RSA_generate_key code, and found that the numbers we're using for p and q ultimately come out of BN_rand(p, 512, 1, 1), which makes it so that the two most significant bits of the output are always 1. To make sure this isn't a new innovation, I checked openssl 0.9.6h, and it was true there too.
When I ran this patch code, Tor didn't complain about any keys of incorrect key length. Also, I tried writing a quick program to generate RSA keys and count their bits: so far it's made ~31k keys without any of an unexpected length.
-
rransom says (quoted with permission):
17:19 < rransom> re #3318: I am now very confused about where I read that. I know I saw that in the documentation of some piece of crypto code, and I thought it was OpenSSL. 17:20 < nickm> It's conceivable that very old versions once had a problem 17:20 < nickm> though I can now confirm that the 0.9.2 and later seem to have the code to do the right thing 17:20 < rransom> Or that I was reading comments/documentation in GPG (or maybe even libtomcrypt/libtommath). 17:21 < rransom> In that case, be more fascist about key length.Thus, merging. (I force-pushed a tweak in the branch that adds a newline at the end of the changes file, so arma isn't sad.)
Trac:
Resolution: N/A to fixed
Status: needs_review to closed moved to tpo/core/tor#3318 (closed)