Last Comment Bug 483437 - PSM doesn't properly escape AVA Values in Cert Viewer Details tab
: PSM doesn't properly escape AVA Values in Cert Viewer Details tab
Status: VERIFIED FIXED
[sg:low spoof][needs r=johnath] embar...
: verified1.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: 538294 480509 484111
Blocks: BH-2009
  Show dependency treegraph
 
Reported: 2009-03-14 13:21 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2011-11-30 15:41 PST (History)
14 users (show)
mbeltzner: blocking1.9.1.1-
dveditz: wanted1.9.1.x+
samuel.sidler+old: blocking1.9.0.13+
dveditz: 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:21 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
v1 (49.71 KB, patch)
2009-04-08 08:48 PDT, Honza Bambas (:mayhemer)
nelson: review+
benjamin: review-
Details | Diff | Review
v1.1 (49.67 KB, patch)
2009-07-20 02:44 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1.2 (50.39 KB, patch)
2009-07-28 08:43 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1.3 [Checkin m-c comment 28] [Checkin 191 comment 29] (50.41 KB, patch)
2009-07-29 06:48 PDT, Honza Bambas (:mayhemer)
benjamin: review+
samuel.sidler+old: approval1.9.1.2+
Details | Diff | Review
v1.3 for 1.9.0 (not including the tests) (1.61 KB, patch)
2009-07-30 03:45 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1.3 as landed on 1.9.0 [Checkin 190 comment 32] [Backed out comment 34] (1.82 KB, patch)
2009-07-30 04:01 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1.3 for 1.9.0, builds ok [Checkin 190 comment 36] (2.58 KB, patch)
2009-07-30 05:02 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
3.0.11 vs 3.0.13 (23.96 KB, image/png)
2009-07-31 15:36 PDT, juan becerra [:juanb]
no flags Details
v1.3 for 1.8.1 [Checking on 1.8.1 comment 51] (1.84 KB, patch)
2009-08-17 08:00 PDT, Honza Bambas (:mayhemer)
dveditz: approval1.8.1.next+
Details | Diff | Review

Description Nelson Bolyard (seldom reads bugmail) 2009-03-14 13:21:31 PDT
Created attachment 367422 [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.

In BER/DER encoded form, the string values in Attribute Value Assertions 
(AVAs) in a Subject name are always explicitly encoded by length.  They
are never NUL-terminated strings.  Consequently, it is possible for a 
malicious actor to create an attribute value string with an embedded NUL
character.  When translating such value strings into ordinary displayable
ASCII or UTF8 strings, it is necessary to "escape" all embedded control
characters (0x00-0x1f and 0x7f, at a minimum) by converting them to a 
sequence of 2 hex characters preceded by a '\',  e.g. an embedded NUL 
should become \00.  

Now imagine a cert with a CN attribute whose value is a string consisting
of a '*' followed by a NUL followed by www.myhost.org.  If it is properly
escaped, it will be displayed as 
   *\00www.myhost.com
but if it is not properly escaped, it will be displayed as 
   *
And that's what's happening in PSM right now.  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 truncated string gets used for some automated security 
decision.  That's why it was a high priority to fix this in NSS.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2009-03-14 14:06:03 PDT
For example, The attached cert in the file named attack2c contains an
issuer name that is properly displayed as:
  CN=myCA,CN=*\00www.myCA.org
but PSM displays it as
  CN = myCA
  CN = *

That same cert has a subject name that is properly displayed as
  CN=www.bank.com\00www.badguy.com,OU=Hacking Division,O=Badguy Inc
but PSM displays it as
  CN = www.bank.com
  OU = Hacking Division
  O = Badguy Inc
Comment 2 Honza Bambas (:mayhemer) 2009-03-16 16:19:42 PDT
ProcessRDN (nsNSSCertHelper.cpp) calls CERT_DecodeAVAValue (that calls SEC_QuickDERDecodeItem) and its result is SECItem with buffer filled correctly with whole subject/issuer name data and with correct length information including all bytes of the subject/issuer data. As SECItem is intended to indicate number of bytes carried by its length member and not by zero-termination, responsibility to escape potential \0 chars imho falls to ProcessRDN. However, ProcessRDN is used only for UI purposes (AVA tree in cert details) and from ProcessCrlDistPoints.

Escaping added to ProcessRDN will only help the UI then. I haven't been deeper discovering where else we need to fix PSM yet. I only found out that CERT_VerifyCertName is fixed by patch for bug 480509 as it and its sub-calls are using PORT_Strcasecmp to check host name against subject fields of the certificate.

Would it be possible to know how to generate the attack certs (I have only certutil) or get a private key for them? I would love to do some real testing with an ssl server authorizing using such an attack cert.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2009-03-17 00:40:23 PDT
(In reply to comment #2)
> ProcessRDN is used only for UI purposes (AVA tree in cert details) and 
> from ProcessCrlDistPoints. Escaping added to ProcessRDN will only help 
> the UI then. 

OK, so perhaps the Security Group's ranking of the threat of this 
vulnerability will give it one of the lower rankings.  But this is very 
likely to appear in a paper at some upcoming conference, so it's worth 
fixing, even if it "will only help the UI", IMO.

> Would it be possible to know how to generate the attack certs (I have only
> certutil) or get a private key for them? I would love to do some real 
> testing with an ssl server authorizing using such an attack cert.

Certainly.  With an NSS build contianing the patch for bug 480509, any 
command that creates a cert or cert request will properly interpret a 
string such as 
  c=*\00www.badguy.com
so generating certs with subject names containing embedded control characters 
is now easy.  Generating certs with incorrectly encoded OIDs (bug 483440) is
more difficult.  I will comment in that bug.
Comment 4 Honza Bambas (:mayhemer) 2009-03-26 13:48:01 PDT
Status: I have a patch for this, but I duplicate the code from bug 480509. The function that I duplicate has been exported in bug 484111. So, I depend on it to have a "nicer" patch using it.
Comment 5 Honza Bambas (:mayhemer) 2009-04-08 08:48:33 PDT
Created attachment 371682 [details] [diff] [review]
v1

I'm using published CERT_RFC1485_EscapeAndQuote to escape all strings.

This patch also includes two new tests for related issues: bug 480509 and bug 484111.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2009-04-08 20:03:58 PDT
Comment on attachment 371682 [details] [diff] [review]
v1

This Hg patch contains 
- base64 encoded copies of a cert8.db and key3.db
- a modification to one .cpp file
- modifications to numerous other file that appear to be part of some 
browser test framework with which I am not familiar.

The only part of this patch that I believe I can review is the 
modification to the .cpp file.  That looks good to me, so I'm giving 
r+ to this patch for the .cpp file change ONLY.  
Someone else will need to review the rest. 

(I don't know how better to handle it.  Give it r-?  Reassign the review 
request to someone?  who? )
Comment 7 Honza Bambas (:mayhemer) 2009-04-09 08:32:06 PDT
Comment on attachment 371682 [details] [diff] [review]
v1

The patch includes new mochitests that needs a custom ssl server certificates (the cert db changes).

I believe Kai is the right person.
Comment 8 Daniel Veditz [:dveditz] 2009-06-26 18:06:31 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.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2009-06-26 20:55:31 PDT
IMO, this is much more serious as a vulnerability than is bug 483440.
Comment 10 Robert Relyea 2009-06-29 11:06:21 PDT
Yes, nelson already revied the parts I would have reviewed an XPCOM person should review the rest (I actualy saw nelson's r+ and assumed that was sufficient).

bob
Comment 11 Samuel Sidler (old account; do not CC) 2009-07-13 15:02:26 PDT
Which XPCOM parts need review? The PGO stuff?

bsmedberg: Can you grab review here for this 1.9.1.1 blocker?
Comment 12 Benjamin Smedberg [:bsmedberg] 2009-07-14 08:23:36 PDT
Comment on attachment 371682 [details] [diff] [review]
v1

>diff --git a/security/manager/ssl/src/nsNSSCertHelper.cpp b/security/manager/ssl/src/nsNSSCertHelper.cpp

>+    nsCAutoString escapedValue;
>+
>+    // We know we can fit buffer of this length. CERT_RFC1485_EscapeAndQuote
>+    // will fail if we provide smaller buffer then the result can fit to.
>+    PRIntn escapedValueCapacity = decodeItem->len * 3 + 3;
>+    escapedValue.SetCapacity(escapedValueCapacity);
>+    SECStatus status = CERT_RFC1485_EscapeAndQuote(
>+          (char*)escapedValue.BeginReading(),
>+          escapedValueCapacity, 
>+          (char*)decodeItem->data, 
>+          decodeItem->len);

This should be BeginWriting, not BeginReading. But I don't think you really need an autostring at all:

nsAutoArrayPtr<char> escapedValue = new char[escapedValueCapacity];
CERT_RFC1485...
CopyUTF8ToUTF16(escapedValue, avavalue);
Comment 13 Honza Bambas (:mayhemer) 2009-07-20 02:44:58 PDT
Created attachment 389434 [details] [diff] [review]
v1.1

addressed Benjamin's comments.
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2009-07-21 17:18:45 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 15 Mike Beltzner [:beltzner, not reading bugmail] 2009-07-21 23:37:55 PDT
Comment on attachment 389434 [details] [diff] [review]
v1.1

Johnath: Nelson tells me Bob is the wrong reviewer here. Can you take or reassign? The second review here is mostly about the tests, AIUI.
Comment 16 Johnathan Nightingale [:johnath] 2009-07-22 06:38:34 PDT
(In reply to comment #13)
> Created an attachment (id=389434) [details]
> v1.1
> 
> addressed Benjamin's comments.

Honza - have you tested this patch?  I get compile errors with your change in response to benjamin's comment (you seem to be using nsAutoPtr where he was suggesting an nsAutoArrayPtr?) and the tests don't seem to actually build/install in my objdir (maybe some makefile whitespace mess?).

The tests themselves look straightforward enough, though s/catched/caught/ throughout (English is a stupid language).

I haven't inspected the certificates, but if these tests fail without the patch and pass with it, that's a good sign.
Comment 17 Honza Bambas (:mayhemer) 2009-07-28 04:16:06 PDT
Can you CC me to bug 506379 please? I don't have the rights. Thanks.
Comment 18 Johnathan Nightingale [:johnath] 2009-07-28 06:46:05 PDT
Honza - I've cc'd you on that bug, though it's just a meta bug for tracking. Can you answer my questions in comment 16, please? It might be that benjamin is a better reviewer for the remaining code than I am, but certainly I'd like to understand why it's not building for me if it is for you.
Comment 19 Honza Bambas (:mayhemer) 2009-07-28 07:44:02 PDT
(In reply to comment #18)
> Honza - I've cc'd you on that bug, though it's just a meta bug for tracking.

I didn't know, thanks.

(In reply to comment #16)
> (In reply to comment #13)
> > Created an attachment (id=389434) [details] [details]
> > v1.1
> > 
> > addressed Benjamin's comments.
> 
> Honza - have you tested this patch?  I get compile errors with your change in
> response to benjamin's comment (you seem to be using nsAutoPtr where he was
> suggesting an nsAutoArrayPtr?) 

It builds and applies clearly on windows on m-c from Jul 25th. I will submit v1.2 with nsAutoArrayPtr, thanks for the catch.

> and the tests don't seem to actually
> build/install in my objdir (maybe some makefile whitespace mess?).

Probably see bug 495387.

> The tests themselves look straightforward enough, though s/catched/caught/
> throughout (English is a stupid language).
> 

:) will fix it.

> but if these tests fail without the patch
> and pass with it, that's a good sign.

Actually, those two are tests for bug 480509 and bug 484111. I believed it would be good to add them. But maybe it should be part of another bug and not this one, but this one is related, anyway. The code I change is not tested by these two tests. I could add a test for it, tough. Bug 483440 contains tests for the subject dump, that touches the new code.
Comment 20 Honza Bambas (:mayhemer) 2009-07-28 08:43:46 PDT
Created attachment 391092 [details] [diff] [review]
v1.2

Fixed nsAutoArrayPtr, 'caught', added a test for the CN check to probe the new code this patch adds.
Comment 22 Honza Bambas (:mayhemer) 2009-07-28 15:54:07 PDT
Comment on attachment 391092 [details] [diff] [review]
v1.2

Ok, 'nsAutoArrayPtr<char> escapedValue = new char[escapedValueCapacity];' doesn't properly build on linux.
Comment 23 Honza Bambas (:mayhemer) 2009-07-29 06:48:24 PDT
Created attachment 391325 [details] [diff] [review]
v1.3 [Checkin m-c comment 28] [Checkin 191 comment 29]

Builds on linux, tests passes.
Comment 24 Samuel Sidler (old account; do not CC) 2009-07-29 11:36:21 PDT
Johnath is just reviewing the tests here, right?
Comment 25 Honza Bambas (:mayhemer) 2009-07-29 11:45:08 PDT
(In reply to comment #24)
> Johnath is just reviewing the tests here, right?

Yes, that is the last part that needs definitive r+.
Comment 26 Samuel Sidler (old account; do not CC) 2009-07-29 12:53:13 PDT
Comment on attachment 391325 [details] [diff] [review]
v1.3 [Checkin m-c comment 28] [Checkin 191 comment 29]

I'm going to approve this for 1.9.1.2 on the assumption that it passes tests and that Johnathan will r+ the tests shortly.

Approved for 1.9.1.2. a=ss for release-drivers. Please land ASAP.
Comment 27 Benjamin Smedberg [:bsmedberg] 2009-07-29 13:07:21 PDT
Comment on attachment 391325 [details] [diff] [review]
v1.3 [Checkin m-c comment 28] [Checkin 191 comment 29]

I'd really prefer nsAutoArrayPtr<char> escapedValue = new char[escapedValueCapacity];
or
nsAutoArrayPtr<char> escapedValue(new char[escapedValueCapacity]);
Comment 28 Honza Bambas (:mayhemer) 2009-07-29 13:30:16 PDT
Comment on attachment 391325 [details] [diff] [review]
v1.3 [Checkin m-c comment 28] [Checkin 191 comment 29]

http://hg.mozilla.org/mozilla-central/rev/2f90a5c89bdd
Comment 29 Honza Bambas (:mayhemer) 2009-07-29 14:10:05 PDT
Comment on attachment 391325 [details] [diff] [review]
v1.3 [Checkin m-c comment 28] [Checkin 191 comment 29]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/89d25217b0b2
Comment 30 Honza Bambas (:mayhemer) 2009-07-30 03:45:06 PDT
Created attachment 391560 [details] [diff] [review]
v1.3 for 1.9.0 (not including the tests)
Comment 31 Honza Bambas (:mayhemer) 2009-07-30 04:01:44 PDT
Created attachment 391564 [details] [diff] [review]
v1.3 as landed on 1.9.0 [Checkin 190 comment 32] [Backed out comment 34]

This is 1.9.0 blocking, no need to approval.
Comment 32 Honza Bambas (:mayhemer) 2009-07-30 04:05:25 PDT
Comment on attachment 391564 [details] [diff] [review]
v1.3 as landed on 1.9.0 [Checkin 190 comment 32] [Backed out comment 34]

Checking in security/manager/ssl/src/nsNSSCertHelper.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp,v  <--  nsNSSCertHelper.cpp
new revision: 1.35; previous revision: 1.34
Comment 33 Honza Bambas (:mayhemer) 2009-07-30 04:06:53 PDT
Comment on attachment 391564 [details] [diff] [review]
v1.3 as landed on 1.9.0 [Checkin 190 comment 32] [Backed out comment 34]

Ok, this MUST have an approval. If it won't get it I will backout (but I assume it will as this bug is blocking 1.9.0.13 and 14)
Comment 34 Honza Bambas (:mayhemer) 2009-07-30 04:14:43 PDT
Comment on attachment 391564 [details] [diff] [review]
v1.3 as landed on 1.9.0 [Checkin 190 comment 32] [Backed out comment 34]

Problem solved: this patch doesn't build on 1.9.0.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.0/1248951840.1248951938.15359.gz

Backed out:

Checking in security/manager/ssl/src/nsNSSCertHelper.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp,v  <--  nsNSSCertHelper.cpp
new revision: 1.36; previous revision: 1.35
Comment 35 Honza Bambas (:mayhemer) 2009-07-30 05:02:06 PDT
Created attachment 391567 [details] [diff] [review]
v1.3 for 1.9.0, builds ok [Checkin 190 comment 36]
Comment 36 Honza Bambas (:mayhemer) 2009-07-30 06:40:13 PDT
Comment on attachment 391567 [details] [diff] [review]
v1.3 for 1.9.0, builds ok [Checkin 190 comment 36]

Checking in security/manager/ssl/src/nsNSSCertHelper.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp,v  <--  nsNSSCertHelper.cpp
new revision: 1.37; previous revision: 1.36

Approved by Daniel Veditz through IRC
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-07-30 14:11:52 PDT
mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp 	1.34.6.1
Comment 38 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 10:54:26 PDT
What is the best/simplest way for QA to verify this bug for Firefox 3.5.2?
Comment 39 Nelson Bolyard (seldom reads bugmail) 2009-07-31 11:22:04 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 40 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 11:26:21 PDT
When I try to import the certs I am prompted for a password...
Comment 41 Nelson Bolyard (seldom reads bugmail) 2009-07-31 11:41:58 PDT
You probably tried importing them into the "my" tab.
Try importing them in the "servers" tab.
Comment 42 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 11:52:56 PDT
Created attachment 391922 [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 43 juan becerra [:juanb] 2009-07-31 15:36:12 PDT
Created attachment 391978 [details]
3.0.11 vs 3.0.13 

On the left you can see the list of certs imported in 3.0.11, on the right the list when imported in 3.0.13. 3.0.13 makes it a easier to see that there's a problem.
Comment 44 Nelson Bolyard (seldom reads bugmail) 2009-07-31 17:55:33 PDT
Marking verified based on the screen shots attached to bug 483440.
Comment 45 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 18:04:31 PDT
As per comment 44, verified1.9.1
Comment 46 juan becerra [:juanb] 2009-07-31 21:10:38 PDT
Also adding verified1.9.0.13 keyword per comment #44. Nelson, thanks for your help verifying this.
Comment 47 Honza Bambas (:mayhemer) 2009-08-05 10:50:01 PDT
Landing on 1.8.1 is bloked by this checkin: http://bonsai.mozilla.org/cvsquery.cgi?module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_BRANCH&date=explicit&mindate=1249424040&maxdate=1249431239, tree's broken.
Comment 48 Honza Bambas (:mayhemer) 2009-08-17 08:00:25 PDT
Created attachment 394819 [details] [diff] [review]
v1.3 for 1.8.1 [Checking on 1.8.1 comment 51]
Comment 49 Al Billings [:abillings] 2009-08-18 17:07:38 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) using attached certs.
Comment 50 Daniel Veditz [:dveditz] 2009-12-21 14:40:19 PST
Comment on attachment 394819 [details] [diff] [review]
v1.3 for 1.8.1 [Checking on 1.8.1 comment 51]

Approved for 1.8.1.24, a=dveditz for release-drivers
Comment 51 Honza Bambas (:mayhemer) 2010-01-15 06:20:08 PST
Comment on attachment 394819 [details] [diff] [review]
v1.3 for 1.8.1 [Checking on 1.8.1 comment 51]

/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp,v  <--  nsNSSCertHelper.cpp
new revision: 1.12.26.10; previous revision: 1.12.26.9
Comment 52 Al Billings [:abillings] 2010-02-19 16:11:21 PST
Verified for 1.8.1.24 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.24pre) Gecko/2010021903 Thunderbird/2.0.0.24pre ThunderBrowse/3.2.8.1.

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