PSM doesn't properly escape AVA Values in Cert Viewer Details tab

VERIFIED FIXED

Status

()

Core
Security: PSM
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: mayhemer)

Tracking

(Depends on: 1 bug, 4 keywords)

1.9.0 Branch
verified1.8.1.24, verified1.9.0.13, verified1.9.0.14, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1.1 -
wanted1.9.1.x +
blocking1.9.0.13 +
blocking1.9.0.14 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +

Firefox Tracking Flags

(blocking1.9.1 .2+, status1.9.1 .2-fixed)

Details

(Whiteboard: [sg:low spoof][needs r=johnath] embargo until blackhat 8/09)

Attachments

(6 attachments, 5 obsolete attachments)

2.51 KB, application/x-zip-compressed
Details
50.41 KB, patch
bsmedberg
: review+
Samuel Sidler (old account; do not CC)
: approval1.9.1.2+
Details | Diff | Splinter Review
2.58 KB, patch
Samuel Sidler (old account; do not CC)
: approval1.9.0.14+
Details | Diff | Splinter Review
44.48 KB, image/png
Details
23.96 KB, image/png
Details
1.84 KB, patch
Details | Diff | Splinter Review
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.
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
(Assignee)

Comment 2

8 years ago
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.
(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.
(Assignee)

Comment 4

8 years ago
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.
Assignee: kaie → honzab.moz
Status: NEW → ASSIGNED
Depends on: 484111
(Assignee)

Comment 5

8 years ago
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.
Attachment #371682 - Flags: review?(nelson)
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? )
Attachment #371682 - Flags: review?(nelson) → review+
(Assignee)

Comment 7

8 years ago
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.
Attachment #371682 - Flags: review?(kaie)
(Assignee)

Updated

8 years ago
Attachment #371682 - Flags: review?(kaie) → review?(rrelyea)
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.
Flags: wanted1.9.1.x+
Flags: wanted1.9.0.x+
Flags: blocking1.9.1.1?
Flags: blocking1.9.0.13?
Whiteboard: [sg:low spoof] embargo until blackhat 8/09
IMO, this is much more serious as a vulnerability than is bug 483440.

Comment 10

8 years ago
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
Flags: blocking1.9.1.1?
Flags: blocking1.9.1.1+
Flags: blocking1.9.0.13?
Flags: blocking1.9.0.13+
Whiteboard: [sg:low spoof] embargo until blackhat 8/09 → [sg:low spoof][needs review] embargo until blackhat 8/09
Which XPCOM parts need review? The PGO stuff?

bsmedberg: Can you grab review here for this 1.9.1.1 blocker?
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);
Attachment #371682 - Flags: review-
blocking1.9.1: --- → .2+
Flags: blocking1.9.1.1+ → blocking1.9.1.1-
(Assignee)

Comment 13

8 years ago
Created attachment 389434 [details] [diff] [review]
v1.1

addressed Benjamin's comments.
Attachment #371682 - Attachment is obsolete: true
Attachment #389434 - Flags: review?(rrelyea)
Attachment #371682 - Flags: review?(rrelyea)
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 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.
Attachment #389434 - Flags: review?(rrelyea) → review?(johnath)
(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.
status1.9.1: --- → wanted
Component: Security: UI → Security: PSM
QA Contact: ui → psm
Blocks: 506379
Whiteboard: [sg:low spoof][needs review] embargo until blackhat 8/09 → [sg:low spoof][needs r=johnath] embargo until blackhat 8/09
(Assignee)

Comment 17

8 years ago
Can you CC me to bug 506379 please? I don't have the rights. Thanks.
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.
(Assignee)

Comment 19

8 years ago
(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.
(Assignee)

Comment 20

8 years ago
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.
Attachment #389434 - Attachment is obsolete: true
Attachment #391092 - Flags: review?(johnath)
Attachment #389434 - Flags: review?(johnath)
(Assignee)

Updated

8 years ago
Attachment #391092 - Attachment is obsolete: true
Attachment #391092 - Flags: review?(johnath)
(Assignee)

Comment 22

8 years ago
Comment on attachment 391092 [details] [diff] [review]
v1.2

Ok, 'nsAutoArrayPtr<char> escapedValue = new char[escapedValueCapacity];' doesn't properly build on linux.
(Assignee)

Comment 23

8 years ago
Created attachment 391325 [details] [diff] [review]
v1.3 [Checkin m-c comment 28] [Checkin 191 comment 29]

Builds on linux, tests passes.
Attachment #391325 - Flags: review?(johnath)
Johnath is just reviewing the tests here, right?
(Assignee)

Comment 25

8 years ago
(In reply to comment #24)
> Johnath is just reviewing the tests here, right?

Yes, that is the last part that needs definitive r+.
Attachment #391325 - Flags: approval1.9.1.2+
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.
Attachment #391325 - Flags: review+
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]);
(Assignee)

Comment 28

8 years ago
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
Attachment #391325 - Attachment description: v1.3 → v1.3 [Checkin m-c comment 28]
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 29

8 years ago
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
Attachment #391325 - Attachment description: v1.3 [Checkin m-c comment 28] → v1.3 [Checkin m-c comment 28] [Checkin 191 comment 29]
(Assignee)

Updated

8 years ago
status1.9.1: wanted → .2-fixed
Flags: blocking1.9.0.13+
(Assignee)

Comment 30

8 years ago
Created attachment 391560 [details] [diff] [review]
v1.3 for 1.9.0 (not including the tests)
Attachment #391560 - Flags: approval1.9.0.14?
(Assignee)

Updated

8 years ago
Attachment #391560 - Attachment filename: 483437-psm-escaping-1.8.1.patch → 483437-psm-escaping-1.9.0.patch
(Assignee)

Comment 31

8 years ago
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.
Attachment #391560 - Attachment is obsolete: true
Attachment #391560 - Flags: approval1.9.0.14?
(Assignee)

Comment 32

8 years ago
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
Attachment #391564 - Attachment description: v1.3 as landed on 1.9.0 [Checkin 190 comment 31] → v1.3 as landed on 1.9.0 [Checkin 190 comment 32]
(Assignee)

Comment 33

8 years ago
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)
Attachment #391564 - Flags: approval1.9.0.14?
(Assignee)

Comment 34

8 years ago
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
Attachment #391564 - Attachment description: v1.3 as landed on 1.9.0 [Checkin 190 comment 32] → v1.3 as landed on 1.9.0 [Checkin 190 comment 32] [Backed out comment 34]
Attachment #391564 - Attachment is obsolete: true
Attachment #391564 - Flags: approval1.9.0.14?
(Assignee)

Comment 35

8 years ago
Created attachment 391567 [details] [diff] [review]
v1.3 for 1.9.0, builds ok [Checkin 190 comment 36]
Attachment #391567 - Flags: approval1.9.0.14?
(Assignee)

Comment 36

8 years ago
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
Attachment #391567 - Attachment description: v1.3 for 1.9.0, builds ok → v1.3 for 1.9.0, builds ok [Checkin 190 comment 36]
(Assignee)

Updated

8 years ago
Keywords: fixed1.9.0.14
Attachment #391567 - Flags: approval1.9.0.14? → approval1.9.0.14+
mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp 	1.34.6.1
Keywords: fixed1.9.0.13
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.
When I try to import the certs I am prompted for a password...
You probably tried importing them into the "my" tab.
Try importing them in the "servers" tab.
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?
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.
Marking verified based on the screen shots attached to bug 483440.
Status: RESOLVED → VERIFIED
As per comment 44, verified1.9.1
Keywords: verified1.9.1
Also adding verified1.9.0.13 keyword per comment #44. Nelson, thanks for your help verifying this.
Keywords: fixed1.9.0.13 → verified1.9.0.13
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
(Assignee)

Comment 47

8 years ago
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.
(Assignee)

Comment 48

8 years ago
Created attachment 394819 [details] [diff] [review]
v1.3 for 1.8.1 [Checking on 1.8.1 comment 51]
Attachment #394819 - Flags: approval1.8.1.next?
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.
Keywords: fixed1.9.0.14 → verified1.9.0.14
Group: core-security
Flags: blocking1.8.1.next? → blocking1.8.1.next+
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
Attachment #394819 - Flags: approval1.8.1.next? → approval1.8.1.next+
Depends on: 538294
(Assignee)

Comment 51

7 years ago
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
Attachment #394819 - Attachment description: v1.3 for 1.8.1 → v1.3 for 1.8.1 [Checking on 1.8.1 comment 51]
Keywords: fixed1.8.1.24
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.
Keywords: fixed1.8.1.24 → verified1.8.1.24
(Assignee)

Updated

6 years ago
Attachment #391325 - Flags: review?(johnath)
You need to log in before you can comment on or make changes to this bug.