Bug 1209152 - Fix possible PLArena leak in ssl3_HandleECDHServerKeyExchange

Bug 1209152 - Fix possible PLArena leak in ssl3_HandleECDHServerKeyExchange

[ekr-rietveld, 2015-09-29 14:57:27]

https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c#newcode710
lib/ssl/ssl3ecc.c:710: }
peerKey->arena points to arena and 
SECKEY_DestroyPublicKey() destroys the arena:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/cryptohi/seck...

So, I think the minimal change here is simply to delete the arena before
the call to no_memory and not change anything else

[ekr-rietveld, 2015-09-29 15:09:03]

Note also the comparable code in the non-ECC DH branch.

[wtc1, 2015-09-29 18:34:45]

https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c#newcode721
lib/ssl/ssl3ecc.c:721: errCode = SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
If we follow Eric's minimal change suggestion, we also need to add a
PORT_FreeArena(arena, PR_FALSE) call here.

I probably would go to the current change. There are two copies of
similar code in lib/ssl/ssl3con.c. Please make sure we use the same
approach in all three places. Consistency is very important.

[ekr-rietveld, 2015-09-29 18:37:55]

https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c#newcode721
lib/ssl/ssl3ecc.c:721: errCode = SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
On 2015/09/29 18:34:45, wtc1 wrote:
> If we follow Eric's minimal change suggestion, we also need to add a
> PORT_FreeArena(arena, PR_FALSE) call here.

Perhaps I am misunderstanding your suggestion, but I am not sure
that this is correct. In the original code, ss->sec.peerKey has already been
assigned, so when it is destroyed, it should destroy the arena as a side effect.



> I probably would go to the current change. There are two copies of
> similar code in lib/ssl/ssl3con.c. Please make sure we use the same
> approach in all three places. Consistency is very important.

[wtc1, 2015-09-29 20:20:11]

https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c#newcode721
lib/ssl/ssl3ecc.c:721: errCode = SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
On 2015/09/29 18:37:55, ekr-webrtc wrote:
> 
> Perhaps I am misunderstanding your suggestion, but I am not sure
> that this is correct. In the original code, ss->sec.peerKey has already been
> assigned, so when it is destroyed, it should destroy the arena as a side
effect.

I see. You're right. I missed the change on line 707.

I think it's better to assign ss.sec.peerKey only when we have constructed
|peerKey| completely.

[ekr-rietveld, 2015-09-29 20:26:01]

On 2015/09/29 20:20:11, wtc1 wrote:
> https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c
> File lib/ssl/ssl3ecc.c (right):
> 
> https://codereview.appspot.com/269030043/diff/1/lib/ssl/ssl3ecc.c#newcode721
> lib/ssl/ssl3ecc.c:721: errCode = SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
> On 2015/09/29 18:37:55, ekr-webrtc wrote:
> > 
> > Perhaps I am misunderstanding your suggestion, but I am not sure
> > that this is correct. In the original code, ss->sec.peerKey has already been
> > assigned, so when it is destroyed, it should destroy the arena as a side
> effect.
> 
> I see. You're right. I missed the change on line 707.
> 
> I think it's better to assign ss.sec.peerKey only when we have constructed
> |peerKey| completely.

I agree that this would be the best practice. Tim, can you produce a patch that
does this as well as for the FF DHE version?

[ttaubert, 2015-09-30 13:22:23]

On 2015/09/29 20:26:01, ekr-webrtc wrote:
> On 2015/09/29 20:20:11, wtc1 wrote:
> > I think it's better to assign ss.sec.peerKey only when we have constructed
> > |peerKey| completely.
> 
> I agree that this would be the best practice. Tim, can you produce a patch
that
> does this as well as for the FF DHE version?

Yes, will do.