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

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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)]

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)]

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)]

(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)]

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)]

Attachment: 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)]

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)]

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]]

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)]

IMO, this is much more serious as a vulnerability than is bug 483440.

Comment 10

[Robert Relyea]

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)]

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]

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)]

Attachment: v1.1

addressed Benjamin's comments.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

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]]

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]]

(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)]

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

Comment 18

[Johnathan Nightingale [:johnath]]

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)]

(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)]

Attachment: 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)]

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)]

Attachment: 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)]

Johnath is just reviewing the tests here, right?

Comment 25

[Honza Bambas (:mayhemer)]

(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)]

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]

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)]

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)]

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)]

Attachment: v1.3 for 1.9.0 (not including the tests)

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: 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)]

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)]

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)]

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)]

Attachment: v1.3 for 1.9.0, builds ok [Checkin 190 comment 36]

Comment 36

[Honza Bambas (:mayhemer)]

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: [e-mail]]]

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

Comment 38

[u279076]

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

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

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

[u279076]

When I try to import the certs I am prompted for a password...

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

You probably tried importing them into the "my" tab.
Try importing them in the "servers" tab.

Comment 42

[u279076]

Attachment: 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]]

Attachment: 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)]

Marking verified based on the screen shots attached to bug 483440.

Comment 45

[u279076]

As per comment 44, verified1.9.1

Comment 46

[juan becerra [:juanb]]

Also adding verified1.9.0.13 keyword per comment #44. Nelson, thanks for your help verifying this.

Comment 47

[Honza Bambas (:mayhemer)]

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)]

Attachment: v1.3 for 1.8.1 [Checking on 1.8.1 comment 51]

Comment 49

[Al Billings [:abillings - ex-MoCo]]

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]]

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)]

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 - ex-MoCo]]

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.