1449355 - Latent heap corruption on allocator mismatch in DecodeQOrBase64Str()

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[mozillabugs]

DecodeQOrBase64Str() (netwerk\mime\nsMIMEHeaderParamImpl.cpp) allocates memory on the PR heap, then frees the same memory on the global heap. If these are different heaps, this mismatch will corrupt both heaps.

The bug is that PL_Base64Decode(), called by line 1128, allocates |decodedText| using PR_MALLOC(), but line 1148 frees it using free():

	1120: nsresult DecodeQOrBase64Str(const char *aEncoded, size_t aLen, char aQOrBase64,
	1121:                             const char *aCharset, nsACString &aResult)
	1122: {
	1123:   char *decodedText;
	1124:   NS_ASSERTION(aQOrBase64 == 'Q' || aQOrBase64 == 'B', "Should be 'Q' or 'B'");
	1125:   if(aQOrBase64 == 'Q')
	1126:     decodedText = DecodeQ(aEncoded, aLen);
	1127:   else if (aQOrBase64 == 'B') {
	1128:     decodedText = PL_Base64Decode(aEncoded, aLen, nullptr);
	1129:   } else {
	1130:     return NS_ERROR_INVALID_ARG;
	1131:   }
	1132: 
	1133:   if (!decodedText) {
	1134:     return NS_ERROR_INVALID_ARG;
	1135:   }
	...
	1148:   free(decodedText);
	...
	1155: }


On Windows builds, these functions use the same heap, so this is only a theoretical bug there. If, however, |_PR_ZONE_ALLOCATOR| is defined in the build, and either the symbol |nspr_use_zone_allocator| or the environment variable |NSPR_USE_ZONE_ALLOCATOR| is defined to |1|, PR_MALLOC() uses a different heap from free().

It does not appear that any builds currently set |nspr_use_zone_allocator| or |NSPR_USE_ZONE_ALLOCATOR| to |1|.


However, it would be possible to cause NSPR to use a separate heap by setting the environment variable |NSPR_USE_ZONE_ALLOCATOR| under Linux, since |_PR_ZONE_ALLOCATOR| is defined in Linux builds indirectly via the definition of |_PR_PTHREADS| in obj-x86_64-pc-linux-gnu/config/external/nspr/pr/backend.mk , which contains the line

	3:	DEFINES += -DDEBUG=1 -D_NSPR_BUILD_ -DHAVE_POINTER_LOCALTIME_R -DHAVE_FCNTL_FILE_LOCKING -D_GNU_SOURCE -DLINUX -D_PR_PTHREADS

That declaration is then used by nsprpub/pr/include/private/primpl.h to define |_PR_ZONE_ALLOCATOR|:

	1872: #if defined(_PR_PTHREADS)
	1873: #define _PR_ZONE_ALLOCATOR
	1874: #endif


It appears that |NSPR_USE_ZONE_ALLOCATOR| is supposed to work, per https://bugzilla.mozilla.org/show_bug.cgi?id=200335 , https://bugzilla.mozilla.org/show_bug.cgi?id=196880 , but it won't with the above or similar bugs, as https://bugzilla.mozilla.org/show_bug.cgi?id=696599 shows.

Comment 1

[Andrew McCreight [:mccr8]]

I think nsMIMEHeaderParamImpl is more of a networking file.

Comment 2

[Daniel Veditz [:dveditz]]

Downgrading to sec-low because this isn't going to happen in a Mozilla build, but could in a distro build or some developer playing around (but lots of things can break in the latter case).

Comment 3

[Daniel Stenberg [:bagder]]

Attachment: bug 1449355 - use the right free function in DecodeQOrBase64Str

Memory returned from PL_Base64Decode() is allocated with PR_MALLOC and
therefore needs to be freed with PR_FREE(), not free().

MozReview-Commit-ID: GzWWsT2nVfQ

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/0f64a4699ae8ffbd3b8fac4816d602a0611811df

BTW, you can disable the MozReview extension now, Daniel. Will make those MozReview-Commit-ID lines go away from your commit messages :)

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> Downgrading to sec-low because this isn't going to happen in a Mozilla
> build, but could in a distro build or some developer playing around (but
> lots of things can break in the latter case).

So the allocator mismatch didn't actually affect our builds?
Do we have a way finding other similar issues?

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/0f64a4699ae8

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to Valentin Gosu [:valentin] from comment #5)
> So the allocator mismatch didn't actually affect our builds?
> Do we have a way finding other similar issues?

Static analysis can find the potential mismatches, at least some times. Instrumented tools like valgrind can as well. If we want to ignore the ones that we hope are OK in our own builds you can instruct the analyzers that different allocator variants are equivalent.

One scanner we do use, Coverity, didn't flag this occurance. I don't know if it's because the connection between alloc and free was not discovered in this case or because it "knows" that in our builds NSPR uses the same underlying allocator.