Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#3318 closed defect (fixed)

Incorrect log message in token_check_object

Reported by: rransom Owned by:
Priority: Very Low Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: easy tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

[00:49:06] <wanoskarnet> token_check_object(): "Wrong size on key for %s: %d bits". crypto_pk_keysize() counts a bytes not a bits.

Child Tickets

Change History (8)

comment:1 Changed 9 years ago by nickm

Resolution: fixed
Status: newclosed

Fixed in 0fd3ad75daf925e8192aa1d44b229b3b7c29829d

comment:2 in reply to:  1 Changed 9 years ago by rransom

Resolution: fixed
Status: closedreopened

Replying to nickm:

Fixed in 0fd3ad75daf925e8192aa1d44b229b3b7c29829d

Not quite. See bug3318b ( git://git.torproject.org/rransom/tor.git bug3318b ).

comment:3 Changed 9 years ago by nickm

Status: reopenedneeds_review

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.

How do you like branch bug3318c in my public repository?

comment:4 in reply to:  3 ; Changed 9 years ago by 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.

comment:5 in reply to:  4 Changed 9 years ago by nickm

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.

comment:6 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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.)

comment:7 Changed 7 years ago by nickm

Keywords: tor-client added

comment:8 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.