Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#7651 defect closed fixed (fixed)

test_inspectCertificate fails on recent versions of OpenSSL because the "public key hash" differs

Reported by: Glyph Owned by: Glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: Tristan Seligmann Branch: branches/inconsistent-hash-7651
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

I recently upgraded some builders from an old, unsupported version of Ubuntu (Natty) to a fairly modern version (Trusty) that previously did not have any coverage in our build fleet.

In so doing, I got this traceback:

[FAIL]
Traceback (most recent call last):
  File "/var/lib/buildbot/bot-glyph-1/ubuntu64-py2.6-epoll/Twisted/twisted/test/test_sslverify.py", line 886, in test_inspectCertificate
    "Public Key with Hash: ff33994c80812aa95a79cdb85362d054"])
  File "/var/lib/buildbot/bot-glyph-1/ubuntu64-py2.6-epoll/Twisted/twisted/trial/_synctest.py", line 447, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = ['Certificate For Subject:',
 '               Common Name: example.twistedmatrix.com',
 '              Country Name: US',
 '             Email Address: nobody@twistedmatrix.com',
 '             Locality Name: Boston',
 '         Organization Name: Twisted Matrix Labs',
 '  Organizational Unit Name: Security',
 '    State Or Province Name: Massachusetts',
 '',
 'Issuer:',
 '               Common Name: example.twistedmatrix.com',
 '              Country Name: US',
 '             Email Address: nobody@twistedmatrix.com',
 '             Locality Name: Boston',
 '         Organization Name: Twisted Matrix Labs',
 '  Organizational Unit Name: Security',
 '    State Or Province Name: Massachusetts',
 '',
 'Serial Number: 12345',
 'Digest: C4:96:11:00:30:C3:EC:EE:A3:55:AA:ED:8C:84:85:18',
 'Public Key with Hash: 4f64143c741c8a3fd41012bc48ec1651']
b = ['Certificate For Subject:',
 '               Common Name: example.twistedmatrix.com',
 '              Country Name: US',
 '             Email Address: nobody@twistedmatrix.com',
 '             Locality Name: Boston',
 '         Organization Name: Twisted Matrix Labs',
 '  Organizational Unit Name: Security',
 '    State Or Province Name: Massachusetts',
 '',
 'Issuer:',
 '               Common Name: example.twistedmatrix.com',
 '              Country Name: US',
 '             Email Address: nobody@twistedmatrix.com',
 '             Locality Name: Boston',
 '         Organization Name: Twisted Matrix Labs',
 '  Organizational Unit Name: Security',
 '    State Or Province Name: Massachusetts',
 '',
 'Serial Number: 12345',
 'Digest: C4:96:11:00:30:C3:EC:EE:A3:55:AA:ED:8C:84:85:18',
 'Public Key with Hash: ff33994c80812aa95a79cdb85362d054']


twisted.test.test_sslverify.OpenSSLOptions.test_inspectCertificate

I think this may be related to some of the failures in #7370; at least one of the failure logs looks like it may have included a snippet of this test failing.

This "public key hash" is computed by creating a new, blank certificate signing request, populating its public key field and nothing else, exporting it as ASN1, and then getting MD5 hex digest of those bytes. This has been deterministic for quite a while, but now it seems that something ever so slightly different happens.

Entirely coincidentally, a few days before I did this, Apple released OS X 10.9.5, which happens to upgrade from OpenSSL 0.9.8y to 0.9.8za. It seems that this version hop has exactly the same property that the version hop between every other buildbot we have with OpenSSL and whatever is in today's version of Trusty (1.0.something plus random ubuntu patches so it's impossible to tell which version it corresponds to).

To wit, the hash of this particular key has changed from ff33994c80812aa95a79cdb85362d054 to 4f64143c741c8a3fd41012bc48ec1651.

It may just be that this is a 1024-bit key and so OpenSSL is now emitting garbage because such a key is too small to be usefully secure. It looks like the key being tested in these tests ought to be the same one that exarkun generated for #7370 rather than the same (apparently separate?) fixed value it's been since 2006.

Change History (14)

comment:1 Changed 7 years ago by Glyph

I've put this in the milestone because the buildbots are failing and given that this is a change in behavior due to a security update, more buildbots are likely to follow.

comment:2 Changed 7 years ago by Glyph

Also, in case the description doesn't make it obvious, I think this is probably a dumb way to get the hash of the public key portion of a certificate. I really hope someone else can recommend a better idea.

comment:3 Changed 7 years ago by Glyph

Experimenting with twistedmatrix.com's public cert, this dumb technique for getting the "public key hash" isn't stable for larger, saner keys across this particular version boundary, so we probably need to come up with a better way to do it or abandon the feature entirely.

comment:4 Changed 7 years ago by Glyph

(Apparently the way to get the actual public key bytes out would be to use the basically undocumented functions in OpenSSL with names like i2d_RSAPublicKey_bio. I'm not sure there is a “good” solution here. Perhaps dropping the feature would be the most expedient thing.)

comment:5 Changed 7 years ago by Glyph

Author: glyph
Branch: branches/inconsistent-hash-7651

(In [43152]) Branching to inconsistent-hash-7651.

comment:6 Changed 7 years ago by Glyph

Keywords: review added

The only build failure I see now is a reference error in the docs, because we can't link to PyCrypto yet. Ignoring those.

comment:7 Changed 7 years ago by Tristan Seligmann

Cc: Tristan Seligmann added
Keywords: review removed
Owner: set to Glyph
  1. You're missing a news fragment. I think perhaps the fact that the return value of keyHash will change should be mentioned here.
  2. I can't really comment on the new method for calculating the hash since I know virtually nothing about OpenSSL. Insofar as I can understand the code, it looks like an improvement over the old method, even if it's still somewhat hackish.

Please merge once you've added a news fragment, unless you are expecting some additional commentary/review from someone with more OpenSSL expertise than I have.

comment:8 Changed 7 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [43179]) Merge inconsistent-hash-7651: fix test_inspectCertificate

Author: glyph

Reviewer: mithrandi

Fixes: #7651

twisted.internet.ssl.Certificate(...).getPublicKey().keyHash() now produces a stable value regardless of OpenSSL version. Unfortunately this means that it is different than the value produced by older Twisted versions.

comment:9 Changed 7 years ago by reaperhulk

Fingerprints of just public key bytes are not something I've seen in a crypto API. In the future pyca/cryptography could offer it from the OpenSSL backend, but I'd like to know more about the use case here. Is it to compare public keys for equality?

In the given example of twisted.internet.ssl.Certificate(...).getPublicKey().keyHash() you would typically ask for the certificate's fingerprint, which is a hash of the entire DER encoded certificate (which, unlike private/public keys, has only one canonical representation, hooray!). However, it would be somewhat interesting to check the public key value, as PKI frequently has cases where multiple certificates have the same private key (most notably in self-sign/cross-sign situations).

comment:10 Changed 7 years ago by reaperhulk

In the interest of pedantry I should note that the certificate digest is distinct from the certificate fingerprint. The digest is computed over the tbsCertificate ASN.1 payload and not the entire ASN.1 DER structure.

comment:11 in reply to:  9 ; Changed 7 years ago by Glyph

Replying to reaperhulk:

Fingerprints of just public key bytes are not something I've seen in a crypto API. In the future pyca/cryptography could offer it from the OpenSSL backend, but I'd like to know more about the use case here. Is it to compare public keys for equality?

Yes, precisely.

This API is not very important; frankly, if I were designing this today, I'm not sure I would include it. But to the extent that it's useful, it's useful to be able to identify a public key and compare it over a slightly longer lifetime than a single process :-).

Consider the use case of a server compromised by heartbleed. If the admin rolls a new cert, but re-uses the same key materials, maybe I'd like to know that. That seems like it would be pretty bad! Without a public key as a discretely identifiable object, I can't tell that that has happened.

In the given example of twisted.internet.ssl.Certificate(...).getPublicKey().keyHash() you would typically ask for the certificate's fingerprint, which is a hash of the entire DER encoded certificate (which, unlike private/public keys, has only one canonical representation, hooray!).

I'm not sure what you mean by "the given example". If you wanted something related to the certificate, then you'd use the digest (available as Certificate.digest), right?

Except uh

In the interest of pedantry I should note that the certificate digest is distinct from the certificate fingerprint. The digest is computed over the tbsCertificate ASN.1 payload and not the entire ASN.1 DER structure.

what

What is the distinction here, semantically? I thought maybe that the signature wasn't included in that part of the structure, but this random link I found suggests strongly that it is.

However, it would be somewhat interesting to check the public key value, as PKI frequently has cases where multiple certificates have the same private key (most notably in self-sign/cross-sign situations).

Yes, precisely.

comment:12 in reply to:  11 ; Changed 7 years ago by reaperhulk

Replying to glyph:

Consider the use case of a server compromised by heartbleed. If the admin rolls a new cert, but re-uses the same key materials, maybe I'd like to know that. That seems like it would be pretty bad! Without a public key as a discretely identifiable object, I can't tell that that has happened.

This is a great reason to want a public key hash/fingerprint.

I'm not sure what you mean by "the given example". If you wanted something related to the certificate, then you'd use the digest (available as Certificate.digest), right?

Correct, sorry for the confusion. I was trying to state that the certificate fingerprint already exists and does contain the public key (along with other data), but your use case requires identification of purely the public key.

In the interest of pedantry I should note that the certificate digest is distinct from the certificate fingerprint. The digest is computed over the tbsCertificate ASN.1 payload and not the entire ASN.1 DER structure.

what

What is the distinction here, semantically? I thought maybe that the signature wasn't included in that part of the structure, but this random link I found suggests strongly that it is.

The ASN.1 for a certificate is defined by RFC 5280 (http://tools.ietf.org/html/rfc5280) as:

Certificate ::= SEQUENCE {

tbsCertificate TBSCertificate, signatureAlgorithm AlgorithmIdentifier, signatureValue BIT STRING }

The signature itself and signature algorithm OID are not inside the tbsCertificate structure (tbs stands for "to be signed"). Confusingly, tbsCertificate *does* contain a field called "Signature", but it is actually an OID algorithmIdentifier.

comment:13 in reply to:  12 Changed 7 years ago by Glyph

Replying to reaperhulk:

Replying to glyph:

The ASN.1 for a certificate is defined by RFC 5280 (http://tools.ietf.org/html/rfc5280) as:

Certificate ::= SEQUENCE {

tbsCertificate TBSCertificate, signatureAlgorithm AlgorithmIdentifier, signatureValue BIT STRING }

The signature itself and signature algorithm OID are not inside the tbsCertificate structure (tbs stands for "to be signed"). Confusingly, tbsCertificate *does* contain a field called "Signature", but it is actually an OID algorithmIdentifier.

Gotcha.

As I understand it now, the only thing that could happen which would change the digest (hash of the TBSCertificate) and not the fingerprint (hash of the Certificate) is a change to the signature algorithm. The TBSCertificate contains both issuer and subject, so a new certificate from a new issuer will necessarily have a different digest anyway. Although the fact that the signature is not included means you could have a digest of a certificate that was not actually signed (?) so you have to be careful not to use the digest as authoritatively conferring some property.

comment:14 Changed 6 years ago by hawkowl

Milestone: Twisted 14.1.0

Ticket retargeted after milestone deleted

Note: See TracTickets for help on using tickets.