crypto_pk_get_digest (et al.?) use i2d_RSAPublicKey obsoletely

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

changed milestone to %Tor: 0.2.5.x-final

added component::core tor/tor easy milestone::Tor: 0.2.5.x-final priority::very low resolution::fixed status::closed tor-client type::defect labels

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

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

Trac:
Milestone: N/A to Tor: unspecified

Trac:
Keywords: easy deleted, easy tor-client added

Trac:
Component: Tor Client to Tor

Trac:
Username: marek

0001-Fix-for-bug-5170-i2d_RSAPublicKey-can-allocate-its-o.patch

Trac:
Username: marek

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.

Trac:
Username: marek
Status: new to needs_review

Trac:
Milestone: Tor: unspecified to Tor: 0.2.5.x-final

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.

Trac:
Status: needs_review to needs_revision

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.

Trac:
Username: marek

Trac:
Username: marek

0001-Bug-5170-remove-id_to_fp.c-as-it-s-not-used-anywhere.patch

Trac:
Username: marek

Trac:
Username: marek

0002-Bug-5170-i2d_RSAPublicKey-supports-allocating-its-ow.patch

Trac:
Username: marek

Trac:
Username: marek

0003-Bug-5170-also-simplify-i2d_DHparams.patch

Trac:
Username: marek

Trac:
Username: marek

0004-Bug-5170-simplify-i2d_X509.patch

Trac:
Username: marek

Trac:
Username: marek

0005-Bug-5170-simplify-i2d_PublicKey-in-pkey_eq.patch

Trac:
Username: 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.

Trac:
Username: marek

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

Trac:
Username: marek

0006-Bug-5170-test-crypto_pk_get_all_digests.patch

Trac:
Username: marek

Trac:
Username: marek

0007-Bug-5170-make-pkey_eq-testable-introduce-test_tortls.patch

Trac:
Username: 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 )

Trac:
Username: marek

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!

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.

Trac:
Resolution: N/A to fixed
Status: needs_revision to closed

closed

mentioned in issue #9043 (moved)

moved to tpo/core/tor#5170 (closed)