345094 - Assertion failure: srclen > 0, at nssb64d.c:565 (aborts [@ PL_Base64DecodeBuffer] from [@ nsCrypto::GenerateCRMFRequest] or [@ nsCrypto::ImportUserCertificates])

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Mats Palmgren (inactive)]

Assertion failure: srclen > 0, at nssb64d.c:565
(aborts [@ PL_Base64DecodeBuffer] from [@ nsCrypto::GenerateCRMFRequest])

(bug 104103 could be related but I don't know)

STEPS TO REPRODUCE
1. load testcase
2. push the button -> crash (abort through PR_ASSERT)

PLATFORMS AND BUILDS TESTED
Bug occurs in SeaMonkey trunk debug build.

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 3

[Mats Palmgren (inactive)]

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/manager/ssl/src/nsCrypto.cpp&rev=1.68&root=/cvsroot&mark=1525#1515

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2, window.crypto.importUserCertificates

Here's another one:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/manager/ssl/src/nsCrypto.cpp&rev=1.68&root=/cvsroot&mark=1889#1855

Comment 5

[Kai Engert (:KaiE:)]

From the stack we can see, we call ATOB_ConvertAsciiToItem with parameter 2 (ascii) being the empty string (""). In my opinion NSS should allow this and fail gracefully.

I filed NSS bug 346583

Comment 6

[Johnny Stenback (:jst)]

This doesn't look like anything more than a bogus assertion, see my last comment in bug 346583 (and the comment about this not being a security bug applies for this one as well).

Removing crash keyword as this doesn't crash release builds, but debug builds die due to the bad assertion.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Removing this from the security group, per bug 346583 comment 3 and this bug's comment 6.

Comment 10

[Mats Palmgren (inactive)]

Attachment: another stack

1.9.2 DEBUG build on Linux64, running cross_fuzz w. seed 1092

file:///usr/local/moz/lcamtuf.coredump.cx/cross_fuzz/cross_fuzz_randomized_20110105_seed.html#1092

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix?

I've had this in my tree for a while now to avoid getting
crashes when running fuzzers.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 521086 [details] [diff] [review]
fix?

I'd do it the following way, see the comment on result values above the function:

-    PR_ASSERT(srclen > 0);
-    if (srclen == 0)
-	return dest;
+    if (srclen == 0) {
+      *output_destlen = 0;
+      return dest;
+    }

This function returns a bin data, not a null-terminated string!

Also note, that the file style is: leave tabs, tab size = 8 spaces

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix, v2

The documentation above the function says:

/*
 * Perform base64 decoding from an input buffer to an output buffer.
 * The output buffer can be provided (as "dest"); you can also pass in
 * a NULL and this function will allocate a buffer large enough for you,
 * and return it.  If you do provide the output buffer, you must also
 * provide the maximum length of that buffer (as "maxdestlen").
 * The actual decoded length of output will be returned to you in
 * "output_destlen".
 *
 * Return value is NULL on error, the output buffer (allocated or provided)
 * otherwise.
 */

> +      return dest;

This would make the function fail if the caller didn't provide
a destination buffer, and succeed if it did.

I think this function should succeed given an empty input buffer,
so we need to allocate something to return when 'dest' is NULL.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 521279 [details] [diff] [review]
fix, v2

Looks like PL_Base64EncodeBuffer has the same problem.  If you plan on fixing it, please rather open a different bug.

You are right.  On success (even the result is zero-length) according to the comment we must return a non-null pointer (a "provided buffer") to not break the API.  Not the best design IMHO.

>diff --git a/security/nss/lib/util/nssb64d.c b/security/nss/lib/util/nssb64d.c
>+	    dest = (unsigned char *) PR_Malloc(1);
>+	}
> 	return dest;

Rather use output_buffer like in the previous patch as dest is actually treated as read-only?

Can you please turn your test case to an automated chrome test?

r=honzab.

Comment 15

[Mats Palmgren (inactive)]

Attachment: fix, v3 + crashtest

We can just return the result from PR_Malloc.

Added a crashtest.

Comment 16

[Mats Palmgren (inactive)]

> Looks like PL_Base64EncodeBuffer has the same problem.

NSSBase64_EncodeItem() is the only caller at the moment:
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/nssb64e.c#664
it handles "inItem->len == 0" at the top.

Returning NULL for encoding "" seems wrong to me (it should succeed and
return "" as the result), but I don't now this code library well.

I'm not intending to fix PL_Base64EncodeBuffer or NSSBase64_EncodeItem.
Please file new bugs as you see fit.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 521366 [details] [diff] [review]
fix, v3 + crashtest

The code changes looks good. Thanks for the test.  

However, it is misplaced.  security/nss/crashtests/ won't survive next nss merge to moz-central as it is a separate project with it's branch.  The best place for the test is probably security/manager/ssl/crashtests/.

r=honzab with moving the test.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 521366 [details] [diff] [review]
fix, v3 + crashtest

See the previous comment please.

Comment 19

[Mats Palmgren (inactive)]

I moved the test to security/manager/ssl/crashtests/ and landed on Cedar:
http://hg.mozilla.org/projects/cedar/rev/77ce8faa1906

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/77ce8faa1906

Comment 21

[Kai Engert (:KaiE:)]

Reopening.

This fix wasn't dealt with properly.

You incorrectly assumed that this patch would be in PSM.
But PSM is mozilla/security/manager

This patch changes
  security/nss/lib/util/nssb64d.c
which is NSS!

Today I tried to land an updated snapshot of NSS.
Despite my previous testing, I was surprised to see a crash failure.
I backed out.

Then glandium pointed me to this bug.

The change to 
  security/nss/lib/util/nssb64d.c

must be reviewed by a NSS developer and added to NSS.

Comment 22

[Kai Engert (:KaiE:)]

Please remember that any changes in directories:
- nsprpub
- dbm
- security/coreconf
- security/dbm
- security/nss

will get OVERWRITTEN whenever updating to newer NSPR/NSS snapshots.

The only exception to land patches in mozilla-central into these directories are urgent build blockers, but they must be noted in directory security/patches and you must make sure you drive any fixes into NSPR/NSS upstream.
Thanks

Comment 23

[Kai Engert (:KaiE:)]

Attachment: NSS portion of v3

NSS portion that needs review.

Comment 24

[Kai Engert (:KaiE:)]

Please see the discussion in the already existing dependent bug 346583, the proper fix should be done there.

Comment 25

[Kai Engert (:KaiE:)]

Comment on attachment 529962 [details] [diff] [review]
NSS portion of v3

r-

see discussion in bug 346583

Comment 26

[Kai Engert (:KaiE:)]

I did a tryserver build with this patch backed out,
and with the patch from bug 346583 applied.

All tests were run, and all tests passed.

Comment 27

[Kai Engert (:KaiE:)]

Wan-Teh and I have discussed that the right way to deal with this is to backout this patch, and we have r=bsmith

Comment 28

[Kai Engert (:KaiE:)]

backed out
http://hg.mozilla.org/mozilla-central/rev/a921de81bc45

You should be able to reland your tests once the dependent NSS bug has been fixed, and the corresponding NSS release delivered into mozilla-central.

(Unfortunately it was too late for NSS 3.12.10, which will get released today.)

Comment 29

[Mats Palmgren (inactive)]

Will this be fixed in mozilla-central soon?
This bug blocking meaningful use of Zalewski's fuzzers...

Comment 30

[Wan-Teh Chang]

The NSS bug 346583 is fixed in NSS 3.12.11.
mozilla-central is using NSS_3_13_BETA1, which also has
the NSS bug fix.  So I believe you can mark this bug
fixed now.

Comment 31

[:Cykesiopka]

Marking fixed based on Comment 30.

In any case, these two functions were removed in Bug 1030963:
https://hg.mozilla.org/mozilla-central/diff/68499003df5e/security/manager/ssl/src/nsCrypto.cpp#l1.2207
https://hg.mozilla.org/mozilla-central/diff/68499003df5e/security/manager/ssl/src/nsCrypto.cpp#l1.1839