346583 - ATOB_ConvertAsciiToItem and NSSBase64_DecodeBuffer should fail gracefully

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Kai Engert [:KaiE:]]

When ATOB_ConvertAsciiToItem is called with parameter 2 (char *ascii) being the empty string (""), we crash.

See bug 345094 for more details and a testcase.

Comment 1

[Johnny Stenback (:jst)]

NSS *does* fail gracefully here in release builds, if you look at the code, it does this:

    PR_ASSERT(srclen > 0);
    if (srclen == 0)
	return dest;

which to my understanding is perfectly fine. IMO we should simply remove the assertion here.

Either way, this doesn't seem like a security bug at all.

Comment 2

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

Kaie, do you agree with jst's analysis in comment 1?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

The NSS group had a big discussion about this some time ago.
I'm trying to find it. 

Passing a zero srclen to this function is considered a programming error.  
Correct code does not do it.  That's why the assertion exists.

OTOH, we make a distinction between "internal" and "external" programming
errors, "internal" meaning  in our own (NSS, NSPR, PSM?) code, and external
meaning all else.  We try to assert on internal errors and not on external
errors.  So if this is an external error, then I agree, we should remove
the assertion.  

Since this is considered an error, we should set the error code 
(call PORT_SetError) before returning in this case.

In any case, Assertion failures are NOT exploitable security problems,
so removing security sensitive flag.  

Comment 4

[Robert Relyea]

ATOB_ConvertAsciiToItem is an exported public function, so we probably should remove the assert.

bob

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: remove assertion, set error code.

pls review today

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Let's try that again

Alexei caught a big booboo in the first patch.  <blush>

Comment 7

[Wan-Teh Chang]

Comment on attachment 264061 [details] [diff] [review]
Let's try that again

Nelson, your patch modifies PL_Base64DecodeBuffer, not ATOB_ConvertAsciiToItem.
PL_Base64DecodeBuffer is a static function.

Comment 8

[Alexei Volkov]

Comment on attachment 264061 [details] [diff] [review]
Let's try that again

r=alexei.volkov for the patch.

NSSBase64_DecodeBuffer is also a public function that have the same problem.
Also, the function PL_Base64DecodeBuffer return dest, that can potentially be not eq to NULL. But in case of error we return dest. User relies  that in case of error NULL should be returned. I think this function need more changes.

Comment 9

[Wan-Teh Chang]

Comment on attachment 264061 [details] [diff] [review]
Let's try that again

Exactly, PL_Base64DecodeBuffer returns NULL on failure.
So if srclen == 0 is considered an invalid argument, we
should return NULL or goto loser.

It also seems reasonable that an empty string input results
in an empty string output.  If we want to do that, we'll need
to set *output_destlen = 0 before returning dest.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

My conclusion from comments 8 & 9 is that the assertion was not the only
problem with these functions.  I'll be happy to review the next patch if
Alexei or Wan-Teh will write it.  :)

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 264061 [details] [diff] [review]
Let's try that again

The header of this function clearly demands that on error, the function shall return NULL.

Nelson, you claim the assert is not the only problem with the function, asking them to attach the next patch.

But by removing the assertion, you are opening the chance of returning an invalid return value. We shouldn't do that.

r-

Comment 12

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Nelson, I think you want this patch.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Kai, your patch to function PL_Base64DecodeBuffer is better than mine.
But IINM, Wan-Teh and Alexei have claimed (in comment 7 and comment 8) 
that I (we) patched the wrong function, and I think they're right.  
I patched the line of code identified in the call stack for bug 345094.
But PL_Base64DecodeBuffer is an "internal" function (as I used that term
in comment 3), and so the assertion is proper in that function.  It is 
the public ("external") functions that need to detect and avoid this error.
So, getting this right is going to require more work than I originally 
put into it.  

Since this crash only occurs in debug builds, and end users do not run 
debug builds as a rule, what do you think is the proper priority for this
bug?  

Comment 14

[Kai Engert [:KaiE:]]

(In reply to comment #13)
> Kai, your patch to function PL_Base64DecodeBuffer is better than mine.
> But IINM, Wan-Teh and Alexei have claimed (in comment 7 and comment 8) 
> that I (we) patched the wrong function, and I think they're right.  
> I patched the line of code identified in the call stack for bug 345094.
> But PL_Base64DecodeBuffer is an "internal" function (as I used that term
> in comment 3), and so the assertion is proper in that function.  It is 
> the public ("external") functions that need to detect and avoid this error.

Ok, thanks for clarifying, so the right thing is to fix the call to PL_Base64DecodeBuffer in function NSSBase64_DecodeBuffer, which also fixes ATOB_ConvertAsciiToItem.


> Since this crash only occurs in debug builds, and end users do not run 
> debug builds as a rule, what do you think is the proper priority for this
> bug?  

This is probably low priority, but now that we've decided what we want, I think the patch is straightforward to do, I'll attach a new one.

Comment 15

[Kai Engert [:KaiE:]]

Attachment: Patch v4

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 264086 [details] [diff] [review]
Patch v4

Kai,  Your patch is good.  Alexei points out that while we're eliminating
Assertion failures for "external" programming errors, we should also 
get rid if the PORT_Assert call just before your patch (the one below),
and turn it into code that likewise sets an error code and returns NULL.
I agree with him.  

I think this would change the "if" below to:
      if (outItemOpt == NULL || outItemOpt->data == NULL || inLen == 0) {

>     PORT_Assert(outItemOpt == NULL || outItemOpt->data == NULL);
> 
>+    if (inLen == 0) {
>+        PORT_SetError (SEC_ERROR_INVALID_ARGS);
>+        return NULL;
>+    }
>+

Comment 17

[Kai Engert [:KaiE:]]

Attachment: Patch v5

Even better patch...

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 264088 [details] [diff] [review]
Patch v5

r=nelson

Comment 19

[Wan-Teh Chang]

Comment on attachment 264088 [details] [diff] [review]
Patch v5

Did you test the patch?

Let's all take a step back.

>-    PORT_Assert(outItemOpt == NULL || outItemOpt->data == NULL);
>+    if (outItemOpt == NULL || outItemOpt->data == NULL || inLen == 0) {
>+        PORT_SetError (SEC_ERROR_INVALID_ARGS);
>+        return NULL;
>+    }

The boolean expression is wrong.  It should be

      if ((outItemOpt != NULL && outItemOpt->data != NULL) || inLen == 0) {

Comment 20

[Alexei Volkov]

Comment on attachment 264088 [details] [diff] [review]
Patch v5

I agree with Wan-teh. New patch will attempt to dereference a NULL pointer if outItemOpt is eq to NULL. We want to have at least one pointer to be NULL: either SECItem outItemOpt or it's pointer to data.

Comment 21

[Kai Engert [:KaiE:]]

ping. ran into this again yesterday (although only in a debug build, running debug only code).

Comment 22

[Kai Engert [:KaiE:]]

Nelson, do you plan to produce an updated patch?

Comment 23

[Kai Engert [:KaiE:]]

Oh wait a minute. I take that back. Although the bug is assigned to Nelson, it was me who attached the latest patch and probably should address the review comments. Adding to my todo list...

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Giving this bug to Kai, since he's writing the patches.

Comment 26

[Kai Engert [:KaiE:]]

Attachment: Patch v5 with review comments addressed

Let's cut this short.

Wan-Teh has review my patch v5. He made a single change request.
I agree with his change request, and I'm attaching the updated patch.
No other change.
I conclude this has r=wtc, but I'll let him mark it.

Alexei has agreed with Wan-Teh and has requested the same change.
I conclude, this has r=alexei.volkov, but I'll let him mark it.

Comment 27

[Kai Engert [:KaiE:]]

I have successful test results on tryserver with this patch.

Comment 28

[Wan-Teh Chang]

Comment on attachment 529970 [details] [diff] [review]
Patch v5 with review comments addressed

r=wtc.  I suggest making some more changes so that we don't
use outItemOpt and inLen before validating them:

Index: mozilla/security/nss/lib/util/nssb64d.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/util/nssb64d.c,v
retrieving revision 1.7
diff -u -r1.7 nssb64d.c
--- mozilla/security/nss/lib/util/nssb64d.c	5 Oct 2008 20:59:26 -0000	1.7
+++ mozilla/security/nss/lib/util/nssb64d.c	4 May 2011 21:35:40 -0000
@@ -747,17 +747,21 @@
 NSSBase64_DecodeBuffer (PRArenaPool *arenaOpt, SECItem *outItemOpt,
 			const char *inStr, unsigned int inLen)
 {
-    SECItem *out_item = outItemOpt;
-    PRUint32 max_out_len = PL_Base64MaxDecodedLength (inLen);
+    SECItem *out_item = NULL;
+    PRUint32 max_out_len = 0;
     PRUint32 out_len;
     void *mark = NULL;
     unsigned char *dummy;
 
-    PORT_Assert(outItemOpt == NULL || outItemOpt->data == NULL);
+    if ((outItemOpt != NULL && outItemOpt->data != NULL) || inLen == 0) {
+	PORT_SetError (SEC_ERROR_INVALID_ARGS);
+	return NULL;
+    }
 
     if (arenaOpt != NULL)
 	mark = PORT_ArenaMark (arenaOpt);
 
+    max_out_len = PL_Base64MaxDecodedLength (inLen);
     out_item = SECITEM_AllocItem (arenaOpt, outItemOpt, max_out_len);
     if (out_item == NULL) {
 	if (arenaOpt != NULL)

Comment 29

[Wan-Teh Chang]

Attachment: WTC's patch, with additional changes

I added some more changes to Kai's patch v5.

1. In NSSBase64_DecodeBuffer, avoid using input arguments
outItemOpt and inLen before validating them.

2. In PL_Base64DecodeBuffer, return NULL (instead of the
caller-provided output buffer 'dest') to indicate a failure.
Note that I set the NSPR error code PR_INVALID_ARGUMENT_ERROR
because this function seems to be copied from NSPR (or was
intended for NSPR).

Comment 30

[Kai Engert [:KaiE:]]

Comment on attachment 530158 [details] [diff] [review]
WTC's patch, with additional changes

r=kaie on the additional changes

Comment 31

[Kai Engert [:KaiE:]]

trunk:

Checking in util/nssb64d.c;
/cvsroot/mozilla/security/nss/lib/util/nssb64d.c,v  <--  nssb64d.c
new revision: 1.8; previous revision: 1.7
done


3.12 branch for 3.12.11:

Checking in util/nssb64d.c;
/cvsroot/mozilla/security/nss/lib/util/nssb64d.c,v  <--  nssb64d.c
new revision: 1.7.32.1; previous revision: 1.7
done

Comment 32

[Kai Engert [:KaiE:]]

Update to target milestone 3.12.11 when bug 655510 is resolved.