644006 - [@ nsNSSCertificateDB::ConstructX509FromBase64] crashes on zero-length string

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Zack Weinberg (:zwol)]

From privileged JS

    Components.classes["@mozilla.org/security/x509certdb;1"]
      .getService(Components.interfaces.nsIX509CertDB)
      .constructX509FromBase64("");

will crash the browser (at least on this computer it does, anyway).  The problem code is:

      PRUint32 len = PL_strlen(base64);
      int adjust = 0;

      /* Compute length adjustment */
      if (base64[len-1] == '=') {

if len == 0, this accesses memory before the beginning of the array, which (at least potentially) is unmapped.  There should be an early return with NS_ERROR_ILLEGAL_VALUE in this case (same as if the subsequent call to PL_Base64Decode fails).

I do not believe this is exploitable from content, but it could be a gotcha for someone coding an extension that did stuff with certificates.

Comment 1

[Mats Palmgren (inactive)]

See bug 345094 regarding PL_Base64DecodeBuffer.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: patch

Since I'm thinking about this, I may as well fix it.  Mats: Your fix for bug 345094 isn't going to fix this crash, which happens before PL_Base64DecodeBuffer gets called.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 521096 [details] [diff] [review]
patch

Taking a look at this.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 521096 [details] [diff] [review]
patch

>diff --git a/security/manager/ssl/src/nsNSSCertificateDB.cpp b/security/manager/ssl/src/nsNSSCertificateDB.cpp
>+  if (len == 0) {
>+    return NS_ERROR_ILLEGAL_VALUE;
>+  }
>+

This is not enough, if you pass "=" then:

>   if (base64[len-1] == '=') {
>     adjust++;
>     if (base64[len-2] == '=') adjust++;

You will still crash on the previous line, becuase:
base64[0] = '='
base64[1] = 0

len = 1     --> you pass your new condition
len-1 = 0   --> base64[0] == '=' is true
len-2 = -1  --> base64[-1] ... crash


>   certDER = PL_Base64Decode(base64, len, NULL);
>   if (!certDER || !*certDER) {
>     rv = NS_ERROR_ILLEGAL_VALUE;
>   }
>   else {
>     ...
>   }

I think you can calculate the 'adjust' value here, in this block, after you get a meaningful result from PL_Base64Decode that does the check correctly for you, actually right before you need it.  Then there is no need for any len > 0 or what ever check at the top of the method.

>diff --git a/security/manager/ssl/tests/mochitest/bugs/Makefile.in b/security/manager/ssl/tests/mochitest/bugs/Makefile.in
>--- a/security/manager/ssl/tests/mochitest/bugs/Makefile.in
>+++ b/security/manager/ssl/tests/mochitest/bugs/Makefile.in
>@@ -49,14 +49,15 @@ _TEST_FILES = \
>         test_bug480509.html \
>         test_bug483440.html \
>         test_bug484111.html \
>         $(NULL)
> 
> _CHROME_FILES = \
>         test_bug413909.html \
>         test_bug480619.html \
>+	test_bug644006.html \

Please fix white spaces.

>         $(NULL)
> 
> libs:: $(_TEST_FILES)
> 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
> libs:: $(_CHROME_FILES)
> 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
>diff --git a/security/manager/ssl/tests/mochitest/bugs/test_bug644006.html b/security/manager/ssl/tests/mochitest/bugs/test_bug644006.html
>+  var threw = false;
>+  var details;
>+  try {
>+    var cert =
>+      Components.classes["@mozilla.org/security/x509certdb;1"]
>+                .getService(Components.interfaces.nsIX509CertDB)
>+                .constructX509FromBase64("");
>+    threw = false;
>+    details = cert.toString();
>+  } catch (e) {
>+    threw = true;
>+    details = e.toString();
>+  }
>+  ok(threw, "constructX509FromBase64(\"\") should throw - got " + details);

You should first, OUT OF your try/catch block, get the "x509certdb;1" service like this:

var x509certdb = Components.classes["@mozilla.org/security/x509certdb;1"]
                .getService(Components.interfaces.nsIX509CertDB)

because if creation of the service, or just some typo makes the block fail, then the test will silently pass w/o testing the code you wanted it to.

Then, in separate try/catch blocks, do test all cases:
null string, "", "=", "==", and valid base64 encoded certs with none, singe and double padding (the '=' character) to check there are no regressions.

You could also test the result value of the exception ok(e.result == Components.results.NS_ERROR_ILLEGAL_VALUE) ...or how this is made...


r- for left possibility of a crash.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: revised patch

I should have caught the "=" case, thanks for noticing that.  Here is a rather more aggressive revision of the function which should address all of your concerns, and a more comprehensive test case.  (Man, generating X.509 certificates is No Fun.)

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 521367 [details] [diff] [review]
revised patch

>diff --git a/security/manager/ssl/src/nsNSSCertificateDB.cpp b/security/manager/ssl/src/nsNSSCertificateDB.cpp
>+nsNSSCertificateDB::ConstructX509FromBase64(const char *base64,
>+  if (!certDER || !*certDER) {
>+    PL_strfree(certDER);

Please check certDER for non-null before call to PL_strfree.

>+    return NS_ERROR_ILLEGAL_VALUE;
>+  }


>+  nsresult rv = NS_OK;

This seems not to be used anymore.  Please remove it.


The rest looks pretty good!  Thanks for the test as well.

r=honzab with the two nits above.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: re-revised patch

(In reply to comment #6)
> >+  if (!certDER || !*certDER) {
> >+    PL_strfree(certDER);
> 
> Please check certDER for non-null before call to PL_strfree.

PL_strfree is a trivial wrapper around plain old 'free' and therefore can be relied on to be a no-op when its argument is null.  Unchanged.

> >+  nsresult rv = NS_OK;
> 
> This seems not to be used anymore.  Please remove it.

Done.

Also, try server testing revealed that CERT_NewTempCertificate would sometimes return SEC_ERROR_LIBRARY_FAILURE instead of SEC_ERROR_BAD_DATA for the non-cert base64 blob -- not sure why -- so I removed the attempt to map various SEC_ error codes to NS_ERROR_ILLEGAL_VALUE.  (I'm still mapping SEC_ERROR_NO_MEMORY to NS_ERROR_OUT_OF_MEMORY, though.)

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 521816 [details] [diff] [review]
re-revised patch

>diff --git a/security/manager/ssl/src/nsNSSCertificateDB.cpp b/security/manager/ssl/src/nsNSSCertificateDB.cpp
>+  if (!certDER || !*certDER) {
>+    PL_strfree(certDER);

I see on other places in the mozilla code we do the check before calling this function.  Please, add it as well.  If it were directly call to free() then OK, but this function may be changed.

>+  if (!cert)
>+    return (PORT_GetError() == SEC_ERROR_NO_MEMORY)
>+      ? NS_ERROR_OUT_OF_MEMORY : NS_ERROR_FAILURE;

Yes, this is better.  I was personally not entirely confident about your error result changes before.


r=honzab with the if (certDER) check before PL_strfree() call.

Comment 9

[Zack Weinberg (:zwol)]

Attachment: revised^3 patch

OK, here is a new revision - only change is to check certDER is non-null before calling PL_strfree.

Comment 10

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/97a8337bd5e6