Closed
Bug 1449358
Opened 6 years ago
Closed 6 years ago
Latent heap corruption on allocator mismatch in nsAuthSambaNTLM::GetNextToken()
Categories
(Core :: Networking: HTTP, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: mozillabugs, Assigned: Alex_Gaynor)
References
Details
(Keywords: sec-moderate, Whiteboard: [necko-triaged][adv-main60+])
Attachments
(1 file)
1.38 KB,
patch
|
mayhemer
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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: }
Updated•6 years ago
|
Group: core-security → network-core-security
Flags: sec-bounty?
Comment 1•6 years ago
|
||
This probably doesn't matter in Mozilla builds of Firefox, but on Linux distros using system NSPR it could matter.
Comment 2•6 years ago
|
||
I'm probably the one to work on this, but right now don't have cycles.
Priority: -- → P3
Whiteboard: [necko-triaged]
Assignee | ||
Comment 3•6 years ago
|
||
Assignee: nobody → agaynor
Attachment #8968528 -
Flags: review?(honzab.moz)
Comment 4•6 years ago
|
||
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•6 years ago
|
Keywords: checkin-needed
Comment 5•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b876ed208711bc346d7ca95b0599f6e4eb02ff2e
Keywords: checkin-needed
Comment 6•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b876ed208711
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 7•6 years ago
|
||
If this primarily affects Linux distros, should we consider this for backport to 60 for the next ESR?
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → affected
tracking-firefox-esr60:
--- → ?
Flags: needinfo?(agaynor)
Assignee | ||
Comment 8•6 years ago
|
||
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 9•6 years ago
|
||
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+
Comment 10•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/23ab5a2827234ac4cfda02ffb93cf0598d510f14
Updated•6 years ago
|
tracking-firefox-esr60:
? → ---
Updated•6 years ago
|
Whiteboard: [necko-triaged] → [necko-triaged][adv-main60+]
Updated•6 years ago
|
Group: network-core-security → core-security-release
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•