Check that tbsCertificate.signature and signatureAlgorithm are equal in mozilla::pkix

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

The tbsCertificate.signature and signatureAlgorithm fields of a certificate are supposed to be equal. I think the traditional rationale for this is somewhat dubious, but Antoine suggested it would be a good idea, and Certificate Transparency depends on this being the case.

It isn't enough to compare the results of calling SignatureAlgorithm on each value to each other, because the SignatureAlgorithm function maps multiple encodings (e.g. AlgorithmIdentifiers with or without NULL, or multiple OIDs that identify the same algorithm) to the same value in some situations. Instead, we need to do a byte-for-byte comparison of the two values.

The code is in pkixcert.cpp, and the issue is already called out:

  // XXX: Ignored. What are we supposed to check? This seems totally redundant
  // with Certificate.signatureAlgorithm. Is it important to check that they
  // are consistent with each other? It doesn't seem to matter!

Probably the best way to implement this would be to change der::SignedData to take an optional Input* encodedSignatureAlgorithm argument. When that argument is supplied, it would initialize encodedSignatureAlgorithm. Then, in pkixcert.cpp, we could use InputsAreEqual to compare the two values, after parsing tbsCertificate.signature. Input::GetMark and Input::GetInput(const Mark& mark, /*out*/ Input& item) are probably useful for this.

Hopefully none of this matters much for security, because OCSP responses don't have a signed indication of the algorithm being used.
See also
https://github.com/openssl/openssl/commit/684400ce192dac51df3d3e92b61830a6ef90be3e and
https://www.openssl.org/news/secadv_20150108.txt and
https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2014-8275.

Honestly, I don't understand CVE-2014-8275. The issue seems to be that some software might calculate a fingerprint that covers both the signed and unsigned portion of the certificate, and then use that fingerprint for some security-sensitive check. But, it's dumb to calculate fingerprints that way; security-sensitive fingerprints should be calculated only over the signed portion. So, it seems like a non-issue, as far as it was described by the OpenSSL people. Maybe I'm overlooking something though; I would love to be educated, if so.
Assignee: nobody → brian
Status: NEW → ASSIGNED
OS: Windows 8.1 → All
Hardware: x86 → All
Target Milestone: --- → mozilla38
I'll add the tests in a separate patch. Just want to make sure you agree with the proposed implementation, in particular the comparison being done after parsing instead of bytewise.
Attachment #8561719 - Flags: review?(dkeeler)
Attachment #8561719 - Flags: feedback?(antoine)
I'd forgotten that we allow roots to use signature algorithms that we don't normally support, causing some of the existing tests to fail. Here's a new version that fixes that.
Attachment #8561719 - Attachment is obsolete: true
Attachment #8561719 - Flags: review?(dkeeler)
Attachment #8561719 - Flags: feedback?(antoine)
Attachment #8561736 - Flags: review?(dkeeler)
Attachment #8561736 - Flags: feedback?(antoine)
Comment on attachment 8561736 [details] [diff] [review]
signature-signatureAlgorithm.patch [v2]

Review of attachment 8561736 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8561736 - Flags: review?(dkeeler) → review+
Since we always verify the signature of OCSP signer certs, and since they are only ever trusted if their issuer is trusted, we should always be doing all the checks we normally do for certs that inherit trust.
Attachment #8564632 - Flags: review?(dkeeler)
Comment on attachment 8564632 [details] [diff] [review]
Never consider OCSP response signer certs to be trust anchors

Review of attachment 8564632 [details] [diff] [review]:
-----------------------------------------------------------------

Good call.
Attachment #8564632 - Flags: review?(dkeeler) → review+
Antoine, you may be interested in the test cases and the special behavior for RSA-with-SHA1.
Attachment #8567756 - Flags: review?(dkeeler)
Comment on attachment 8567756 [details] [diff] [review]
Add tests and new error code

Review of attachment 8567756 [details] [diff] [review]:
-----------------------------------------------------------------

Great - r=me.

::: security/pkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This code is made available to you under your choice of the following sets
> +* of licensing terms:

nit: I think commonly the '*' are aligned in these comments.
Attachment #8567756 - Flags: review?(dkeeler) → review+

Comment 10

4 years ago
Comment on attachment 8567756 [details] [diff] [review]
Add tests and new error code

Review of attachment 8567756 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixnss.cpp
@@ +193,5 @@
>        "The server presented a certificate that is not yet valid." },
>      { "MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE",
>        "A certificate that is not yet valid was used to issue the server's "
>        "certificate." },
> +    { "MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH",

Shouldn't this be accompanied by an addition to nsserrors.properties as well?
(Unless it gets added in another patch somewhere)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d49d558961b

(In reply to David Keeler [:keeler] (use needinfo?) from comment #9)
> nit: I think commonly the '*' are aligned in these comments.

Text editor was trying to be to smart. Fixed.

(In reply to Cykesiopka from comment #10)
> Shouldn't this be accompanied by an addition to nsserrors.properties as well?
> (Unless it gets added in another patch somewhere)

I didn't see this comment in my bugmail before I pushed the changeset to inbound. I will attach a follow-up patch to do that to this bug.
You need to log in before you can comment on or make changes to this bug.