934673 - nsRandomGenerator::GenerateRandomBytes can leak

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

22 nsRandomGenerator::GenerateRandomBytes(uint32_t aLength,
...
28   uint8_t *buf = reinterpret_cast<uint8_t *>(NS_Alloc(aLength));
29   if (!buf)
30     return NS_ERROR_OUT_OF_MEMORY;
31 
32   mozilla::ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
33   if (slot == nullptr) {
34     return NS_ERROR_FAILURE;
35   }

If PK11_GetInternalSlot fails, this leaks.
Other things to fix in this function:
1. braces around the NS_ERROR_OUT_OF_MEMORY early return
2. if (!slot), not if (slot == nullptr)
3. Make sure we actually want to use NS_Alloc/Free instead of possibly something else...

Comment 1

[J A]

Hi.

I'd like to try to take a shot at this. I've looked through some of the files via dxr.mozilla.org so that I can easily see the callers/references for these functions. I'm not sure that NS_Alloc/NS_Free is what should be used here, but not seeing either anything that would be used in its place.

Any pointers in the right direction would be helpful.

Comment 2

[J A]

After doing some more research into this, it appears to me that we would want to use PR_Malloc/Free here instead of NS_Alloc/Free.

Comment 3

[J A]

Attachment: Patch #1

Buf was being leaked in the event that PK11_GetInternalSlot failed, and is now properly freed.

See attached patch.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 828192 [details] [diff] [review]
Patch #1

Review of attachment 828192 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch, but I basically told you to do the wrong thing - sorry about that.

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +24,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(aBuffer);
>    *aBuffer = nullptr;
>  
> +  uint8_t *buf = reinterpret_cast<uint8_t *>(PR_Malloc(aLength));

Actually, I kind-of sent you off on a wild goose chase here. All callers of GenerateRandomBytes use NS_Free to free the data allocated, so we shouldn't change this (and them) to PR_Malloc.

@@ +36,1 @@
>      return NS_ERROR_FAILURE;

If we moved this whole part to before the allocation, we wouldn't need the extra NS_Free.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to David Keeler (:keeler) from comment #4)
> @@ +36,1 @@
> >      return NS_ERROR_FAILURE;
> 
> If we moved this whole part to before the allocation, we wouldn't need the
> extra NS_Free.

Well, splinter got rid of the context I was counting on, here. What I meant was to move this:

32   mozilla::ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
33   if (slot == nullptr) {
34     return NS_ERROR_FAILURE;
35   }

to above the allocation of buf.

Comment 6

[J A]

Attachment: Patch #2

I moved the slot stuff before the buf allocation (which does make more sense than having to call NS_Free(buf) on it if the slot fails).

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 828310 [details] [diff] [review]
Patch #2

Review of attachment 828310 [details] [diff] [review]:
-----------------------------------------------------------------

Great - thanks!

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7afdb613a7

Jacob - thanks for working on this patch. It's inbound right now, and barring a backout, it will land on central within a day or so. In the future, when a patch is ready for check-in, you can add checkin-needed to the bug keywords and a tree sheriff will know to check the patch in.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb7afdb613a7