320336 - SECITEM_AllocItem returns a non-NULL pointer if the allocation of its 'data' buffer fails

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

The documentation (comments in the secitem.h header file)
for SECITEM_AllocItem says:

  Allocate an item.  If "arena" is not NULL, then allocate from there,
  otherwise allocate from the heap.  If "item" is not NULL, allocate
  only the data for the item, not the item itself.  The item structure
  is allocated zero-filled; the data buffer is not zeroed.

  The resulting item is returned; NULL if any error occurs.

Note it says "NULL [is returned] if any error occurs."  This
is not true.  If only the allocation of the 'data' buffer for
the item fails, we still return a non-NULL pointer to the item.

I hope we can fix this bug.  However, since we need to maintain
backward compatibility, we may need to change the documentation
to match the current implementation.

Comment 1

[Wan-Teh Chang]

Attachment: Patch

This patch should fix the bug.  Whether we should fix
this bug is another question.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I think we can safely fix this.  
I seriously doubt any NSS code depends on the present behavior if the data
allocation fails.

Comment 3

[Wan-Teh Chang]

Attachment: Patch v2

I reviewed SECITEM_AllocItem thoroughly.  I added
three comments in this patch to record my questions
and findings.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 206028 [details] [diff] [review]
Patch v2

Comments on patch v2:

>     if (item == NULL) {
 ...
>     } else {
>+	/*
>+	 * Instead of the assertion, should we just set item->data
>+	 * to NULL?
>+	 */

item->data will be overwritten below in all cases, EXCEPT when !len.
more on that below.

> 	PORT_Assert(item->data == NULL);
>+	/*
>+	 * item->type is not touched, so if it's uninitialized, it
>+	 * remains uninitialized.
>+	 */

I think we might document that it is the caller's responsiblity to 
initialize result->type, whether item is null or not.

> 	result = item;
>     }
> 
>     result->len = len;
>     if (len) {
> 	if (arena != NULL) {
> 	    result->data = PORT_ArenaAlloc(arena, len);
> 	} else {
> 	    result->data = PORT_Alloc(len);
> 	}
>+	if (result->data == NULL) {
>+	    goto loser;
>+	}
>     }

When len is zero, result->data is untouched.  If item was NULL, 
result->data will be NULL (zero, actually), otherwise it is untouched.
Is that acceptable?  Here are some possible alternatives for that case:

a) leave it alone (present behavior)
b) treat !len as an error case, and return a NULL result.
c) set result->data to NULL, and do not treat this as an error case,
   so that result->data is the same whether item was null or not.
d) treat the !len case the same as !!len.  Call the allocation function
   either way, making the result dependent on and consistent with the 
   behaviors of the two allocation functions.  Will return NULL result 
   if allocation function returns NULL. (IIRC, Alloc will return NULL, 
   and ArenaAlloc will return non-NULL for zero length.)
e) like d, but don't treat a null result->data as an error when !len.

I think a case can be made to support any of those choices EXCEPT a.

If we believe that this function should never return an allocated result
in which result->data is NULL, then b and d are the only two acceptable
choices.

Comment 5

[Wan-Teh Chang]

Attachment: Patch v3

Nelson, thanks a lot for the analysis.

I like e.  I think it is fine for result->data to be NULL
if len is 0.  This is consistent with libc's malloc.

I implemented e in this patch and added a comment to
secitem.h to note it's the caller's responsibility
to initialize result->type.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 206037 [details] [diff] [review]
Patch v3

Another option occurs to me.

f) another variant of d.  Like d, but add a small positive constant to the 
amount actually allocated, e.g. 

> 	    result->data = PORT_ArenaAlloc(arena, len + 8);
> 	} else {
> 	    result->data = PORT_Alloc(len + 8);

and then always treat a NULL result->data as an error.  

But I agree that option e seems acceptable.  so r+=nelson

Comment 7

[Wan-Teh Chang]

I checked in patch v3 on the NSS trunk (3.12).  SECITEM_AllocItem
now returns NULL if the allocation of the data buffer fails.

Checking in secitem.c;
/cvsroot/mozilla/security/nss/lib/util/secitem.c,v  <--  secitem.c
new revision: 1.12; previous revision: 1.11
done
Checking in secitem.h;
/cvsroot/mozilla/security/nss/lib/util/secitem.h,v  <--  secitem.h
new revision: 1.5; previous revision: 1.4
done

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I would like you to fix this for 3.11.1 also.
otherwise, I think the inconsistency of this behavior between 
3.11 and the trunk would drive me (or us) crazy.

Comment 9

[Wan-Teh Chang]

I checked in patch v3 on the NSS_3_11_BRANCH (3.11.1).

Checking in secitem.c;
/cvsroot/mozilla/security/nss/lib/util/secitem.c,v  <--  secitem.c
new revision: 1.11.28.1; previous revision: 1.11
done
Checking in secitem.h;
/cvsroot/mozilla/security/nss/lib/util/secitem.h,v  <--  secitem.h
new revision: 1.4.28.1; previous revision: 1.4
done

Because of this bug, some callers of SECITEM_AllocItem do not
check its return value, but instead check item->data after the
call returns.  My patch won't break such callers because we
continue to ensure that item->data is NULL under the loser label.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Unforunately, this checkin broke the SMIME tests on both trunk and branch.
It caused the first 40 of the "S/MIME Tests with ECC" to fail with various 
errors, most of which were "cmsutil: problem decoding: security library: improperly formatted DER-encoded message."  Backing out the one change to 
secitem.c solved the problem.  So that is what I am now doing.  :-(

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Backed out.
secitem.c; new revision: 1.11.28.2; previous revision: 1.11.28.1
secitem.c; new revision: 1.13;      previous revision: 1.12

Comment 12

[Wan-Teh Chang]

Sorry about that.  I did only optimized builds today, which
is why I didn't see these test failures, which are assertion
failures.  Here is an example:

Running Tests for smime
smime.sh: S/MIME Tests with ECC ===============================
smime.sh: Signing Detached Message {SHA1} ------------------
cmsutil -S -T -N Alice -H SHA1 -i alice.txt -d ../alicedir -p nss -o alice.dsig.SHA1
Assertion failure: dest == NULL || dest->data == NULL, at secasn1e.c:1563
Abort - core dumped
<TR><TD>Create Detached Signature Alice (SHA1). Core file is detected.
smime.sh: Create Detached Signature Alice (SHA1) . FAILED
cmsutil -D -i alice.dsig.SHA1 -c alice.txt -d ../bobdir 
cmsutil: failed to decode message.
cmsutil: problem decoding: security library: improperly formatted DER-encoded message.
smime.sh: Verifying Alice's Detached Signature (SHA1) . FAILED

The reason for the assertion failure is that my patch v3 also changed
the function's behavior in the len==0 case.  I will attach a new patch
that implements option c in comment 4 tomorrow.

Comment 13

[Wan-Teh Chang]

Attachment: Patch v4

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Regarding "Patch v4":

There are a number of aspects of NSS's memory allocation functions that 
are less than ideal.  For example, IIRC, PORT_ArenaAlloc(arena, len) 
today has the property that if you call it with zero "len", it returns 
a non-NULL pointer.  If you call it on the same arenapool with zero len 
several times in a row, it will return the same non-NULL pointer value 
each time.  This seems "wrong", but I would not be surprised at all to 
learn that some code in NSS depends on this behavior.

We would like to change NSS's memory allocation functions to have more 
rational behavior.  But I fear that NSS may have (much) code that depends 
on the present behavior, code that will break if we "fix" the allocators.
Our recent attempt (documented in this bug) shows us that NSS depends on 
some odd behaviors.  

We can use our QA test scripts in an attempt to disprove that hypothesis,
by showing that the test script continues to run even after such a change 
is made.  But unfortunately, our NSS QA test coverage is inadequate to 
give me a high confidence that it will have tested all the cases.  

Consequently, at this time (last week of April) I think it is too risky to 
contemplate this change for inclusion in NSS 3.11.1, even if it does pass
all.sh.  

So, I would suggest holding this change out of releases upcoming in the 
next 2-3 weeks, and considering it later when we have more time to test 
the hypothesis.

Comment 15

[Wan-Teh Chang]

Nelson,

I should have been more clear.  The reason our S/MIME tests failed
with my patch v3 was that our S/MIME code calls
SECITEM_AllocItem(arena,item=NULL,len=0) (there are also such
calls in other parts of NSS) to mean:
  just allocate the SECItem structure, but do not allocate its
  data buffer.  Initialize its 'data' field to NULL and 'len'
  field to 0.

So, len=0 has a special meaning: allocate a blank SECItem structure
that doesn't have a data buffer.

Then, such blank SECItems are passed to SEC_ASN1EncodeItem, which
asserts that the destination SECItem pointer 'dest' is either NULL
or points to a blank SECItem (dest->data == NULL). This is the
assertion that failed.

So, what PORT_Alloc or PORT_ArenaAlloc returns for len=0 is
irrelevant to this bug because SECITEM_AllocItem doesn't need
to allocate the data buffer when len is 0, and in fact len=0
is a special argument value used to turn off the second allocation
in SECITEM_AllocItem.

Comment 16

[Wan-Teh Chang]

Attachment: Patch v5

Document the special meaning of the len=0 argument in the comment.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

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

I confirm that all.sh passes with this patch in debug mode.  However, I still have concerns about unintended consequences.

Comment 18

[Wan-Teh Chang]

I checked in the patch on the NSS trunk (3.12) only.

Checking in secitem.c;
/cvsroot/mozilla/security/nss/lib/util/secitem.c,v  <--  secitem.c
new revision: 1.14; previous revision: 1.13
done
Checking in secitem.h;
/cvsroot/mozilla/security/nss/lib/util/secitem.h,v  <--  secitem.h
new revision: 1.6; previous revision: 1.5
done

This patch only has three change.
1. Added comments to explain the special case of len=0 and remind
   the caller to initialize the type field.  The existing comments
   weren't changed.  So this change has low risk.
2. Set result->data to NULL explicitly for the len=0 case.  This
   was previously done by zeroing the item or an assertion.  This
   change has low risk.
3. Make the function fail if the allocation of the buffer fails.
   This is a change of behavior, but this is what the function is
   documented to do.  The risk of this change is higher than the
   other changes, but I think it is still low for two reasons:
   - This change of behavior is only manifested when memory
     allocation fails.  Frankly, I doubt any of our customers
     has actually exercise their memory allocation failure code
     paths in their testing.
   - The change of behavior only affects the len!=0 case.

     For the item=NULL subcase, this change of behavior is transparent
     to our customers because our customers already need to handle
     a NULL return value in this case (when the allocation of the
     item fails).  If they have code that checks for a returned item
     with a NULL buffer, that code will simply become dead code.

     For the item!=NULL subcase, this change only affects code that
     uses the function's return value.  Previously, the return
     value would always be non-NULL (=item).  So it's unlikely
     that customers would use the return value without checking.
     (The purpose of using the return value would be to test it;
     otherwise they could just use item.)  These customers most
     likely test item->data directly; that code will continue to
     work.

So I think unintended consequences are unlikely.