This is a highly security sensitive issue. It should be "embargoed" (kept secret) until the researcher who reported it to us publicizes it. This problem evidently exists separately in both NSS and PSM. The fix for NSS bug 480509 did not fix the problem in PSM. It is a problem in all FF 3.0.x and probably in all FF 2.0.x versions (IINM). An Object Identifier (OID) is a series of unsigned integers, typically displayed separated by dots or spaces, e.g. 2.5.4.3 or 2 5 4 3 . In BER/DER encoded form, The first two of those integers are specially encoded in a single octet. The rest of the integers are encoded as a series of variable length unsigned integers, 7 bits per octet, with the high order bit signifying "this is not the last octet of this integer". So, all octets but the last have the high bit set. The last octet does not. For example, the OID 2.5.4.3 is correctly encoded as (hex) 55 04 03 the OID 2.5.4.131 is encoded as (hex) 55 04 81 03 the OID 2.5.4.16387 is encoded as (hex) 55 04 81 80 03 The BER and DER encoding rules require that each integer be given a minimal encoding. A leading octet of (hex) 80 is not allowed for any integer component. So, the encoding 55 04 80 03 is an invalid encoding of the OID 2.5.4.3 The trouble is that some software that attempts to display OIDs will display the above incorrectly encoded OID as 2.5.4.3 and will not display any indication that it has been incorrectly encoded. PSM does that now. This allows attackers to craft certs with subject names with incorrectly encoded OIDs that some CAs will not validate (because they're not valid OIDs) but some browsers will treat (or at least display) as valid OIDs. This is a way for an attacker to get unvalidated host names into cert names. Another trick is to encode an integer that exceeds 32 or 64 bits and is truncated. So for example, the encoding 55 04 82 80 80 80 80 80 80 80 80 80 03 contains a final integer whose value exceeds 64 bits. Some software will truncate it, and display that value as 2.5.4.3 I will attach to this bug a zip file with some certs (in PEM form) with such names in them, for testing. If the problem only affects the UI display, then perhaps its not quite as bad as if the unsupported OIDs get used for some automated security decision. NSS does not recognize an incorrectly encoded OID for security decisions, but it did incorrectly convert them to strings for display purposes. I changed NSS's function for converting DER OIDs to strings to display any invalid or too long component as UNSUPPORTED, e.g. 2.5.4 UNSUPPORTED. But that is not the only way to address this problem.

Examples: The cert in the attached file named attack2b.cer contains an OID encoded as 55 04 80 03 PSM displays this as Object Identifier (2 5 4 3) = www.bank.com The cert in the attached file named pk10oflo.cer contains an OID encoded as 55 04 82 80 80 80 80 80 80 80 80 80 03 PSM displays it as Object Identifier (2 5 4 3) = www.bank.com NSS displays both of these as OID.2.5.4.UNSUPPORTED=www.bank.com

The good news regarding this bug is that at no time does PSM go the next step of translating 2 5 4 3 into CN. So, this bug is perhaps less severe than bug 483440, but this bug is likely to get some press anyway.

To generate certs and/or CSRs with these bogus OID encodings, I used certutil and a debugger. I generated a cert/csr requesting an OID such as OID.2.5.4.2.8.8.8.8.8.8.8.8.8.3 and then hacked the DER in the debugger. It's inelegant, to say the least. Try creating a cert/csr with this attribute in the name: >OID.2.5.4.2.8.8.8.8.8.8.8.8.8.3=#131B7777772E62616E6B2E636F6D007777772E6261646775792E636F6D

This is after some audit of OID usage in PSM only place that has to be fixed. Do we have to add a bundled string for "UNSUPPORTED" label as well?

Comment on attachment 367839 [details] [diff] [review] WIP1 I may be mistaken, but it appears to me that this patch will disallow all occurrences of 0x80 in a component. 0x80 is not allowed for the first (most significant) octet of a component, but is allowed after that. The value 55 04 80 80 80 03 is invalid, but even though it contains some 80 octets, the value 55 04 c0 80 80 03 is potentially valid, corresponding to the OID 2.5.4.402653187 So, I think this patch is more restrictive that is optimal.

Status: I already have a fixed patch at the moment, just have to re-check it and create a test for it and submit for review.

Addressed your comments to WIP1.

One important note, this modifies strings and string freeze for 1.9.1 is on Thursday, March 19th at 23:59 PDT, if we want to get this there.

(In reply to comment #8) > One important note, this modifies strings and string freeze for 1.9.1 is on > Thursday, March 19th at 23:59 PDT, if we want to get this there. That was last month, right?

Yeah, 1.9.1 is string-frozen. If you need strings landed there, you'll need to make a pretty strong case.

Comment on attachment 371679 [details] [diff] [review] v1 I'm not enough in tune with Firefox's string issues to know how big a deal this string issue is. We're talking about one word that will appear in the place of a decimal number in a string of decimal numbers, e.g. instead of 1 2 13 98 653456 456 321 2 1 1 we might wee 1 2 13 98 UNSUPPORTED 456 321 2 1 1 Considering the vulnerability aspects, I think even an untranslated string of unrecognizable (untranslated) characters might be preferable to the display of an incorrect decimal value. But someone else who has better understanding of Mozilla's policies should make that judgment. I found one tiny issue that I think is probably a bug. It's so easy to fix that I would not give it r-, but rather r+ on the condition that it be fixed. >+ // we are storing to val of type unsigned long - 32 bits, only >+ // 7 * 4 = 28 bits can fit. Also to start with 0x80 means invalid >+ // formating. >+ if ((j == 0x80 && arbitraryLength == 1) || arbitraryLength == 4) { I believe that last test should be || arbitraryLength == 5 because 4 is OK, but 5 is not.

(In reply to comment #11) > (From update of attachment 371679 [details] [diff] [review]) > Considering the vulnerability aspects, I think even an untranslated > string of unrecognizable (untranslated) characters might be preferable > to the display of an incorrect decimal value. But someone else who > has better understanding of Mozilla's policies should make that judgment. > Agree, we have to have a string here. A word UNSUPPORTED as a default would be OK IMHO. I can create a 1.9.1 patch that hard-codes that word and leave a 1.9.2 patch with localization. > I found one tiny issue that I think is probably a bug. > It's so easy to fix that I would not give it r-, but > rather r+ on the condition that it be fixed. > > >+ // we are storing to val of type unsigned long - 32 bits, only > >+ // 7 * 4 = 28 bits can fit. Also to start with 0x80 means invalid > >+ // formating. > >+ if ((j == 0x80 && arbitraryLength == 1) || arbitraryLength == 4) { > > I believe that last test should be || arbitraryLength == 5 > because 4 is OK, but 5 is not. The patch is ok. Note that this whole block in under a condition if (j & 0x80). For instance, handle of 0xFF 0x80 0x80 0x80 0x7F: 0xFF: val = 0x0000007F, arbitraryLength = 1 0x80: val = 0x00003F80, arbitraryLength = 2 0x80: val = 0x001FC000, arbitraryLength = 3 0x80: val = 0x0FE00000, arbitraryLength = 4 0x01: val = 0xF0000000, arbitraryLength = 5 !! val should be 7F0000000 on the fifth step, we are loosing data here (doesn't fit 32 bits). As commented, we can store only 7 * 4 = 28 bits to 32 bit space. 7 * 5 = 35 > 32.

This is the same patch as "v1" but doesn't introduce localization.

Let's invite l10n to the party, then.

Ok, we should also decide if the string UNSUPPORTED here is the best choice. As the string has to have meaning of warning that there is something wrong and user might be a subject to a security attack we might better say something like INVALID, CORRUPTED, BROKEN. Opinions?

In reply to comment 12, Consider the string 0x81, 0xff, 0xff, 0xff, 0x7f which corresponds to the 32-bit value 0x1fffffff. It's valid, and does not overflow, but with the test if ((j == 0x80 && arbitraryLength == 1) || arbitraryLength == 4) { it will be reported as invalid.

That's true. So we have to check we fit by bits and not but number of 7-bit blocks added to val, and in case of overflow report an error. I will create a new patch to address it.

The patch is now simpler, but I would like you to take a look if it's correct. I was testing manually with following values: 88 80 80 80 01 - fits exactly 32 bits, passes 90 80 80 80 01 - doesn't fit, the top most bit would be lost, fails and displays UNSUPPORTED 88 80 80 80 80 01 - also fails 80 - fails, displays UNSUPPORTED

Honza, If string localization remains an issue, maybe you could use "unknown" instead of "unsupported". There are several existing already-localized strings for unknown. See http://mxr.mozilla.org/mozilla/search?string=%3D%5B%5Ea-z%5D*unknown%5B%5Ea-z%5D*%24&regexp=on&find=%2Fpipnss.properties&tree=mozilla

Comment on attachment 372057 [details] [diff] [review] v2 Honza, Assuming an OID component separator of '.', given an oid with the true DER encoding of 1.2.3.4.5.6666666666666666666666666666666.7.8.9 your patch will display that encoding as: 1.2.3.4.5.UNSUPPORTED Another way to display it would be: 1.2.3.4.5.UNSUPPORTED.7.8.9 Personally, I would prefer the latter, but I think it's a matter of personal taste rather than correctness. One issue with continuing on after an error is that you could end up with a long string such as 1.2.3.UNSUPPORTED.UNSUPPORTED.UNSUPPORTED.UNSUPPORTED.UNSUPPORTED.7.8.9 so one might impose a rule such as "do not continue after a second display of UNSUPPORTED". I'll give this an r+, on the assumption that stopping after the first UNSUPPORTED is your intent. If it was/is not your intent, then perhaps you will submit another patch for review.

(In reply to comment #20) > I'll give this an r+, on the assumption that stopping after the first > UNSUPPORTED is your intent. If it was/is not your intent, then perhaps > you will submit another patch for review. It was my intention. I tough it's good to stop parsing at that point because (as you mention) we may end up with a very long string and actually numbers that are behind the corrupted particle might be wrong anyway. However, this is patch that allows to continue after we got a broken number. I believe it's a bit better then the previous one.

Comment on attachment 372218 [details] [diff] [review] v3 I do like this patch better than the previous one, but I see one other issue (which I should have seen in the previous patch too, but missed until now). I should go back and give r- to the previous one, too, I suppose. Consider a DER OID string consisting of 0x29 0xff 0xff 0xff 0xff After the first special byte, which represents the OID components 1.1, all the remaining bytes have the "more bytes to follow" bit set. I think this patch (and the one before it) will be silent about it and output nothing for those last bytes, rather than saying anything about invalid/unsupported. (Please correct me if I'm wrong about that). For the above input, I think this patch will out a string like 1.1 instead of 1.1.unsupported and that's exploitable. The crucial requirement of this bug is to be sure to say "unsupported" at least once whenever any component in invalid. (Hmm, now I must go back and revisit my NSS patch to make sure it handles this case, too. )

Yes, I had this bug in NSS, too. One more issue which Honza mentioned in the NSS bug. The most significant bit of the first byte in the OID string is a "more bytes follow in this component" bit, just as in all other bytes of the string. The PSM code should handle that correctly, too.

Improved patch including automated tests for this. PSM is now handling the leading byte as an arbitrary int and displays "Unknown" in all cases of bad formatting we are aware of (leading 0x80, unclosed trailing 0x8X sequence, 32 bit overhead).

Comment on attachment 372389 [details] [diff] [review] v4 [Checkin m-c comment 40] [Checkin 191 comment 41] r=nelson for the change to the file security/manager/ssl/src/nsNSSCertHelper.cpp

Comment on attachment 372389 [details] [diff] [review] v4 [Checkin m-c comment 40] [Checkin 191 comment 41] Kaie, please r the rest of the patch.

We really ought to have gotten this into Firefox 3.5 Bob (Relyea): should we find another reviewer here? You probably didn't see the request because you weren't CC'd. Unlike bug 480509 this is just broken UI and not a vulnerability, right?

(In reply to comment #27) > We really ought to have gotten this into Firefox 3.5 Agreed. > Unlike bug 480509 this is just broken UI and not a vulnerability, right? Well, that's a matter of opinion and of degree, I think. Dan K's paper called explicit attention to this. The reasoning was that if the browser says "this cert name is bad", but upon examination it appears to the user that the cert name is good, then the user may be misled into overriding the error. The case about which he was concerned was the case of an attribute type and value pair that is NOT a valid "CN=www.bank.com", because the OID is not truly the CN OID. A CA might allow this, and issue a cert with that pair without checking that the applicant controls that host, on the grounds that it's not a correct CN OID, and so it should not matter. But if the UI displays it as CN=host.name.tld or even as Object Identifier (2 5 4 3) = www.bank.com then a user might be misled. The thought is that if the display clearly signifies that something is wrong/invalid, then the user will not be as easily misled, e.g. Object Identifier (2 5 4 3 INVALID) = www.bank.com or Object Identifier (2 5 INVALID 3) = www.bank.com

We need to take this in 1.9.1.1, especially since we've fixed the other related issues in 3.5.0.

Bob: Any update on reviewing this?

Bob is on vacation until next week.

Rob, if you could expedite review here it'd be appreciated. We know this will get opened at BlackHat, and would love to have a fix ready to ship, but probably wouldn't block 1.9.1.2 on it as it's sg:low, so the sooner the better!

Comment on attachment 372389 [details] [diff] [review] v4 [Checkin m-c comment 40] [Checkin 191 comment 41] Johnath: Nelson tells me Bob is the wrong reviewer here. Can you take or reassign?

Can you CC me to bug 506379 please? I don't have the rights. Thanks.

Comment on attachment 372389 [details] [diff] [review] v4 [Checkin m-c comment 40] [Checkin 191 comment 41] Honza - looking at the tests only, they seem like they should do the intended thing (I assume you've run them and they pass? And that they fail without the NSS components of this patch?). Why are we doing these as a mochitest though? It seems like an xpcshell test would be fine here, and require less boilerplate? r=me for the tests only, assuming as stated above that your tests pass try server and that they would fail without the fix in this bug. I'm hedging only because I have not confirmed the details of the OIDs in question, just the structure of the test itself, which is pretty straightforward.

(In reply to comment #35) > (From update of attachment 372389 [details] [diff] [review]) > Honza - looking at the tests only, they seem like they should do the intended > thing (I assume you've run them and they pass? And that they fail without the > NSS components of this patch?). Do, locally, I didn't try the try server. > Why are we doing these as a mochitest though? > It seems like an xpcshell test would be fine here, and require less > boilerplate? > I could do that but I don't know how to add certificates to xpcshell database (if there is even one as there is no profile directory). > r=me for the tests only, assuming as stated above that your tests pass try > server and that they would fail without the fix in this bug. It fails w/o this fix, for this bug. > I'm hedging only > because I have not confirmed the details of the OIDs in question, just the > structure of the test itself, which is pretty straightforward. The oids are all covering the reported ways of attack and also possible mistakes in the parser code.

(In reply to comment #36) > I could do that but I don't know how to add certificates to xpcshell database > (if there is even one as there is no profile directory). Ah, that was my stupid - sorry. I forgot that we built the https harness for mochitest. Okay, so then for the tests only, r=me. We should get this baking as soon as we can - AIUI, we'll need versions of this patch for trunk, 1.9.1 AND 1.9.0. I'm not sure what date notation the embargo note in the whiteboard is meant to indicate, but it feels to me like throwing this at the try server (particularly for 3.5 and 3.0) would be a free sanity check while we wait for the embargo to lift. Is that something you can do today, Honza? I'm sure it will be fine, but when we firedrill those releases, we'll want to avoid introducing ANY instability that we could have caught ahead of time.

The embargo isn't for checking in, but rather for announcing the vuln. This should land ASAP.

Comment on attachment 372389 [details] [diff] [review] v4 [Checkin m-c comment 40] [Checkin 191 comment 41] Approved for 1.9.1.2. a=ss for release-drivers. Please land ASAP.

Comment on attachment 372389 [details] [diff] [review] v4 [Checkin m-c comment 40] [Checkin 191 comment 41] http://hg.mozilla.org/mozilla-central/rev/ffcc4c3efed0

Comment on attachment 372389 [details] [diff] [review] v4 [Checkin m-c comment 40] [Checkin 191 comment 41] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80f1d1f62834

Comment on attachment 391562 [details] [diff] [review] v4 for 1.9.0 (not includeing the tests) [Checkin 190 comment 43] Checking in security/manager/ssl/src/nsNSSCertHelper.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp,v <-- nsNSSCertHelper.cpp new revision: 1.38; previous revision: 1.37 Approved by Daniel Veditz through IRC.

mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp 1.34.6.2

What is the best/simplest way for QA to verify this bug for Firefox 3.5.2?

The first attachment to this bug is a zip file of "attack" certs. It should suffice to import them and then view them with cert viewer's detail view. Doing that test with old code should show different results than with new code. If you have trouble importing the "attack" certs, use certutil to import them.

Here's a screenshot of the cert manager after importing the certs on 3.5.2. Is this expected?

The screen shot does not show evidence of this problem or its solution. Please double-click on that entry with the \00 in it. You should get a window called the "certificate viewer" with two tabs, general and details. Get a shot of the general tab, and then select the details tab, scroll down in the middle pane to "Subject", click on that, and then get a screen shot of that window. Those screen shots will either confirm or refute the fixes for thib bug and bug 483437.

Screenshots of General tab and Details tab (with Subject selected) uploaded. Please review and give feedback. Thanks.

Thanks, now please repeat those two steps for the other 2 of those 3 certs. Thanks.

Here are the remaining cert screenshots

Comparison between 3.0.11 and 3.0.13, for 3.0.13 verification purposes.

Comment on attachment 391983 [details] 3.0.11(top) vs 3.0.13(bottom); Details tab, Subject field Thanks for these screen shots. These appear as expected. Now, If I may request one more pair of screen shots. Please show the Issuer (instead of the Subject) of any one of the certificates. They all have the same issuer.

Here's a screenshot of the issuer using the pk10oflo.cer in Firefox 3.5.2

Excellent. Thanks. Marking verified.

As per comment 57, verified1.9.1

Comparison screenshots for verification in 3.0.13 and 3.5.2.

Also adding verified1.9.0.13. Thanks, Nelson.

Verified still fixed in 1.9.0.14 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.14pre) Gecko/2009081305 GranParadiso/3.0.14pre (.NET CLR 3.5.30729) and attached certs.

Comment on attachment 394818 [details] [diff] [review] v4 for 1.8.1 [Checkin on 1.8.1 comment 64] Approved for 1.8.1.24, a=dveditz for release-drivers

Comment on attachment 394818 [details] [diff] [review] v4 for 1.8.1 [Checkin on 1.8.1 comment 64] /cvsroot/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp,v <-- nsNSSCertHelper.cpp new revision: 1.12.26.11; previous revision: 1.12.26.10

Using the bad certs already in the bug here, it isn't clear to me that this bug is fixed. In Firefox, post-fix, the bad data in the OID in the subject field in the cert details shows up as "unknown". In Thunderbird, the bad data is just removed. I'd like someone who fully understands the fix to compare the two Thunderbird attachments, pre and post-fix, and tell me what they think.

AL, please explain what each of the 4 window-captures is in each of your two images.

It is the list of certificates and then the details view focused on the subject line/field within each of them (just as it is with Juan's attachments). The certificates are the lines attached in comment 0.