Closed Bug 644006 Opened 9 years ago Closed 9 years ago

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


(Core :: Security: PSM, defect, critical)

Not set





(Reporter: zwol, Assigned: zwol)


(Keywords: crash)

Crash Data


(1 file, 3 obsolete files)

From privileged JS


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.
See bug 345094 regarding PL_Base64DecodeBuffer.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → zackw
Attachment #521096 - Flags: review?(bzbarsky)
Comment on attachment 521096 [details] [diff] [review]

Taking a look at this.
Attachment #521096 - Flags: review?(bzbarsky) → review?(honzab.moz)
Comment on attachment 521096 [details] [diff] [review]

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

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) {
>   }
>   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/ b/security/manager/ssl/tests/mochitest/bugs/
>--- a/security/manager/ssl/tests/mochitest/bugs/
>+++ b/security/manager/ssl/tests/mochitest/bugs/
>@@ -49,14 +49,15 @@ _TEST_FILES = \
>         test_bug480509.html \
>         test_bug483440.html \
>         test_bug484111.html \
>         $(NULL)
>         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[";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[";1"]

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.
Attachment #521096 - Flags: review?(honzab.moz) → review-
Attached patch revised patch (obsolete) — Splinter Review
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.)
Attachment #521096 - Attachment is obsolete: true
Attachment #521367 - Flags: review?(honzab.moz)
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.

>+  }

>+  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.
Attachment #521367 - Flags: review?(honzab.moz) → review+
Attached patch re-revised patch (obsolete) — Splinter Review
(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.


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.)
Attachment #521367 - Attachment is obsolete: true
Attachment #521816 - Flags: review?(honzab.moz)
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)

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.
Attachment #521816 - Flags: review?(honzab.moz) → review+
Severity: normal → critical
Keywords: crash
Summary: nsNSSCertificateDB::ConstructX509FromBase64 crashes on zero-length string → [@ nsNSSCertificateDB::ConstructX509FromBase64] crashes on zero-length string
Attached patch revised^3 patchSplinter Review
OK, here is a new revision - only change is to check certDER is non-null before calling PL_strfree.
Attachment #521816 - Attachment is obsolete: true
Attachment #522225 - Flags: review+
Closed: 9 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsNSSCertificateDB::ConstructX509FromBase64]
You need to log in before you can comment on or make changes to this bug.