192179 - CMS libraries inconsistent in use of NSSCMSGetDecryptKeyCallback

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Javier Delgadillo]

When encoding or decoding an NSSCMSEncrytpedData message, the CMS libraries ask
the application to provide a callback that determines the symmetric to use when
encrypting/decrypting the EncryptedData.

Unfortunately, the code is inconsistent in it's calling of PK11_FreeSymKey on
the symmetric key that is returned.  So depending on whether the app decides to
call PK11_ReferenceSymKey or not in its callback implementation, the app runs
the risk of either leaking a reference to the key's slot (causing NSS_Shutdown
to fail) or having a dangling pointer while decoding an encrypted data.

For now, the workaround is to call PK11_ReferenceSymKey before calling decode
and having the callback implementation not call PK11_RefernceSymKey.

The 2 blocks where the callback is used:
http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsencdata.c#156
(encoding)

http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsencdata.c#230
(decoding)

Comment 1

[Robert Relyea]

You should return a reference to the key. Encrypt should free the reference when
it is done. The bug is the code is assuming that NSS_CMSContentInfo_SetBulkKey is
adopting the key, but it gets it's own reference.

The app has to return the reference because otherwise there is a race between the
app freeing the key and the CMS code getting it's own reference.

bob

Comment 2

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch

Bob, this is the fix you described, right?

Comment 4

[Robert Relyea]

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

Yes, that's the correct patch

Comment 5

[Wan-Teh Chang]

Bob, could you check this fix in on the trunk and
the NSS_3_7_BRANCH?

Comment 6

[Robert Relyea]

Fix checked into branch and trunk

Comment 7

[Wan-Teh Chang]

Kai,

I'd like you to verify that PSM is not affected by this
bug.

This bug is concerned with the NSSCMSGetDecryptKeyCallback
function, which has this prototype:

typedef PK11SymKey *(*NSSCMSGetDecryptKeyCallback)(void *arg, SECAlgorithmID
*algid);

I did a search for "SECAlgorithmID" and "PK11SymKey" in
mozilla/security/manager, and the only function with that
prototype is in
mozilla/security/manager/ssl/src/nsNSSComponent.cpp:

static PK11SymKey * GetDecryptKeyCallback(void *arg, 
                                                 SECAlgorithmID *algid)
{
  return nsnull;
}


This function is not returning any bulkkey, so PSM can't possibly
be affected by this bug, which has to do with the reference count
of the returned bulkkey.  Do you agree?

Comment 8

[Kai Engert (:KaiE:)]

Wan-Teh, I fully agree with your analysis, I also can't find other areas of code
that would be affected, from what you are describing.