Closed Bug 715073 (CVE-2012-0441) Opened 12 years ago Closed 12 years ago

Insufficient length checking in QuickDER decoder

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(firefox9 wontfix, firefox10- wontfix, firefox11- wontfix, firefox12- wontfix, firefox13+ verified, firefox14+ verified, firefox-esr1013+ verified, blocking1.9.2 needed, status1.9.2 wanted)

VERIFIED FIXED
3.13.4
Tracking Status
firefox9 --- wontfix
firefox10 - wontfix
firefox11 - wontfix
firefox12 - wontfix
firefox13 + verified
firefox14 + verified
firefox-esr10 13+ verified
blocking1.9.2 --- needed
status1.9.2 --- wanted

People

(Reporter: mozbgz, Assigned: mozbgz)

References

Details

(Whiteboard: [sg:moderate][advisory-tracking+])

Crash Data

Attachments

(5 files, 4 obsolete files)

Attached patch Possible fix (obsolete) — Splinter Review
I'm afraid I have to begin the new year with not so nice news - the "QuickDER" ASN.1 decoder in NSS has a flaw with rather serious consequences: while it rejects bogus (oversized) lengths when trying to match a buffer against a given template, it fails to properly detect a number of zero-length items which are illegal as per X.690.

This can cause all sorts of troubles: one of them is reported in bug 697420 (which happened to be the starting point for my discovery, actually). Depending on what exactly NSS - or the application - does after having called SEC_QuickDERDecodeItem, nasty bugs will occur, such as:

1) CERT_DecodeOCSPResponse() will mistakenly assume that it has successfully parsed a response, even if that response includes a zero-length ENUMERATED value. It will return the response to the caller, and when code subsequently - and rightfully - tries to clean up said response, CERT_DestroyOCSPResponse() will crash... because it's getting a bogus pointer from ocsp_GetResponseSignature(response), which it tries to dereference.

2) CERT_DecodeBasicConstraintValue is another case where illegal zero-length items lead to a serious problem: if a certificate includes a basicConstraints extension where the "CA" BOOLEAN is encoded with zero length, NSS mistakenly treats such a cert as a CA certificate (not as bad as Moxie's findings from 2002 about MS CAPI and basicConstraints, but still, quite worrying).

I stopped looking for further issues at that point, but it would be quite surprising if these were the only ones. From my reading of X.690, of the 25 ASN.1 UNIVERSAL types currently recognized/defined in secasn1t.h, the following 7 can never have zero length (when properly DER encoded): BOOLEAN, INTEGER, BIT STRING, OBJECT_IDENTIFIER, ENUMERATED, UTCTime, GeneralizedTime. QuickDER should abort further processing when the template specifies one of these types and the buffer being processed holds such an illegal encoding.

It's probably debatable what level of content checking should go into QuickDER (and what checks are better done in code which actually parses the values, such as DER_GetInteger), but some minimal zero-length sanity checks seem justified in QuickDER. The attached patch is a possible approach; note that due to re-indentation, the changes might appear larger at first sight than they effectively are.

A demo "exploit" for the two issues mentioned above can be seen by visiting https://orod.velox.ch. First, the CA cert attached to this bug needs to be installed and trusted for SSL. A default setup of Firefox will then crash immediately in CERT_DestroyOCSPResponse() when opening the above URL. Note that the "CA cert" isn't really a certificate with "CA" set to TRUE, but NSS considers it a valid CA cert.

As ASN.1 is at the heart of many parts of NSS (there are dozens of SEC_QuickDERDecodeItem calls in lib/, X.509/PKIX processing heavily depends on it), the potential impact of this issue should not be underestimated. Applications - and NSS itself - must be able to rely on SEC_QuickDERDecodeItem() rejecting invalid ASN.1 data, when asked to match this input against a given template. Note that the "classic" DER decoder, i.e. SEC_ASN1DecodeItem, has checks for illegal zero-length items, and will e.g. reject an OCSP response where the response status isn't encoded properly.

Finally, let me point out that the patch also includes a second, belt-and-suspenders type fix: when returning a "dest" item with zero length (for those ASN.1 types where this is acceptable as per X.690, such as strings), QuickDER should not fill bogus pointers into dest->data - if code dealing with a SECItem afterwards only checks for a non-NULL data pointer without looking at the SECItem's length at the same time, even more subtle bugs may surface (the CERT_DecodeBasicConstraintValue actually being an example of this).

I did not yet request a CVE ID number for this issue - let me know if I should do this, or if Mozilla can get an assignment.
We can assign a CVE number when we get our CVE-2012- allotment.
might only be [sg:high] but assuming the worst.
Whiteboard: [sg:critical]
blocking1.9.2: --- → ?
The OCSP crash doesn't appear particularly scary (crash due to near-NULL read)
bp-6b96473b-ad95-4448-a3c9-be59f2120112

It's happening in Thunderbird, too:
https://crash-stats.mozilla.com/report/list?signature=CERT_DestroyOCSPResponse

Another real-world instance of this crash appears to be bug 707873

I'm more worried about Kaspar's "2)" -- could someone get a malicious EE cert constructed in this way signed by a trusted CA?
Blocks: 707873, 697420
Crash Signature: [@ CERT_DestroyOCSPResponse ]
Whiteboard: [sg:critical] → [sg:high]
blocking1.9.2: ? → needed
Assigning CVE-2012-0441
Alias: CVE-2012-0441
Attachment #585661 - Flags: review?(rrelyea)
Comment on attachment 585661 [details] [diff] [review]
Possible fix

r+ rrelyea

We already have knowledge of at least two tags within the decoder, so validating that temp.len != 0 for a known set is not a problem.

bob
Attachment #585661 - Flags: review?(rrelyea) → review+
Whiteboard: [sg:high] → [sg:high] needcheckin
Target Milestone: --- → 3.13.4
Keywords: checkin-needed
Whiteboard: [sg:high] needcheckin → [sg:high]
Kaspar, thank you very much for your contribution.

Checking in quickder.c;
/cvsroot/mozilla/security/nss/lib/util/quickder.c,v  <--  quickder.c
new revision: 1.26; previous revision: 1.25
done
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reopening.

Unfortunately I had to back out the patch, because the NSS tinderbox went orange (test failures on multiple platforms).


merge.sh: Merging in SDR 
certutil --merge --source-dir ../SDR -d . -f ../tests.pw -@ ../tests.pw
certutil: Could not merge object  (type Secret Key): security library: received bad data.
merge.sh: #2581: Merging SDR  - FAILED


merge.sh: Decrypt - With Merged SDR Key
sdrtest -d . -i /home/tinderbox/mozilla/security/tinderlight/data/buildnss02_trunk_64_OPT/mozilla/tests_results/security/buildnss02.1/tests.v1.20788 -t Test1 -f ../tests.pw
merge.sh: #2583: Decrypt - Value 1  - FAILED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
The SEC_ASN1_INTEGER case is causing problem with our key database.
It seems that RSA keys in the NSS key database are stored without
the modulus.  Then they are encoded with this template:

86 const SEC_ASN1Template lg_nsslowkey_RSAPrivateKeyTemplate[] = {
87     { SEC_ASN1_SEQUENCE, 0, NULL, sizeof(NSSLOWKEYPrivateKey) },
88     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.version) },
89     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.modulus) },
90     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.publicExponent) },
91     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.privateExponent) },
92     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.prime1) },
93     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.prime2) },
94     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.exponent1) },
95     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.exponent2) },
96     { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.coefficient) },
97     { 0 }                                                                     
98 };

So u.rsa.modulus is encoded as a zero-length INTEGER.  When decoding
we need to be able to handle this.

Perhaps we need to add a bit flag to specify this relaxed mode of decoding
so that the decoder can be strict by default.
I recommend removing the following two changes from Kaspar's patch.

1. Remove the line

+                                        tagnum == SEC_ASN1_INTEGER ||

This causes the test failures in merge.sh as I noted in comment 9.
Bob is the best person to determine what to do about this.

2. Remove the "belt-and-suspenders" fix at the end of the patch:

@@ -863,7 +884,7 @@ static SECStatus DecodeItem(void* dest,
                If part of the destination was allocated by the decoder, in
                cases of POINTER, SET OF and SEQUENCE OF, then type is set to
                siBuffer due to the use of PORT_ArenaZAlloc*/
-            destItem->data = temp.data;
+            destItem->data = temp.len ? temp.data : NULL;
             destItem->len = temp.len;
         }
         else

I am worried that some code may use the a non-null 'data' in SECItem to
distinguish between two cases:

Case 1: data=NULL, len=0: the data is absent, for example, if the data
is marked as optional in ASN.1, it may be absent.  Another possibility
would be the ASN.1 RSAPrivateKey structure, in which some fields may
be absent, it seems.

Case 2: data=non-NULL, len=0: the data is present but has a zero length.
Assignee: nobody → wtc
Status: REOPENED → ASSIGNED
Priority: -- → P1
(In reply to Kaspar Brand from comment #0)
>
> ... Note that the "classic" DER decoder,
> i.e. SEC_ASN1DecodeItem, has checks for illegal zero-length items, and will
> e.g. reject an OCSP response where the response status isn't encoded
> properly.

Kaspar, did you determine this by experiments or by code inspection?

I couldn't find where the "classic" DER decoder checks for illegal zero-length
items by code inspection.  I tried the following:
- I searched for "BOOLEAN" and "ENUMERATED" in secasn1d.c but didn't find any
  code for these two types relevant to their lengths.
- I searched for "len != 0", "len > 0", and "len == 0" in secasn1d.c.  I found
  several occurrences of "len == 0", but it's not clear if any of them rejects
  illegal zero-length items.

Thank you for your help.
(In reply to Wan-Teh Chang from comment #11)
> (In reply to Kaspar Brand from comment #0)
> >
> > ... Note that the "classic" DER decoder,
> > i.e. SEC_ASN1DecodeItem, has checks for illegal zero-length items, and will
> > e.g. reject an OCSP response where the response status isn't encoded
> > properly.
> 
> Kaspar, did you determine this by experiments or by code inspection?

The latter. It's at the end of sec_asn1d_prepare_for_contents:

http://mxr.mozilla.org/mozilla/source/security/nss/lib/util/secasn1d.c#1401

Relaxing the checks in DecodeItem as suggested in comment 10 would be rather unfortunate, I think. The way I understand the API of SEC_QuickDERDecodeItem is that it should answer the question: based on the supplied template, is this buffer valid ASN.1 encoded data? If you permit zero-length INTEGERs, this is obviously no longer the case.
Not likely to get NSS 3.13.4 in Firefox 12, hopefully Firefox 13.
> The SEC_ASN1_INTEGER case is causing problem with our key database.
> It seems that RSA keys in the NSS key database are stored without
> the modulus.

That doesn't sound right. Are you sure it's really RSA keys? I think we store symmetric keys with an RSA template. In those cases modulus is set to '0'.

> The way I understand the API of SEC_QuickDERDecodeItem is that it should answer the question:
> based on the supplied template, is this buffer valid ASN.1 encoded data? 

It's always been more pragmatic than that. Historically we do accept improperly encoded ASN.1 if such encodings are common and needed for inter-operability. In this case the problem is in our own legacy database. We really don't want 2 sets of functions if we can help it. I don't know if we can fix the problem by clever use of the templates (perhaps creating a template that expects a zero encoded field). In any case, taking wtc's proposal would get something checked in much more quickly, otherwise we would have to do more work to test things out. We clearly aren't going to check in any code that can't read the user's databases NSS has written in the past, no matter how broken the ANS.1 is in those databases may be.

bob
(In reply to Daniel Veditz [:dveditz] from comment #4)
> I'm more worried about Kaspar's "2)" -- could someone get a malicious EE
> cert constructed in this way signed by a trusted CA?

I'm trying the assess the urgency of this bug as a security problem. I'm less worried about self-created certs like Kaspar's testcase, it might be an "sg:moderate" sort of bug since if you accept root certs from strangers you've probably got problems already. But if you could get a commercial CA to issue such a cert it would be a real worry. Do such CA's allow customers to specify the basic constraints and accept it without modification?
The crash is clearly an issue. Any crash we can generate by creating invalid fields in the certificate are also a problem.

The basic constraints thing, though, is an anomaly (we should fail safe), but it's clearly a case where someone shoots themselves in the foot. Anyone who can create a basic constraint with a NULL bool can create a basic constraint with the bool set to true. Security safety says we should probably just fail such a cert as badly encoded (the semantic Kasper is asking for), but I don't think this in particular is a security issue.

More interesting is do we fail properly if the Key or the modulus in the key is incorrectly encoded. If that's the case, we can go a long way toward fixing this issue by adopting wtc's suggestions.

bob
Attached patch Fix for lg_mkSecretKeyRep() (obsolete) — Splinter Review
Ok, so after another closer look, it turns out that the failures reported in comment 8 uncover a bug in lg_createSecretKeyObject() / lg_mkSecretKeyRep(), actually:

For the legacy DB, we have (see lg_mkSecretKeyRep):

    /* Secret keys are represented in the database as "fake" RSA keys.
     * The RSA key is marked as a secret key representation by setting the
     * public exponent field to 0, which is an invalid RSA exponent.
     * The other fields are set as follows:
     *   modulus - CKA_ID value for the secret key
     *   private exponent - CKA_VALUE (the key itself)
     *   coefficient - CKA_KEY_TYPE, which indicates what encryption algorithm
     *      is used for the key.
     *   all others - set to integer 0
     */

(Note that "all others - set to integer 0" does NOT mean that the length is zero - instead the item stores the "integer 0", so its len is 1.)

In the case of the merge.sh tests, the issue is that the (secret) key to be merged does not yet have a CKA_ID, and while lg_createSecretKeyObject actually creates an new id in such a case, lg_mkSecretKeyRep fails to properly pick it up. I.e., the merge.sh failures are due to certutil trying to create a secret key without a CKA_ID.

One way to solve that is to grab the CKA_ID from the (somewhat misnomed) "pubkey" argument in lg_mkSecretKeyRep(), as shown in the attached
patch. The other one would be to stuff "pubKey" back into "templ" in lg_createSecretKeyObject(), before calling lg_mkSecretKeyRep().

With this patch (on top of attachment 585661 [details] [diff] [review], of course), all.sh passes for me: "Passed: 5808", "Failed: 0".
Whiteboard: [sg:high] → [sg:moderate]
Kaspar: thank you very much for tracking this down.

I found that it is nearly impossible to stuff "pubKey" back into "templ"
because ultimately "templ" is an argument passed to sdb->sdb_CreateObject
(lg_CreateObject).  So your approach is the right solution.  However, it
is necessary to copy the data in "pubKey" because lg_createSecretKeyObject
frees pubKey.data before it returns.
Attachment #610218 - Attachment is obsolete: true
Attachment #611117 - Flags: superreview?(rrelyea)
Attachment #611117 - Flags: review?(mozbugzilla)
I edited Kaspar's "Possible fix" (attachment 585661 [details] [diff] [review]) and
checked it in on the NSS trunk (NSS 3.13.4).

Checking in quickder.c;
/cvsroot/mozilla/security/nss/lib/util/quickder.c,v  <--  quickder.c
new revision: 1.28; previous revision: 1.27
done

In that switch statement, under the case SEC_ASN1_BIT_STRING,
we know temp.len is nonzero because of the new check we added,
and temp.data is always non-NULL (which is why Kaspar added
his "belts and suspenders" fix), so the test
     if (temp.len && temp.data)
can be deleted.  I then removed the unnecessary (unsigned char *)
casts for temp.data and simplied the expressions.
Attachment #585661 - Attachment is obsolete: true
Attachment #611131 - Flags: review+
Comment on attachment 611117 [details] [diff] [review]
Fix for lg_mkSecretKeyRep(), v2 by Kaspar Brand

Patch checked in on the NSS trunk (NSS 3.13.4).

Checking in lgcreate.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgcreate.c,v  <--  lgcreate.c
new revision: 1.8; previous revision: 1.7
done
Comment on attachment 611117 [details] [diff] [review]
Fix for lg_mkSecretKeyRep(), v2 by Kaspar Brand

r+, thanks for correcting my first attempt. You're right that "templ" is already read-only at that point, so there's indeed no other option left (pre-3.12, the code used sftk_forceAttribute for this [http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11.c&rev=1.140&mark=1633#1624], but that's no longer available/feasible).

I assume that the merge code actually supplies no ID on purpose - the failures previously observed occur when it deals with the SDR key (which usually has the default "keyID" from pk11sdr.c). When merging, there are cases where a new ID is needed: see bug 423093 comment 7. Fixing lg_mkSecretKeyRep is therefore definitely the proper way of addressing this.

As far as compatibility with existing cert8 DBs is concerned, I think we're fine: symmetric keys were required to have a CKA_ID for a very long time - see bug 130699 (attachment 100657 [details] [diff] [review] introduced the "Secret keys must have a CKA_ID value" comment). Only cert8 DBs created with --merge would have such corrupt entries, and I doubt that such an "ID-less" SDR key will work properly.
Attachment #611117 - Flags: review?(mozbugzilla) → review+
Kaspar: thank you for reviewing the patch and analyzing the
compatibility with existing cert8/key4 DBs.  I am also worried
about compatibility with existing DBs.  I hope Bob can give
an authoritative answer to that question.

I looked at the code more and I believe your patch (which doesn't
copy pubKey into the arena of privKey) will also work because
lg_createSecretKeyObject destroys pubKey after destroying privKey.
privKey already has a bunch of members that point to data (derZero)
outside its arena.  But I think copying pubKey allows us to not
worry about object lifetime and the lg_Attribute2SecItem(arena, CKA_ID, ...)
call in the original code also copies pubKey.
Assignee: wtc → mozbugzilla
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Kaspar: In the previous comment, I meant existing key3 DBs, not
key4 DBs.  Sorry about the confusion.

It would be nice if you could check the second change I made to
your patch.  Thanks.

https://bugzilla.mozilla.org/attachment.cgi?oldid=585661&action=interdiff&newid=611131&headers=1

(The first change merely removes white space.)
(In reply to Wan-Teh Chang from comment #24)
> Kaspar: In the previous comment, I meant existing key3 DBs, not
> key4 DBs.  Sorry about the confusion.

Right, I was a bit sloppy when referring to cert8 DBs - we're obviously talking about "ID-less" entries in key3.db files, to be precise.

> It would be nice if you could check the second change I made to
> your patch.

Looks good to me. For the sake of consistent style, you might want to use temp.data++ (see 12 lines above).

There's a corner case (the empty BIT STRING, i.e. one with a single zero contents octet) where advancing the temp.data pointer is unnecessary, but we will now adjust for this by setting destItem->data to NULL in the end, in any case.
Comment on attachment 611117 [details] [diff] [review]
Fix for lg_mkSecretKeyRep(), v2 by Kaspar Brand

r+

yes, the variable naming is pretty bad in this function.

It does lead to a question, however. When was QuickDER failing. I don't see QuickDER in the current code path. This means that this code could have created fautly asn.1 during it's time in the field.

I believe the symmetric keys in the real world are all generated from the sdr code, which I also believe adds the CKA_ID. If this is the case, then we may not have any real world database with this null DER encoding, but I'm not totally sure this is the case. We really need to get some real world testing on this before we it drops into a mozilla update. We don't want even 1% of mozilla users to loose access to their SDR keys (and thus all their passwords).

Anyway, this doesn't affect this particular patch (which fixes the underlying issue in the test, and doesn't itself cause broken entries to fail to be read).

bob
Attachment #611117 - Flags: superreview?(rrelyea) → superreview+
[Triage Comment]
Not sure why this is marked as FIXED when it doesn't appear to have landed yet, please change status flags accordingly when landed and also land to ESR branch as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process once fixed on mainline.
(In reply to Robert Relyea from comment #26)
>
> It does lead to a question, however. When was QuickDER failing.

QuickDER only failed somewhere in the merge.sh test.  See Kai's
comment 8.

> This means that this code could have
> created fautly asn.1 during it's time in the field.

Kaspar thinks that only [key3] DBs created with --merge would have
such corrupt entries.  (See the last paragraph of his comment 22).
I think this means there should be no real world database with
this null DER encoding.  Do you agree?

If not, then we will need to remvoe the
    tagnum == SEC_ASN1_INTEGER ||
line from NSS 3.13.4 to reduce the risk.
(In reply to Lukas Blakk [:lsblakk] from comment #27)
> Not sure why this is marked as FIXED when it doesn't appear to have landed
> yet, please change status flags accordingly when landed and also land to ESR
> branch as per
> https://wiki.mozilla.org/Release_Management/ESR_Landing_Process once fixed
> on mainline.

Lukas, we consider NSS a separate product, and the fix has already landed in NSS.

Mozilla software consumes full releases of NSS, only, not individual patches.

For tracking this consumption and upgrade of mozilla-central to the newer NSS release, please see bug 741135
Independently of Lukas suggestion:

If you continue to make modifications to this bug, you might consider to reopen it until the final change has been added to cvs.
It seems that a bug against "product NSS" combined with "firefox version tracking flags" cause confusion.

I propose to transfer the firefox tracking flags currently in this bug to bug 741135, which is the one that will bring the fix into mozilla.

If that seems to complicated, we could also chose to keep this bug open until bug 741135 was marked as resolved on mozilla-central, for simplicity.
"tracking minus" for ESR, we'll get this when NSS is uplifted in bug 741135
> QuickDER only failed somewhere in the merge.sh test.  See Kai's comment 8.

I think you misunderstood me. I know it fails in the merge test. Where in the code in the merge test is the question. I believe it has to be when reading the key back out.

> Kaspar thinks that only [key3] DBs created with --merge would have
> such corrupt entries.  (See the last paragraph of his comment 22).

The code he points to is the same code that got moved to legacydb and turned out to be broken.

> I think this means there should be no real world database with
> this null DER encoding.  Do you agree?

No, not on that evidence. There is no code in softoken the enforce the CKA_ID to be set. In fact we know this to be the case because --merge itself uses PK11_MergeTokens which does everything from the PK11_Wrap layer. If the issue was --upgrade-merge, then we may have had some internal softoken stuff going on, but --upgrade-merge only works from old to new, while PK11_MergeTokens can be used with any token as either the source or target).

The question is, how are keys generated? I believe the vast majority of them are generated using the SDR functions. The SDR functions will accept a CKA_ID from the application, but apply it's own if no CKA_ID is given. I think that all real databases should be OK. But there is nothing to restrict someone from calling PK11_TokenKeyGen() with a NULL keyid, so it's possible that we may have an in-the-field database problem.

The upshot is: I think we are safe, but I'm not positive, and we should treat this update with the appropriate care as it rolls downstream...

bob
Bob:

I had the stack trace when I wrote comment 9, but
for some reason I didn't think it necessary to paste
the call stack in the comment :-(
(In reply to Robert Relyea from comment #33)
> Where in the code in the merge test is the question. I believe it has to be
> when reading the key back out.

PK11_MergeTokens does two passes of pk11_mergeByObjectIDs (first the private keys, and then the rest). In the second pass, it fails to re-read the seckey entry it just created.

> The question is, how are keys generated? I believe the vast majority of them
> are generated using the SDR functions. The SDR functions will accept a
> CKA_ID from the application, but apply it's own if no CKA_ID is given. I
> think that all real databases should be OK.

I can confirm this observation, from looking at both existing in-the-field key3.db files and from creating new ones (with current Firefox releases, e.g.). They always have the SDR key with the 0xf8000000000000000000000000000001 CKA_ID.

> we should treat this update with the appropriate care as it rolls downstream...

Will you make an announcement/advisory of some sort for NSS, specifically? Listing it (only) under http://www.mozilla.org/security/announce/ might not catch the attention of all downstream NSS users.

(In reply to Wan-Teh Chang from comment #28)
> If not, then we will need to remvoe the
>     tagnum == SEC_ASN1_INTEGER ||
> line from NSS 3.13.4 to reduce the risk.

Before doing that, let me add that I have proof of concept code for "re-parsing" broken key3db entries with a modified lg_nsslowkey_RSAPrivateKeyTemplate (in seckey_decrypt_private_key), and it seems to do the trick. It would need further polishing, but it's definitely preferrable over dropping the SEC_ASN1_INTEGER check in QuickDER, IMO.
As Kaspar suggested in comment 25 for consistent style.

Patch checked in on the NSS trunk and NSS_3_13_4_BRANCH
for NSS 3.13.4.

Checking in quickder.c;
/cvsroot/mozilla/security/nss/lib/util/quickder.c,v  <--  quickder.c
new revision: 1.29; previous revision: 1.28
done

Checking in quickder.c;
/cvsroot/mozilla/security/nss/lib/util/quickder.c,v  <--  quickder.c
new revision: 1.28.2.1; previous revision: 1.28
done
Kaspar: I believe this is the approach you mentioned at the end
of comment 35.  This also occurred to me as a good solution.

I found that the u.rsa.modulus field of a secret key doesn't seem
to be used, because NSS gets the CKA_ID from the DB key instead:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/legacydb/lgattr.c&rev=1.11&mark=701,741-743#700

To verify this point, this patch also changed lg_mkSecretKeyRep to
store 0 in u.rsa.modulus.
Attachment #612055 - Flags: superreview?(rrelyea)
Attachment #612055 - Flags: review?(mozbugzilla)
Comment on attachment 612055 [details] [diff] [review]
Use an alternative template that allows RSA modulus to be zero length

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

r-, primarily because of the changes to lgcreate.c. If we're concerned about maintaining backward compatibility, then we shouldn't switch to storing secret keys with empty CKA_IDs at this point. (I'm not opposed to it in general, but if we want to reduce risks - which seems the primary goal of this additional patch -, then we should consider this the subject of a separate bug.)

It's true that lg_FindSecretKeyAttribute ignores the CKA_ID and retrieves the DB key instead, but from a PKCS#11 point of view, applications might rely on the fact that NSS releases between 3.6 and 3.11.9 always store[d] secret key entries with a non-empty CKA_ID.

::: mozilla/security/nss/lib/softoken/legacydb/keydb.c
@@ +1798,5 @@
> +		 */
> +		rv = SEC_QuickDERDecodeItem(permarena, pk,
> +					lg_nsslowkey_RSAPrivateKeyTemplate2,
> +					&newPrivateKey);
> +		if (rv == SECSuccess && pk->u.rsa.modulus.len != 0) {

I suggest to make this check somewhat tighter, by looking also at the publicExponent (which is the defining property of a secret key in disguise of an RSA key, as per the comment in lg_mkSecretKeyRep). I.e., I would add

&& pk->u.rsa.publicExponent.len != 1
&& pk->u.rsa.publicExponent.data[0] != 0

to the if clause.

Second, in the lg_nsslowkey_RSAPrivateKeyTemplate2, you could omit the SEC_ASN1_INNER modifier and apply the following checks here instead ("2" being the ASN.1 tag number for an INTEGER):

pk->u.rsa.modulus.len != 2
&& pk->u.rsa.modulus.data[0] != 2
&& pk->u.rsa.modulus.data[1] != 0

@@ +1801,5 @@
> +					&newPrivateKey);
> +		if (rv == SECSuccess && pk->u.rsa.modulus.len != 0) {
> +		    PORT_SetError(SEC_ERROR_BAD_DER);
> +		    rv = SECFailure;
> +		}

Here we could perhaps "fix" pk->u.rsa.modulus (in an else clause), by setting it to "integer 0" - which is better than returning an NSSLOWKEYPrivateKey with a zero-length integer lying around.

::: mozilla/security/nss/lib/softoken/legacydb/lowkey.c
@@ +104,5 @@
> + */
> +const SEC_ASN1Template lg_nsslowkey_RSAPrivateKeyTemplate2[] = {
> +    { SEC_ASN1_SEQUENCE, 0, NULL, sizeof(NSSLOWKEYPrivateKey) },
> +    { SEC_ASN1_INTEGER, offsetof(NSSLOWKEYPrivateKey,u.rsa.version) },
> +    { SEC_ASN1_ANY | SEC_ASN1_INNER,

(see change suggested for seckey_decrypt_private_key above)
Attachment #612055 - Flags: review?(mozbugzilla) → review-
Kaspar: thank you very much for your review.  I made all
the changes you suggested.  (The exact conditional expressions
you suggested seem wrong.  I changed != to ==.)
Attachment #612055 - Attachment is obsolete: true
Attachment #612420 - Flags: superreview?(rrelyea)
Attachment #612420 - Flags: review?(mozbugzilla)
Attachment #612055 - Flags: superreview?(rrelyea)
Comment on attachment 612420 [details] [diff] [review]
Use an alternative template that allows RSA modulus to be zero length, v2

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

Looks good and doesn't seem to deserve an overall r-, but IINM, it's not yet tight enough: if the input includes a publicExponent which is not 0, then the modulus can basically include anything (SEC_ASN1_ANY), and we will let it through (and leave the modulus untouched):

        if (rv == SECSuccess &&
            pk->u.rsa.publicExponent.len == 1 &&
            pk->u.rsa.publicExponent.data[0] == 0) {
            if (pk->u.rsa.modulus.len == 2 &&
                pk->u.rsa.modulus.data[0] == SEC_ASN1_INTEGER &&
                pk->u.rsa.modulus.data[1] == 0) {
                /* Change the zero-length integer to integer 0. */
                pk->u.rsa.modulus.data = pk->u.rsa.publicExponent.data;
                pk->u.rsa.modulus.len = pk->u.rsa.publicExponent.len;
            } else {
                PORT_SetError(SEC_ERROR_BAD_DER);
                rv = SECFailure;
            }
        }

(The first if clause will evaluate to false in this case, so there's no way to reach the else block.)

What I had in mind is allowing the following lines - how about that?

	/* A publicExponent of 0 is the defining property of a secret
	 * key disguised as an RSA key. When decoding with the
         * alternative template, only accept a secret key with an
         * improperly encoded modulus and a publicExponent of 0.
	 */
	if (rv == SECSuccess &&
	    pk->u.rsa.modulus.len == 2 &&
	    pk->u.rsa.modulus.data[0] == SEC_ASN1_INTEGER &&
	    pk->u.rsa.modulus.data[1] == 0 &&
	    pk->u.rsa.publicExponent.len == 1 &&
	    pk->u.rsa.publicExponent.data[0] == 0) {
		/* Change the zero-length integer to integer 0. */
		pk->u.rsa.modulus.data = pk->u.rsa.publicExponent.data;
		pk->u.rsa.modulus.len = pk->u.rsa.publicExponent.len;
	} else {
	    PORT_SetError(SEC_ERROR_BAD_DER);
	    rv = SECFailure;
	}
Kaspar: you are right on!  Thanks a lot for catching this error.
(I also realized my conditional expression was wrong in the middle
of the night, but had not had time to update the patch.)

This is the solution I came up with, which is the same as your fix
except that I preserve the error code if the SEC_QuickDERDecodeItem
call with the alternative template returns SECFailure.
Attachment #612420 - Attachment is obsolete: true
Attachment #612619 - Flags: superreview?(rrelyea)
Attachment #612619 - Flags: review?(mozbugzilla)
Attachment #612420 - Flags: superreview?(rrelyea)
Attachment #612420 - Flags: review?(mozbugzilla)
Comment on attachment 612619 [details] [diff] [review]
Use an alternative template that allows RSA modulus to be zero length, v3

In the interest of time (it's late at night in Kaspar's time zone),
I checked in this patch based on Kaspar's comment 40 because I've
made essentially the change he suggested.

Patch checked in on the NSS trunk and the NSS_3_13_4_BRANCH for
NSS 3.13.4.

Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/keydb.c,v  <--  keydb.c
new revision: 1.13; previous revision: 1.12
done
Checking in lowkey.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lowkey.c,v  <--  lowkey.c
new revision: 1.6; previous revision: 1.5
done
Checking in lowkeyti.h;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lowkeyti.h,v  <--  lowkeyti.h
new revision: 1.5; previous revision: 1.4
done

Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/keydb.c,v  <--  keydb.c
new revision: 1.12.2.1; previous revision: 1.12
done
Checking in lowkey.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lowkey.c,v  <--  lowkey.c
new revision: 1.5.2.1; previous revision: 1.5
done
Checking in lowkeyti.h;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lowkeyti.h,v  <--  lowkeyti.h
new revision: 1.4.2.1; previous revision: 1.4
done
Comment on attachment 612619 [details] [diff] [review]
Use an alternative template that allows RSA modulus to be zero length, v3

r+ rrelyea
Attachment #612619 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 612619 [details] [diff] [review]
Use an alternative template that allows RSA modulus to be zero length, v3

Happy to r+ this post-commit.
Attachment #612619 - Flags: review?(mozbugzilla) → review+
Are we taking NSS 3.13.4 in ESR? I doubt it somehow but this is tracking for ESR 13+ currently.
(In reply to Al Billings [:abillings] from comment #45)
> Are we taking NSS 3.13.4 in ESR? I doubt it somehow but this is tracking for
> ESR 13+ currently.

ESR 10.0.4 has already shipped NSS 3.13.4
(In reply to Kai Engert (:kaie) from comment #46)
> ESR 10.0.4 has already shipped NSS 3.13.4

Marking this as fixed, in that case.
I don't believe it's in 10.0.4 because it's not in Firefox 12, but it /is/ in ESR 10.0.5 and Firefox 13 already.
(In reply to Alex Keybl [:akeybl] from comment #47)
> (In reply to Kai Engert (:kaie) from comment #46)
> > ESR 10.0.4 has already shipped NSS 3.13.4
> 
> Marking this as fixed, in that case.

Sorry, I was wrong.

According to bug 741135 the ESR branch was updated to NSS 3.13.4 on April 26, this was later than the ESR 10.0.4 release.

Dan is right, it will be in ESR 10.0.5
Whiteboard: [sg:moderate] → [sg:moderate][advisory-tracking+]
Can someone please advise me how QA can verify this is fixed?
Whiteboard: [sg:moderate][advisory-tracking+] → [sg:moderate][advisory-tracking+][qa?]
You can verify that NSS 3.13.4 or later is being used.
(In reply to Wan-Teh Chang from comment #51)
> You can verify that NSS 3.13.4 or later is being used.

Looking at the source trees (security/nss/TAG-INFO):
mozilla-esr10: NSS_3_13_5_RTM
mozilla-beta: NSS_3_13_5_RTM
mozilla-release: NSS_3_13_4_RTM

Calling this verified based on the above. Please reset flags if there's something more needed for verification.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:moderate][advisory-tracking+][qa?] → [sg:moderate][advisory-tracking+]
Group: core-security
I haven't found the test added to the regression suite to address this bug. Was any such test added? I ask because I have received questions regarding the best way to verify the fix.
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.