Latent heap corruption on allocator mismatch in nsAuthSambaNTLM::GetNextToken()

RESOLVED FIXED in Firefox -esr60

Status

()

defect
P3
normal
RESOLVED FIXED
Last year
8 months ago

People

(Reporter: mozillabugs, Assigned: Alex_Gaynor)

Tracking

({sec-moderate})

58 Branch
mozilla61
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 fixed, firefox59 wontfix, firefox60 fixed, firefox61 fixed)

Details

(Whiteboard: [necko-triaged][adv-main60+])

Attachments

(1 attachment)

Reporter

Description

Last year
nsAuthSambaNTLM::GetNextToken() (extensions\auth\nsAuthSambaNTLM.cpp) allocates memory on the PR heap, then frees the same memory on the global heap. This mismatch will corrupt both heaps if they are different (see https://bugzilla.mozilla.org/show_bug.cgi?id=1449355 for details).

The bugs are on lines 251 and 268, which use global |free()| instead of PR_Free(). |encoded| on line 244 is allocated by PL_Base64Encode() using PR_MALLOC(), and |buf| on line 264 is allocated by ExtractMessage() by calling PL_Base64Decode(), which then calls PR_MALLOC():


228: NS_IMETHODIMP
229: nsAuthSambaNTLM::GetNextToken(const void *inToken,
230:                               uint32_t    inTokenLen,
231:                               void      **outToken,
232:                               uint32_t   *outTokenLen)
233: {
234:     if (!inToken) {
235:         /* someone wants our initial message */
236:         *outToken = nsMemory::Clone(mInitialMessage, mInitialMessageLen);
237:         if (!*outToken)
238:             return NS_ERROR_OUT_OF_MEMORY;
239:         *outTokenLen = mInitialMessageLen;
240:         return NS_OK;
241:     }
242: 
243:     /* inToken must be a type 2 message. Get ntlm_auth to generate our response */
244:     char* encoded = PL_Base64Encode(static_cast<const char*>(inToken), inTokenLen, nullptr);
245:     if (!encoded)
246:         return NS_ERROR_OUT_OF_MEMORY;
247: 
248:     nsCString request;
249:     request.AssignLiteral("TT ");
250:     request.Append(encoded);
251:     free(encoded);
...
264:     uint8_t* buf = ExtractMessage(line, outTokenLen);
265:     if (!buf)
266:         return NS_ERROR_FAILURE;
267:     *outToken = nsMemory::Clone(buf, *outTokenLen);
268:     free(buf);
...
277: }
Reporter

Updated

Last year
See Also: → 1449355
Group: core-security → network-core-security
Flags: sec-bounty?
This probably doesn't matter in Mozilla builds of Firefox, but on Linux distros using system NSPR it could matter.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-moderate
I'm probably the one to work on this, but right now don't have cycles.
Priority: -- → P3
Whiteboard: [necko-triaged]
Assignee

Comment 3

Last year
Posted patch 1449358.patchSplinter Review
Assignee: nobody → agaynor
Attachment #8968528 - Flags: review?(honzab.moz)
Comment on attachment 8968528 [details] [diff] [review]
1449358.patch

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

Thanks!
Attachment #8968528 - Flags: review?(honzab.moz) → review+
Assignee

Updated

Last year
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b876ed208711
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: sec-bounty? → sec-bounty+
If this primarily affects Linux distros, should we consider this for backport to 60 for the next ESR?
Assignee

Comment 8

Last year
Comment on attachment 8968528 [details] [diff] [review]
1449358.patch

Requesting uplift to beta because it's a security issue that'd primarily affect linux distros.

Approval Request Comment
[Feature/Bug causing the regression]: Security issue
[User impact if declined]: Vulnerable to a potential security issue
[Is this code covered by automated tests?]: Not sure, doubtful.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: nope
[Why is the change risky/not risky?]: very small change that's easily amenable to human review
[String changes made/needed]: none
Flags: needinfo?(agaynor)
Attachment #8968528 - Flags: approval-mozilla-beta?
Comment on attachment 8968528 [details] [diff] [review]
1449358.patch

fix for sec-moderate issue for linux distros, let's get this into beta 16.
Attachment #8968528 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [necko-triaged] → [necko-triaged][adv-main60+]
Group: network-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.