227296 - NSS_CMSAttribute_AddValue adds the address of a SECItem on the stack to the attr->values array

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

In lib/smime/cmsattr.c, function NSS_CMSAttribute_AddValue
adds the address of a SECItem on the stack (copiedvalue)
to the attr->values array.  &copiedvalue is no longer valid
as soon as we return from this function.

Fortunately, this function is not being used by NSS and is
not exported from the smime3 shared library.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

I am assuming that it is wrong to pass a NULL 'value'
to NSS_CMSAttribute_AddValue because NULL is used to
terminate the attr->values array.  Note that a 'NULL'
value can be passed to NSS_CMSAttribute_Create to
create a NSSCMSAttribute without a value.

This patch also fixes two other bugs I found.
1. In NSS_CMSArray_Add, we should check for the failure
to allocate or grow memory in the arena.
2. In NSS_CMSAttribute_Create, we should check for the
failure of NSS_CMSArray_Add.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This patch looks correct, but I'd like to suggest an alternative change for
NSS_CMSAttribute_AddValue.  I'd suggest adding a new function, 
SECItem * SECITEM_ArenaDupItem(PLArenaPool *poolp, SECItem *src)
to lib/util/secitem.c 
and then change NSS_CMSAttribute_AddValue to use that function.

The new function would allocate both the SECItem itself and the data to 
which it points from the arena.  
I'd suggest that If poolp is null, it would just call SECITEM_DupItem.

Then we could look for other places that use both SECITEM_AllocItem and 
SECITEM_CopyItem and change them to use this func, too.

Comment 3

[Robert Relyea]

Comment on attachment 136677 [details] [diff] [review]
Proposed patch

r=relyea.

I like nelson's suggestion as well. A 'DupItem' in secutil would be usefull.

bob

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch v2

This patch implements Nelson's suggestion.  I added
a new function SECITEM_ArenaDupItem.  Two comments.

1. SECITEM_ArenaDupItem is what SECITEM_DupItem should
have been.
2. I found that the code sequence SECITEM_AllocItem +
SECITEM_CopyItem actually leaks memory because the item's
data buffer is allocated in SECITEM_AllocItem and allocated
again in SECITEM_CopyItem. This is why SECITEM_ArenaDupItem
cannot call SECITEM_CopyItem.  Fortunately, if 'arena' is
not NULL, this leak is in the arena and is not really a
problem.

Comment 5

[Wan-Teh Chang]

Comment on attachment 136698 [details] [diff] [review]
Proposed patch v2

Another comment about this patch.

3. The behavior of SECITEM_DupItem is changed when from->len
is 0.  In the original code, the returned item's 'data' is
not NULL (because we set it to PORT_Alloc(0), which is not
NULL on most platforms).  In the new code, the returned item's
'data' is NULL because we won't call PORT_Alloc if from->len
is 0.

I believe this behavior change is not a problem, but I'd
like the reviewers to confirm this.  If we want to be
conservative I can rewrite SECITEM_ArenaDupItem to avoid
this behavior change.  Let me know what you think.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 136698 [details] [diff] [review]
Proposed patch v2

r=MisterSSL with a suggestion

My only concern for this patch is the one expressed in comment 5 above. 

I think callers SHOULD NOT expect SECITEM_DupItem to allocate a buffer 
for a length of zero bytes, but code might do that inadvertently.  
So, I'd suggest that SECITEM_ArenaDupItem should 

a) PORT_Assert(from->len || arena);
   If we hit this assertion, then we know our assumption was invalid.

and/or 

b) emulate the old behavior, allocating a buffer even for zero length.

Comment 7

[Wan-Teh Chang]

Attachment: Proposed patch v2.1

Implemented Nelson's suggestion: emulate the old behavior,
allocating a buffer even for zero length.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 136902 [details] [diff] [review]
Proposed patch v2.1

r=MisterSSL.  Looks good to me.

Comment 9

[Wan-Teh Chang]

Patch checked in.