1296986 - Disable parameter unsafeAllowMissingParameters in _SGN_VerifyPKCS1DigestInfo

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[David Benjamin]

Attachment: 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

[Ryan Sleevi]

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)

Comment 2

[David Benjamin]

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

[Ryan Sleevi]

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.

Comment 4

[Ryan Sleevi]

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

Comment 5

[David Benjamin]

> 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

[Ryan Sleevi]

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

Comment 7

[David Benjamin]

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.

Comment 8

[David Benjamin]

Friendly ping. There's been another two years of BoringSSL and Go only accepting standards-compliant signatures. Additionally, OpenSSL 1.1.0 is also standards-compliant.

Per https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00, please follow the standards in NSS as well.

Comment 9

[Eric Rescorla (:ekr)]

We really need to resolve this.

I concur with David that it would be better to just make this API compliant, but if the answer is that it can't, let's decide that and then we'll land the patch at the call sites instead. I propose we give Bob and Kai a week to respond one way or the other and then if no answer, ask David to upload the patch for the call sites and land it.

Bob? Kai?

Comment 10

[Kai Engert (:KaiE:)]

Likely, the reason for exporting the API was the NSS internal boundary between the util library part and the softokn library part, which are build and packaged independently on Fedora/RHEL.

I agree with Ryan's position that the API cannot be changed.

However, in a new version of NSS, we could declare that parameter unsafeAllowMissingParameters is deprecated, the real parameter will be ignored, and the implementation could always behave like the parameter was set to PR_FALSE. That's effectively what you'd like to achieve, right? E.g. Fedora and future, new major versions of RHEL could use the safer behavior.

The callers inside NSS should also be changed to explicitly set the parameter to PR_FALSE.

I currently don't know what backwards compatibility expectations we have. It might be useful, if the old behavior (allow parameter value PR_TRUE) could get activated using an optional compile time switch. This way, older OS branches in maintenance mode, that must update to newer NSS for Firefox, but which also must remain compatibility for this digest info issue, could easily keep the old behavior.

We can discuss this at Red Hat on our Wednesday meeting. We'll give you more input by Thursday.

Comment 11

[Robert Relyea]

Kai's analysis is correct. I think we should change NSS from PR_TRUE to PR_FALSE (either by changing the two callers, or changing the the function to ignore the parameter and always treat it as false). I believe we will have to execute a deprecation plan in older RHEL versions, so we would need at least one RHEL release to see if it breaks anyone (I'm not worried about the internet, but there could be issues on people's intranet). I think the chances are low, but it's a firedrill for us if we are wrong. Having the option to quickly revert on older versions of the OS would be preferable.

Comment 12

[David Benjamin]

Thanks! I'll update the patch to the version you all decide in your meeting then.

Comment 13

[Kai Engert (:KaiE:)]

Hello David, nothing new was discussed in the meeting. The previous comments correctly describe what we need:

- change the default to the more correct implementation

- also by default, make it impossible to use the less correct implementation

- keep the less correct implementation in the code,
  and allow it to be enabled using a compile time switch.

Thanks for offering to implement a new patch!

Comment 14

[David Benjamin]

A compile-time switches seem the worst of both worlds w.r.t. code cleanliness. In my original patch, unwinding unsafeAllowMissingParameters was rather non-trivial. If you need it to be easy to revert, it might make more sense to just change the call sites and then pick a date (6 months?) after which, if the patch didn't get reverted, the non-compliant mode can get removed and unsafeAllowMissingParameters turned into a no-op.

Comment 15

[Kai Engert (:KaiE:)]

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

>-    unsigned int innerSeqLen = 2 + hashOid->oid.len;
>+    unsigned int innerSeqLen = 2 + hashOid->oid.len + 2;
>     unsigned int outerSeqLen = 2 + innerSeqLen + 2 + digestLen;
>-    unsigned int extra = 0;
>-
>-    if (withParams) {
>-        innerSeqLen += 2;
>-        outerSeqLen += 2;
>-        extra = 2;
>-    }

Did you forget to increase outerSeqLen ?

Comment 16

[Kai Engert (:KaiE:)]

(In reply to David Benjamin from comment #14)
> A compile-time switches seem the worst of both worlds w.r.t. code
> cleanliness.

But that's what we require, and we'll need it for a much longer period than 6 months.


> In my original patch, unwinding unsafeAllowMissingParameters
> was rather non-trivial. If you need it to be easy to revert, it might make
> more sense to just change the call sites and then pick a date (6 months?)
> after which, if the patch didn't get reverted,

David, it isn't a matter of IF the patch will get reverted. We aren't suggesting that the primary NSS project should eventally revert it.

The intention is that some consumers, in particular maintainers of Linux distributions with long term stability promises, will revert it on those older branches. 

Reverting a patch will be non-trivial if NSS changes further. A compile time switch will simplify this need.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: 1296986-v2.patch

This is an example how to code it with minimal #ifdef clutter. It's just at one position in the code, assuming we keep the flexibility in the encodePrefix function. It uses most of the simplification from your patch.

Comment 18

[Kai Engert (:KaiE:)]

Attachment: 1296986-v3.patch

v3 fixes a mistake in v2

Comment 19

[David Benjamin]

Yes, I precisely meant reverted on those distros. Presumably there is some period of time, perhaps much longer than 6 months, after which NSS can reasonably assume that slower-moving distros have dealt with their transition plans and are ready to follow security standards.

My suggestion was to just change the call sites until that happens, which is less likely to bitrot (the PR_TRUE branch will at least be compiled). Maybe do a very small ifdef at the top of _SGN_VerifyPKCS1DigestInfo like:

#ifndef NSS_UNSAFE_ALLOW_MISSING_PKCS1_PARAMETERS
  unsafeAllowMissingParameters = PR_FALSE;
#endif

Otherwise the #ifdef'd code will likely break on accident due to lack of regular building.

But this isn't my project, so if you all prefer the larger #ifdefs, that works. I am more concerned about NSS's impact on ecosystem health, which any of these options satisfy.

Comment 20

[David Benjamin]

What's the status on this bug? It sounds like some long-term distros might prefer the non-compliant behavior in their NSS packages, perhaps even indefinitely. This is unfortunate, but I think the more important thing is that NSS upstream and new deployments of NSS use the standards-compliant behavior. Kai's patch achieves this, while allowing the less secure distros to do their thing. As does the even smaller version I suggested in comment #19.

Seems all that's left is for someone on the NSS team to pick whatever approach is preferred.

Comment 21

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8957093 [details] [diff] [review]
1296986-v3.patch

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

This looks to me like we could land it with the nit addressed.

::: lib/util/pkcs1sig.c
@@ +129,5 @@
> +                }
> +                rv = encodePrefix(hashOid, digest->len, &prefix, PR_FALSE);
> +                if (rv != SECSuccess) {
> +                    lengthMismatch = PR_FALSE;
> +                } else if (dataRecoveredFromSignature->len ==

This should go into one if clause to avoid duplicating the `lengthMismatch = PR_FALSE` line.

Comment 22

[Kai Engert (:KaiE:)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #21)
> > +                }
> > +                rv = encodePrefix(hashOid, digest->len, &prefix, PR_FALSE);
> > +                if (rv != SECSuccess) {
> > +                    lengthMismatch = PR_FALSE;
> > +                } else if (dataRecoveredFromSignature->len ==
> 
> This should go into one if clause to avoid duplicating the `lengthMismatch =
> PR_FALSE` line.

You want it like this, right?

                rv = encodePrefix(hashOid, digest->len, &prefix, PR_FALSE);
                if (rv != SECSuccess ||
                    dataRecoveredFromSignature->len == prefix.len + digest->len) {
                    lengthMismatch = PR_FALSE;
                }

Comment 23

[Kai Engert (:KaiE:)]

Attachment: 1296986-test-v3.patch

Also, we should take the test that David has implemented.

This attachment is the test subset of David's original patch. I suggest to simply wrap it with
  #ifndef NSS_PKCS1_AllowMissingParameters

That will enable the test for the new and modern default. The backward compatible mode will remain untested, as it is today.

Comment 24

[Kai Engert (:KaiE:)]

Comment on attachment 8994883 [details] [diff] [review]
1296986-test-v3.patch

Actually, we should be able to use test #ifdef NSS_PKCS1_AllowMissingParameters.

The "valid signature" test should be successful in both scenarios.

The "invalid signature" should fail by default, and pass #ifdef NSS_PKCS1_AllowMissingParameters.

Comment 25

[Kai Engert (:KaiE:)]

Attachment: 1296986-test-v4.patch

This is mostly David's test code.

It adds the test to gyp. It supports the environment variable NSS_PKCS1_AllowMissingParameters to make it easier to build with it when using "make" (forwards to -D). It uses the #idef at the end of the test code.

Comment 26

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8994970 [details] [diff] [review]
1296986-test-v4.patch

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

Right I wanted to ask for a test :)
r+ with the nits addressed.

::: gtests/pk11_gtest/pk11_rsapkcs1_unittest.cc
@@ +14,5 @@
> +
> +namespace nss_test {
> +
> +static unsigned char* toUcharPtr(const uint8_t* v) {
> +  return const_cast<unsigned char*>(static_cast<const unsigned char*>(v));

This is defined in cpputil.h already.

@@ +93,5 @@
> +  SECItem sigItem = {siBuffer, toUcharPtr(kSignature), sizeof(kSignature)};
> +  SECStatus rv = VFY_VerifyDigestDirect(&hash, pubKey.get(), &sigItem,
> +                                        SEC_OID_PKCS1_RSA_ENCRYPTION,
> +                                        SEC_OID_SHA256, nullptr);
> +  EXPECT_EQ(rv, SECSuccess);

For all EXPECT_*, the expected value comes first.

Comment 27

[Kai Engert (:KaiE:)]

Checked in with nits address, thanks for the reviews.
https://hg.mozilla.org/projects/nss/rev/be5c5d3ad5f6