456409 - GetSlotWithMechanism leaks slot list element in several paths

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

In manager/ssl/src/nsKeygenHandler.cpp function GetSlotWithMechanism 
near line 439, we see a loop that calls PK11_GetNextSafe until it returns
NULL, or until a NULL is found in an array, which (according to the comments)
signifies an OOM error.  That OOM error code path jumps out of the loop,
leaking the slotElement (which is normally freed by the next call to 
PK11_GetNextSafe).  It should call PK11_FreeSlotListElement to free the
slotElement before jumping out of the loop.

I doubt this is happening frequently, but if it did happen, the leak would
prevent the PKCS#11 module for the list element from being C_Finalized at 
shutdown time.  Bug 452751 is an example of the kind of problem that occurs
when that happens.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Also in that same function, near line 482, we see another loop that calls
PK11_GetNextSafe until either 
(a) that function returns NULL, or 
(b) a match is found on the token name.  

This probably breaks out of the loop commonly (unlike the code in comment 0).
When it does so, it leaks the slotElement.  This conceivably might be the 
cause of bug 452751.

Comment 2

[timeless]

Attachment: various bits

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 354570 [details] [diff] [review]
various bits

Here are some comments on this patch.  I will not give it an r+ or r-, 
but will refer it to the PSM module owner for that purpose.

>@@ -476,6 +477,7 @@ GetSlotWithMechanism(PRUint32 aMechanism
>         nsAutoString tokenStr(unicodeTokenChosen);
>         while (slotElement) {
>             if (tokenStr.Equals(NS_ConvertUTF8toUTF16(PK11_GetTokenName(slotElement->slot)))) {
>+                PK11_FreeSlotListElement(slotList, slotElement);
>                 *aSlot = slotElement->slot;
>                 break;
>             }

The above patch frees the slotElement, and then uses the slotElement it 
just freed.  Change the order of those two lines, so that you take the 
slot address out of the element before freeing it.


>@@ -530,8 +530,15 @@ NS_IMETHODIMP nsPK11TokenDB::ListTokens(
> 
>   for (le = PK11_GetFirstSafe(list); le; le = PK11_GetNextSafe(list, le, PR_FALSE)) {
>     nsCOMPtr<nsIPK11Token> token = new nsPK11Token(le->slot);
>+    rv = token
>+      ? array->AppendElement(token)
>+      : NS_ERROR_OUT_OF_MEMORY;
> 
>-    array->AppendElement(token);
>+    if (NS_FAILED(rv)) {
>+      PK11_FreeSlotListElement(list, le);
>+      rv = NS_ERROR_OUT_OF_MEMORY;
>+      goto done;
>+    }
>   }

Is the only possible cause of token being NULL an OOM?
What about a slot that has no token?
In general, functions whose job is to traverse the slots looking
for a token with certain properties should NOT abort the search
because they ecounter a slot with a problematic token.  They 
should just ignore it and go on.  Now, IFF this really is an OOM
condition, and we're sure that there's no point in going on, 
because any further attempts to get tokens for subsequent slots 
will also fail, then this is OK.  Otherwise, it isn't.

>--- a/security/manager/ssl/src/nsSmartCardMonitor.cpp
>+++ b/security/manager/ssl/src/nsSmartCardMonitor.cpp

This patch to nsSmartCardMonitor.cpp seems unrelated to this bug.
I did not review it.

Comment 4

[timeless]

Attachment: per nelson

Comment 5

[timeless]

yes the only way it can fail is the one line which you quoted in context:

>     nsCOMPtr<nsIPK11Token> token = new nsPK11Token(le->slot);

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 355258 [details] [diff] [review]
per nelson

r=kaie, but please make a simple change:

                 // OOM. adjust numSlots so we don't free unallocated memory. 
+                PK11_FreeSlotListElement(slotList, slotElement);
                 numSlots = i;

Please move your new code one line down.
The existing comment and existing assignment should remain next to each other.

Comment 7

[timeless]

http://hg.mozilla.org/mozilla-central/rev/43ee697bd867