Bug1026688

[CID 1202865][CID 1202869] Possible leak of |ciphercx| in sec_PKCS7CreateEncryptObject and NSS_CMSCipherContext_StartEncrypt on failure

[mt, 2015-10-13 17:53:17]

https://codereview.appspot.com/266430043/diff/1/lib/pkcs7/p7local.c
File lib/pkcs7/p7local.c (right):

https://codereview.appspot.com/266430043/diff/1/lib/pkcs7/p7local.c#newcode207
lib/pkcs7/p7local.c:207: sec_PKCS7DestroyEncryptObject(ciphercx);
PK11_DestroyContext() ??

https://codereview.appspot.com/266430043/diff/1/lib/smime/cmscipher.c
File lib/smime/cmscipher.c (right):

https://codereview.appspot.com/266430043/diff/1/lib/smime/cmscipher.c#newcode184
lib/smime/cmscipher.c:184: PK11_DestroyContext(ciphercx, PR_TRUE);
it's customary to do this in the loser: section below.  initialize ciphercx to
NULL, then, if it is non-NULL when you hit the loser block, free it.

[franziskus, 2015-10-13 19:31:45]

On 2015/10/13 17:53:17, mt wrote:
> https://codereview.appspot.com/266430043/diff/1/lib/pkcs7/p7local.c
> File lib/pkcs7/p7local.c (right):
> 
> https://codereview.appspot.com/266430043/diff/1/lib/pkcs7/p7local.c#newcode207
> lib/pkcs7/p7local.c:207: sec_PKCS7DestroyEncryptObject(ciphercx);
> PK11_DestroyContext() ??

not sure where that came from ... fixed

> 
> https://codereview.appspot.com/266430043/diff/1/lib/smime/cmscipher.c
> File lib/smime/cmscipher.c (right):
> 
>
https://codereview.appspot.com/266430043/diff/1/lib/smime/cmscipher.c#newcode184
> lib/smime/cmscipher.c:184: PK11_DestroyContext(ciphercx, PR_TRUE);
> it's customary to do this in the loser: section below.  initialize ciphercx to
> NULL, then, if it is non-NULL when you hit the loser block, free it.

done

[mt, 2015-10-14 21:02:19]

Maybe the change to the loser section was a bad choice.  This has a real bug
now.

https://codereview.appspot.com/266430043/diff/20001/lib/smime/cmscipher.c
File lib/smime/cmscipher.c (right):

https://codereview.appspot.com/266430043/diff/20001/lib/smime/cmscipher.c#new...
lib/smime/cmscipher.c:198: }
This will free the cipher context even if thins succeed, which isn't what you
want.  You could set ciphercx to NULL after line 188.

[franziskus, 2015-10-15 23:25:34]

On 2015/10/14 21:02:19, mt wrote:
> Maybe the change to the loser section was a bad choice.  This has a real bug
> now.
> 
> https://codereview.appspot.com/266430043/diff/20001/lib/smime/cmscipher.c
> File lib/smime/cmscipher.c (right):
> 
>
https://codereview.appspot.com/266430043/diff/20001/lib/smime/cmscipher.c#new...
> lib/smime/cmscipher.c:198: }
> This will free the cipher context even if thins succeed, which isn't what you
> want.  You could set ciphercx to NULL after line 188.

uh, right, somehow missed that.