1209152 - [Coverity 1123241] Leak of memory or pointers to system resources in ssl3_HandleECDHServerKeyExchange

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

>  arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>  if (arena == NULL) {
>      goto no_memory;
>  }
>
>  ss->sec.peerKey = peerKey = PORT_ArenaZNew(arena, SECKEYPublicKey);
>  if (peerKey == NULL) {
>      goto no_memory;
>  }

9. leaked_storage: Variable arena going out of scope leaks the storage it points to.

Comment 1

[Eric Rescorla (:ekr)]

Assuming I am reading this correctly, this creates a leak if we have an OOM in
PORT_ArenaZNew, right? 

I see that below we actually do handle this case.

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3ecc.c?case=true&from=ssl3_HandleECDHServerKeyExchange#728

Probably the right fix is to have no_memory delete arena if non-nULL

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Eric Rescorla (:ekr) from comment #1)
> Assuming I am reading this correctly, this creates a leak if we have an OOM
> in PORT_ArenaZNew, right? 

Yeah, although it's very unlikely because we just created the arena. OTOH |PORT_ArenaAlloc()| can also fail if another thread holds a mark in it.

> Probably the right fix is to have no_memory delete arena if non-nULL

Yeah, that would also cover |alert_loser| where we currently don't free the arena either, and I think we should?

Another thing: in that function we assign to |ss->sec.peerKey| twice, once when allocating and again shortly before returning SECSuccess. We should probably remove the first assignment? I assume it is the reason why Coverity isn't complaining about the |alert_loser| case if we assigned it to |ss| already before bailing out.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Patch at: https://codereview.appspot.com/269030043/

Comment 4

[Eric Rescorla (:ekr)]

Comments up.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Fixed by bug 1209546.