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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See also https://rt.openssl.org/Ticket/Display.html?id=3665.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: signature-signatureAlgorithm.patch

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.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: signature-signatureAlgorithm.patch [v2]

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.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8561736 [details] [diff] [review]
signature-signatureAlgorithm.patch [v2]

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

LGTM.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Never consider OCSP response signer certs to be trust anchors

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.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add tests and new error code

Antoine, you may be interested in the test cases and the special behavior for RSA-with-SHA1.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 10

[:Cykesiopka]

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)

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Oh yeah - good catch.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce10ba72485b

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: update nsserrors.properties so error message gets localized

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f92594c0d0a

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d49d558961b
https://hg.mozilla.org/mozilla-central/rev/ce10ba72485b
https://hg.mozilla.org/mozilla-central/rev/5f92594c0d0a