[patch] Remove unsafeAllowMissingParameters from _SGN_VerifyPKCS1DigestInfo.

NEW
Unassigned
(NeedInfo from)

Status

NSS
Libraries
P3
normal
a year ago
2 months ago

People

(Reporter: David Benjamin, Unassigned, NeedInfo)

Tracking

trunk

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Created attachment 8783380 [details] [diff] [review]
0001-Remove-unsafeAllowMissingParameters-from-_SGN_Verify.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.57 Safari/537.36

Steps to reproduce:

Give NSS an invalid RSASSA-PKCS1-v1_5 signature where the DigestInfo structure is missing the NULL parameter.


Actual results:

The signature is accepted.


Expected results:

The signature is rejected.

RFC 3447, section 8.2.2, steps 3 and 4 state that verifiers must encode the DigestInfo struct and then compare the result against the public key operation result. This implies that one and only one encoding is legal.

Bug #1064636 was about security consequences of NSS failing to do this. Per comment #22, due to the tight timeframe and unclear compatibility consequences, NSS decided to take a conservative approach and still allow one misencoded version (missing NULL parameter), and then tighten it up later. Per comment #133, the intent was to wait for data from BoringSSL as it became more widely deployed.

Since the original discussion, BoringSSL is now used in all ports of Chrome, recent versions of Android, and many of Google's other products. Go has also implemented the encoding correctly from the beginning. It appears the ecosystem is indeed compatible with correct implementations of RSASSA-PKCS1-v1_5.

Per https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00, NSS should follow suit so that it too may help grow a health ecosystem.

As a bonus, this code is now much cleaner, not having to deal with two different prefixes.

Comment 1

a year ago
David: In fairness, the use of that in BoringSSL doesn't really play into our signature validation, since that is handed off to (NSS, macOS, CryptoAPI)
(Reporter)

Comment 2

a year ago
That's true, but we've got evidence for X.509 by way of recent Androids (Chrome or other apps using the platform verifier) and CT.

Comment 3

a year ago
Even for Chrome, I would strongly call suspect using Android+BoringSSL to indicate success or failures, given the adoption rate of Android versions using BoringSSL (introduced in Version 6.0/API Level 23, aka Marshmallow, this represents only 15% of the Android ecosystem).

For CT, it would be useful for you to expand your comments. I can interpret it as one of two things:
1) The implication that some CT logs (presumably, Google) will reject such certificates as not passing signature checks
2) The historical data available within the CT logs that can be used to confirm or deny such signatures exist.

It would be useful to be precise. It's also worth noting that NSS, as a library, is also used in other products (e.g. all of RHEL's stuff, Thunderbird for S/MIME, etc), so we cannot a-priori claim that the change is safe because CT.


To be clear, I don't disagree with you at all that NSS should make this change, but I don't think we have any reasonable confidence that it won't create issues. As such, it likely means introducing yet-another-legacy-environment-flag to phase out (the same as has been done for, say, MD5), in order to satisfy RHEL&friends, since we can't know that ecosystems haven't deployed/come to rely on it, as improper as it is.

Updated

a year ago
Attachment #8783380 - Attachment is patch: true
Attachment #8783380 - Attachment mime type: text/x-patch → text/plain

Comment 4

a year ago
Comment on attachment 8783380 [details] [diff] [review]
0001-Remove-unsafeAllowMissingParameters-from-_SGN_Verify.patch

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

Actually, I have to r-, because it would be an ABI and API break (since introduced) to remove it.

secutil in NSS 3.17.1 exported this symbol, so it's now frozen. It would require a major rev to remove.

::: lib/util/pkcs1sig.h
@@ -24,5 @@
>   */
>  SECStatus _SGN_VerifyPKCS1DigestInfo(SECOidTag digestAlg,
>                                       const SECItem* digest,
> -                                     const SECItem* dataRecoveredFromSignature,
> -                                     PRBool unsafeAllowMissingParameters);

This is an exported API. It would break ABI compatability to remove this flag.

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/util/nssutil.def#276
Attachment #8783380 - Flags: review-
(Reporter)

Comment 5

a year ago
> For CT, it would be useful for you to expand your comments. I can interpret it as one of two things:
> 1) The implication that some CT logs (presumably, Google) will reject such certificates as not passing signature checks
> 2) The historical data available within the CT logs that can be used to confirm or deny such signatures exist.

I meant (1). Though I forget the timelines and it's possible the earlier entries the logs predate them using BoringSSL and (2) would apply to them. I assume at least one of the other logs out there uses OpenSSL (though I'm hoping to make them spec-compliant here too), if (2) is preferable.


> It would be useful to be precise. It's also worth noting that NSS, as a library, is also used in other products (e.g. all of RHEL's stuff, Thunderbird for S/MIME, etc), so we cannot a-priori claim that the change is safe because CT.

Sure. No change can a priori be claimed safe. Any change can potentially break something, and all software sees a slightly different set of users.

Notably this is true for the original change in bug #1064636 which was still made. From the discussion there, it seems both versions were allowed largely due to the then tight timeframe, and that there was hope that data from BoringSSL would be available.

The best we can do is gather what evidence we can (and the desired BoringSSL evidence has held well thus far) and, if good enough for what we'd gain, try it. I think the evidence is reasonable and draft-thomson-postel-was-wrong-00 (of notable authorship for this project) outlines the motivation pretty well. But obviously this is for NSS to decide.


> This is an exported API. It would break ABI compatability to remove this flag.

Ugh, really? This is a random implementation detail of a signature algorithm. It even has an underscore in front. Why was it exported?

I can make it ignore the parameter which will change behavior (but this patch is inherently a behavior change) but keep existing callers working. Or I can just change the two callers to PR_FALSE which is the minimal change as far as stability goes. It just means the cleanup can never happen, which is unfortunate. Which is preferable?

Comment 6

a year ago
(In reply to David Benjamin from comment #5)
> I meant (1). Though I forget the timelines and it's possible the earlier
> entries the logs predate them using BoringSSL and (2) would apply to them.

Correct, they do. That is, there are a number of certs in Google logs that were parsed using OpenSSL at one point in time.

> The best we can do is gather what evidence we can (and the desired BoringSSL
> evidence has held well thus far) and, if good enough for what we'd gain, try
> it. I think the evidence is reasonable and draft-thomson-postel-was-wrong-00
> (of notable authorship for this project) outlines the motivation pretty
> well. But obviously this is for NSS to decide.

Right, and again, I do what to emphasize that I think it would be good for NSS to do so, just with the right acknowledgement that we can't be as confident that it is as safe as was suggested on this issue.

> > This is an exported API. It would break ABI compatability to remove this flag.
> 
> Ugh, really? This is a random implementation detail of a signature
> algorithm. It even has an underscore in front. Why was it exported?

http://hg.mozilla.org/projects/nss/rev/fb7208e91ae8
https://bugzilla.mozilla.org/show_bug.cgi?id=1064636#c110

(it was never ack'd that it's safe to remove, which is the closest we can get)

> I can make it ignore the parameter which will change behavior (but this
> patch is inherently a behavior change) but keep existing callers working. Or
> I can just change the two callers to PR_FALSE which is the minimal change as
> far as stability goes. It just means the cleanup can never happen, which is
> unfortunate. Which is preferable?

That's up to Kai and Bob as how to how they're shipping NSS util. I believe the latter option is the safe/correct option, as that avoids needing to uprev NSS util while still getting the security benefits. My (very likely incorrect) understanding is that NSS util is part of/close to the FIPS security boundary, so actually removing it would prevent uprev'ing NSS util w/o breaking the FIPS module, until the FIPS module doesn't call back into it. But Kai and Bob can confirm for sure.
Flags: needinfo?(kaie)
(Reporter)

Comment 7

a year ago
Kai and Bob, how would you like to proceed? I'm happy to do whatever is appropriate stability-wise with the parameter. Obviously, for code health, remove parameter > ignore parameter > only change callers. But stability may dictate a different decision at code health's expense, so I defer to you all.

Updated

2 months ago
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.