Last Comment Bug 483440 - PSM doesn't detect invalid OID encodings in Cert Viewer Details tab
: PSM doesn't detect invalid OID encodings in Cert Viewer Details tab
Status: VERIFIED FIXED
[sg:nse] embargo until blackhat 8/09
: fixed1.8.1.24, verified1.9.0.13, verified1.9.0.14, verified1.9.1
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: 1.9.0 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: 480509
Blocks: BH-2009
  Show dependency treegraph
 
Reported: 2009-03-14 13:49 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2010-02-18 22:18 PST (History)
13 users (show)
mbeltzner: blocking1.9.1.1-
dveditz: wanted1.9.1.x+
samuel.sidler+old: blocking1.9.0.13+
samuel.sidler+old: blocking1.9.0.14+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2+
.2-fixed


Attachments
zip file of attack certs (2.51 KB, application/x-zip-compressed)
2009-03-14 13:49 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
WIP1 (1.73 KB, patch)
2009-03-17 13:13 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1 (9.64 KB, patch)
2009-04-08 08:37 PDT, Honza Bambas (:mayhemer)
nelson: review+
Details | Diff | Review
v1 for 1.9.1 (1.97 KB, patch)
2009-04-09 09:59 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v2 (9.56 KB, patch)
2009-04-10 07:44 PDT, Honza Bambas (:mayhemer)
nelson: review+
Details | Diff | Review
v3 (8.85 KB, patch)
2009-04-11 08:20 PDT, Honza Bambas (:mayhemer)
nelson: review-
Details | Diff | Review
v4 [Checkin m-c comment 40] [Checkin 191 comment 41] (16.18 KB, patch)
2009-04-13 07:30 PDT, Honza Bambas (:mayhemer)
nelson: review+
bugzilla: review+
samuel.sidler+old: approval1.9.1.2+
Details | Diff | Review
v4 for 1.9.0 (not includeing the tests) [Checkin 190 comment 43] (9.67 KB, patch)
2009-07-30 03:46 PDT, Honza Bambas (:mayhemer)
samuel.sidler+old: approval1.9.0.14+
Details | Diff | Review
Screenshot: 3.5.2 (44.48 KB, image/png)
2009-07-31 11:52 PDT, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
no flags Details
Screenshot: General Tab (63.64 KB, image/png)
2009-07-31 12:40 PDT, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
no flags Details
Screenshot: Details tab (51.29 KB, image/png)
2009-07-31 12:40 PDT, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
no flags Details
Screenshots of Certs (227.58 KB, application/zip)
2009-07-31 13:03 PDT, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
no flags Details
3.0.11(top) vs 3.0.13(bottom); Details tab, Subject field (87.40 KB, image/png)
2009-07-31 15:53 PDT, juan becerra [:juanb]
no flags Details
Screenshot: 3.5.2 Issuer (46.52 KB, image/png)
2009-07-31 17:49 PDT, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
no flags Details
3.0.11 vs 3.0.13 (top); 3.5 vs 3.5.2 (bottom) (53.56 KB, image/png)
2009-07-31 18:03 PDT, juan becerra [:juanb]
no flags Details
v4 for 1.8.1 [Checkin on 1.8.1 comment 64] (9.96 KB, patch)
2009-08-17 07:58 PDT, Honza Bambas (:mayhemer)
dveditz: approval1.8.1.next+
Details | Diff | Review
Cert appears in TB 2.0.0.23, showing bug. (97.91 KB, image/png)
2010-02-18 17:10 PST, [On PTO until 6/29]
no flags Details
View of same certs in Thunderbird 2.0.0.24pre (post-fix) (97.20 KB, image/png)
2010-02-18 17:13 PST, [On PTO until 6/29]
no flags Details

Description Nelson Bolyard (seldom reads bugmail) 2009-03-14 13:49:27 PDT
Created attachment 367423 [details]
zip file of attack certs

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.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2009-03-14 14:12:03 PDT
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
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-03-14 14:25:40 PDT
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.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2009-03-17 00:53:42 PDT
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
Comment 4 Honza Bambas (:mayhemer) 2009-03-17 13:13:43 PDT
Created attachment 367839 [details] [diff] [review]
WIP1

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 5 Nelson Bolyard (seldom reads bugmail) 2009-03-17 21:30:46 PDT
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.
Comment 6 Honza Bambas (:mayhemer) 2009-03-26 13:49:23 PDT
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.
Comment 7 Honza Bambas (:mayhemer) 2009-04-08 08:37:06 PDT
Created attachment 371679 [details] [diff] [review]
v1

Addressed your comments to WIP1.
Comment 8 Honza Bambas (:mayhemer) 2009-04-08 08:43:29 PDT
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.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2009-04-08 14:11:52 PDT
(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?
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-04-08 19:06:47 PDT
Yeah, 1.9.1 is string-frozen. If you need strings landed there, you'll need to make a pretty strong case.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2009-04-08 20:18:56 PDT
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.
Comment 12 Honza Bambas (:mayhemer) 2009-04-09 08:55:10 PDT
(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.
Comment 13 Honza Bambas (:mayhemer) 2009-04-09 09:59:51 PDT
Created attachment 371882 [details] [diff] [review]
v1 for 1.9.1

This is the same patch as "v1" but doesn't introduce localization.
Comment 14 Johnathan Nightingale [:johnath] 2009-04-09 10:03:17 PDT
Let's invite l10n to the party, then.
Comment 15 Honza Bambas (:mayhemer) 2009-04-09 10:10:03 PDT
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?
Comment 16 Nelson Bolyard (seldom reads bugmail) 2009-04-09 10:59:59 PDT
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.
Comment 17 Honza Bambas (:mayhemer) 2009-04-09 11:04:04 PDT
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.
Comment 18 Honza Bambas (:mayhemer) 2009-04-10 07:44:49 PDT
Created attachment 372057 [details] [diff] [review]
v2

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
Comment 19 Nelson Bolyard (seldom reads bugmail) 2009-04-10 15:17:14 PDT
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 20 Nelson Bolyard (seldom reads bugmail) 2009-04-10 16:12:10 PDT
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.
Comment 21 Honza Bambas (:mayhemer) 2009-04-11 08:20:08 PDT
Created attachment 372218 [details] [diff] [review]
v3

(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 22 Nelson Bolyard (seldom reads bugmail) 2009-04-11 12:47:44 PDT
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. )
Comment 23 Nelson Bolyard (seldom reads bugmail) 2009-04-11 14:44:29 PDT
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.
Comment 24 Honza Bambas (:mayhemer) 2009-04-13 07:30:50 PDT
Created attachment 372389 [details] [diff] [review]
v4 [Checkin m-c comment 40] [Checkin 191 comment 41]

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 25 Nelson Bolyard (seldom reads bugmail) 2009-04-13 10:48:31 PDT
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 26 Honza Bambas (:mayhemer) 2009-04-13 10:50:12 PDT
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.
Comment 27 Daniel Veditz [:dveditz] 2009-06-26 18:02:32 PDT
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?
Comment 28 Nelson Bolyard (seldom reads bugmail) 2009-06-26 20:52:38 PDT
(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
Comment 29 Samuel Sidler (old account; do not CC) 2009-07-06 14:48:05 PDT
We need to take this in 1.9.1.1, especially since we've fixed the other related issues in 3.5.0.
Comment 30 Samuel Sidler (old account; do not CC) 2009-07-13 14:59:53 PDT
Bob: Any update on reviewing this?
Comment 31 Nelson Bolyard (seldom reads bugmail) 2009-07-13 15:13:53 PDT
Bob is on vacation until next week.
Comment 32 Mike Beltzner [:beltzner, not reading bugmail] 2009-07-21 17:19:26 PDT
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 33 Mike Beltzner [:beltzner, not reading bugmail] 2009-07-21 23:38:14 PDT
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?
Comment 34 Honza Bambas (:mayhemer) 2009-07-28 04:16:07 PDT
Can you CC me to bug 506379 please? I don't have the rights. Thanks.
Comment 35 Johnathan Nightingale [:johnath] 2009-07-28 06:54:42 PDT
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.
Comment 36 Honza Bambas (:mayhemer) 2009-07-28 08:35:03 PDT
(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.
Comment 37 Johnathan Nightingale [:johnath] 2009-07-28 11:49:08 PDT
(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.
Comment 38 Samuel Sidler (old account; do not CC) 2009-07-29 11:32:06 PDT
The embargo isn't for checking in, but rather for announcing the vuln. This should land ASAP.
Comment 39 Samuel Sidler (old account; do not CC) 2009-07-29 12:56:18 PDT
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 40 Honza Bambas (:mayhemer) 2009-07-29 13:29:44 PDT
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 41 Honza Bambas (:mayhemer) 2009-07-29 14:09:21 PDT
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 42 Honza Bambas (:mayhemer) 2009-07-30 03:46:32 PDT
Created attachment 391562 [details] [diff] [review]
v4 for 1.9.0 (not includeing the tests) [Checkin 190 comment 43]
Comment 43 Honza Bambas (:mayhemer) 2009-07-30 06:43:12 PDT
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.
Comment 44 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-07-30 14:11:45 PDT
mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp 	1.34.6.2
Comment 45 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 10:55:08 PDT
What is the best/simplest way for QA to verify this bug for Firefox 3.5.2?
Comment 46 Nelson Bolyard (seldom reads bugmail) 2009-07-31 11:23:42 PDT
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.
Comment 47 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 11:52:39 PDT
Created attachment 391920 [details]
Screenshot: 3.5.2

Here's a screenshot of the cert manager after importing the certs on 3.5.2.  Is this expected?
Comment 48 Nelson Bolyard (seldom reads bugmail) 2009-07-31 12:30:37 PDT
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.
Comment 49 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 12:40:28 PDT
Created attachment 391937 [details]
Screenshot: General Tab
Comment 50 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 12:40:57 PDT
Created attachment 391938 [details]
Screenshot: Details tab
Comment 51 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 12:41:36 PDT
Screenshots of General tab and Details tab (with Subject selected) uploaded.  Please review and give feedback.

Thanks.
Comment 52 Nelson Bolyard (seldom reads bugmail) 2009-07-31 12:43:24 PDT
Thanks, now please repeat those two steps for the other 2 of those 3 certs.
Thanks.
Comment 53 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 13:03:34 PDT
Created attachment 391943 [details]
Screenshots of Certs

Here are the remaining cert screenshots
Comment 54 juan becerra [:juanb] 2009-07-31 15:53:36 PDT
Created attachment 391983 [details]
3.0.11(top) vs 3.0.13(bottom); Details tab, Subject field

Comparison between 3.0.11 and 3.0.13, for 3.0.13 verification purposes.
Comment 55 Nelson Bolyard (seldom reads bugmail) 2009-07-31 17:37:01 PDT
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.
Comment 56 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 17:49:43 PDT
Created attachment 392017 [details]
Screenshot: 3.5.2 Issuer

Here's a screenshot of the issuer using the pk10oflo.cer in Firefox 3.5.2
Comment 57 Nelson Bolyard (seldom reads bugmail) 2009-07-31 17:53:50 PDT
Excellent.  Thanks.  Marking verified.
Comment 58 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 18:03:43 PDT
As per comment 57, verified1.9.1
Comment 59 juan becerra [:juanb] 2009-07-31 18:03:59 PDT
Created attachment 392019 [details]
3.0.11 vs 3.0.13 (top); 3.5 vs 3.5.2 (bottom)

Comparison screenshots for verification in 3.0.13 and 3.5.2.
Comment 60 juan becerra [:juanb] 2009-07-31 18:06:42 PDT
Also adding verified1.9.0.13.

Thanks, Nelson.
Comment 61 Honza Bambas (:mayhemer) 2009-08-17 07:58:15 PDT
Created attachment 394818 [details] [diff] [review]
v4 for 1.8.1 [Checkin on 1.8.1 comment 64]
Comment 62 [On PTO until 6/29] 2009-08-18 17:14:30 PDT
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 63 Daniel Veditz [:dveditz] 2009-12-21 14:41:30 PST
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 64 Honza Bambas (:mayhemer) 2010-01-15 06:27:42 PST
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
Comment 65 [On PTO until 6/29] 2010-02-18 17:10:57 PST
Created attachment 427678 [details]
Cert appears in TB 2.0.0.23, showing bug.
Comment 66 [On PTO until 6/29] 2010-02-18 17:13:20 PST
Created attachment 427679 [details]
View of same certs in Thunderbird 2.0.0.24pre (post-fix)

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.
Comment 67 Nelson Bolyard (seldom reads bugmail) 2010-02-18 21:49:34 PST
AL, please explain what each of the 4 window-captures is in each of your 
two images.
Comment 68 [On PTO until 6/29] 2010-02-18 22:18:21 PST
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.

Note You need to log in before you can comment on or make changes to this bug.