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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[mozillabugs]

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: }

Comment 1

[Daniel Veditz [:dveditz]]

This probably doesn't matter in Mozilla builds of Firefox, but on Linux distros using system NSPR it could matter.

Comment 2

[Honza Bambas (:mayhemer)]

I'm probably the one to work on this, but right now don't have cycles.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Attachment: 1449358.patch

Comment 4

[Honza Bambas (:mayhemer)]

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

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

Thanks!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

If this primarily affects Linux distros, should we consider this for backport to 60 for the next ESR?

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

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

Comment 9

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

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

fix for sec-moderate issue for linux distros, let's get this into beta 16.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/23ab5a2827234ac4cfda02ffb93cf0598d510f14