Closed Bug 934673 Opened 6 years ago Closed 6 years ago

nsRandomGenerator::GenerateRandomBytes can leak

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: keeler, Assigned: jmaccor1012)

Details

(Whiteboard: [good first bug][qa-])

Attachments

(1 file, 1 obsolete file)

1.26 KB, patch
keeler
: review+
Details | Diff | Splinter Review
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...
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.
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.
Attached patch Patch #1 (obsolete) — Splinter Review
Buf was being leaked in the event that PK11_GetInternalSlot failed, and is now properly freed.

See attached patch.
Attachment #828192 - Flags: review?(dkeeler)
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.
Attachment #828192 - Flags: review?(dkeeler) → review-
(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.
Attached patch Patch #2Splinter Review
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).
Attachment #828192 - Attachment is obsolete: true
Attachment #828310 - Flags: review?(dkeeler)
Comment on attachment 828310 [details] [diff] [review]
Patch #2

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

Great - thanks!
Attachment #828310 - Flags: review?(dkeeler) → review+
Assignee: nobody → jmaccor93
Status: NEW → ASSIGNED
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.
https://hg.mozilla.org/mozilla-central/rev/bb7afdb613a7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [good first bug] → [good first bug][qa-]
You need to log in before you can comment on or make changes to this bug.