Opened 8 years ago

Closed 7 years ago

#5170 closed defect (fixed)

crypto_pk_get_digest (et al.?) use i2d_RSAPublicKey obsoletely

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

Description

i2d_RSAPublicKey now supports (in OpenSSL 0.9.7 or later, so in all OpenSSL versions we support) allocating its own output buffer (using OPENSSL_malloc). Is this feature worth using in crypto_pk_get_digest (and some other functions which call i2d_RSAPublicKey twice to allocate a temporary buffer)?

(See the i2d_X509(3ssl) man page.)

Child Tickets

Change History (21)

comment:1 Changed 8 years ago by rransom

(If we don't make this change, we should make crypto_pk_get_digest et al. use our crypto_pk_asn1_encode utility function.

comment:2 Changed 8 years ago by nickm

Milestone: Tor: unspecified

Worth doing; not necessary for 0.2.3.x. Anything that gets allocated by OPENSSL_malloc needs to get freed with OPENSSL_free.

comment:3 Changed 8 years ago by nickm

Keywords: tor-client added

comment:4 Changed 8 years ago by nickm

Component: Tor ClientTor

comment:5 Changed 7 years ago by marek

Status: newneeds_review

Patch attached. crypto_pk_asn1_encode and crypto_pk_get_digest seem to be covered by src/test/test but crypto_pk_get_all_digests and the tool id_to_fp.c aren't. The latter is not even compiled as a part of build process. I might use some help with testing those.

comment:6 Changed 7 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.5.x-final

comment:7 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

Thanks! QUick review:

IMO it's okay to remove id_to_fp.c in 0.2.5 if it's not even compiled.

The "return -1" cases need to free the buffer if the buffer is non-NULL.

In addition to the 8 calls to i2d_RSAPublicKey, there are 2 calls to i2d_DHParams, 2 calls to i2d_X509, and 4 calls to i2d_PublicKey. Probably, they could be simplified as well.

comment:8 in reply to:  7 Changed 7 years ago by marek

Replying to nickm:

IMO it's okay to remove id_to_fp.c in 0.2.5 if it's not even compiled.

I've updated the patch.

The "return -1" cases need to free the buffer if the buffer is non-NULL.

Thanks. I got it wrong in one place, but except for that I disagree. Although the specific behaviour is undocumented, if I can read the openssl sources that's the relevant code: https://github.com/majek/openssl/blob/2f58cda4ce45c88f5c1d284eb155cbc9bcf4705f/crypto/asn1/tasn_enc.c#L111

Which reads: buf will only be !=NULL If returned len >=0. IE: we don't need to free buf when len is -1.

Changed 7 years ago by marek

comment:9 Changed 7 years ago by marek

Modified functions:

  • covered by src/test/test: crypto_pk_asn1_encode, crypto_pk_get_digest
  • not covered: crypto_pk_get_all_digests, crypto_store_dynamic_dh_modulus, tor_cert_new, pkey_eq (used via tor_tls_cert_matches_key).

Any idea how to test the latter?

I can't find a manual page oni2d_PublicKey confirming that the buffer semantics are the same as for i2d_X509. It's only a guess that it actually works the way @nickm suggested.

pkeq_eq contained a bug - return value of i2d_PublicKey was not checked. If both certs were invalid (return == -1) then it would run tor_memeq(..,.., (size_t)-1). In updated code I assume that the certificates don't match if both are invalid.

comment:10 Changed 7 years ago by nickm

crypto_pk_get_all_digests should be simple to write a test for: just add one to src/test/test_crypto.c that takes a pregenerated key (you can use the one encoded as AUTHORITY_SIGNKEY_1, or include another one as a static string) and compares the output to a known-good values.

It should also be not too hard to write pkey_eq similarly -- for those, you don't even need canned public keys. (If you want, you can rename it and expose it, or use the "#ifdef FILENAME_PRIVATE" trick we do elsewhere to make the identifier exposed for testing only.

tor_cert_new() will have the same static-function issue. For crypto_store_dynamic_dh_modulus, the trick will be getting a filename -- in the unit tests, we do temporary filenames with the "get_fname()" function.

Let me know if you have any other questions there

comment:11 Changed 7 years ago by marek

Added tests for everything except crypto_store_dynamic_dh_modulus and tor_cert_new. Writing tests for those functions would be cumbersome and pretty much useless - we would end up testing openssl i2d/d2i functions.

I'm pretty happy with the code now.

(View changes as a single patch, for easy review: https://gist.github.com/majek/5750346 , the changes are under bug5170 branch on git://github.com/majek/tor.git )

comment:12 Changed 7 years ago by nickm

This is looking pretty solid to me. Just a thing on one of the untested functions:

  • The unconditional openssl_free at the end of crypto_store_dynamic_dh_modulus should probably be conditional on dh_string_repr being non-null.

Other than that, looks good to me. I'll merge the branch and patch that. Thanks!

comment:13 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Merged that, plus 7f9066ceee32f4df6d94b18c7b65e92d1730fce8 and c94f6b228b0f6296a1fd74a5e12373754d5f07a9; thanks!

Minor tips:

  • "make check-spaces" will tell you about some kinds of deviations from Tor's coding style.
  • Thanks for the link to a git branch! Once there are more than one or two patches in a branch, it's usually easier to merge the branch than to "git am" the patches, at least for me.
  • When giving people links to github branches, it's sometimes a better idea to give https:// links than git:// links -- the https:// links are authenticated.
Note: See TracTickets for help on using tickets.