gfxSVGGlyphsDocument uses unchecked gzip original length for array allocation and access, causing potential use-before-initialization
Categories
(Core :: Graphics: Text, defect, P2)
Tracking
()
People
(Reporter: zzjas98, Assigned: bradwerth)
Details
(Keywords: csectype-uninitialized, reporter-external, sec-low, Whiteboard: [adv-main148+])
Attachments
(2 files)
Steps to reproduce:
- Requires a debug or memory sanitizer build of Firefox
- Unzip the attached
poc.zip - [Optional] Run
poc.pyto regenerate the malicous font and html files. - Start firefox and load the
poc.html
With a debug build, it will trigger the assertion MOZ_ASSERT(size_t(s.next_out - outBuf.Elements()) == origLen);.
[615273] Assertion failure: size_t(s.next_out - outBuf.Elements()) == origLen, at .../firefox/gfx/thebes/gfxSVGGlyphs.cpp:313
#01: ???[.../firefox/obj-ff-debug/dist/bin/libxul.so +0x7935e82]
#02: ???[.../firefox/obj-ff-debug/dist/bin/libxul.so +0x79349c8]
#03: ???[.../firefox/obj-ff-debug/dist/bin/libxul.so +0x793567e]
We tested this on commit 7b95c88f48b802f984fb3804fcc1fea7bdea7c8d.
Analysis
gfxSVGGlyphsDocument::gfxSVGGlyphsDocument(const uint8_t* aBuffer,
...
size_t origLen = (size_t(aBuffer[aBufLen - 1]) << 24) +
(size_t(aBuffer[aBufLen - 2]) << 16) +
(size_t(aBuffer[aBufLen - 3]) << 8) +
size_t(aBuffer[aBufLen - 4]); // <----- Can be a large value
AutoTArray<uint8_t, 4096> outBuf;
if (outBuf.SetLength(origLen, mozilla::fallible)) {
z_stream s = {0};
...
s.next_out = outBuf.Elements();
s.avail_out = outBuf.Length();
...
int result = inflate(&s, Z_FINISH); // <----- Will only populate part of outBuf
if (Z_STREAM_END == result) {
MOZ_ASSERT(size_t(s.next_out - outBuf.Elements()) == origLen); // <----- Correct check, but compiled out in release build
ParseDocument(outBuf.Elements(), outBuf.Length()); // <----- Will parse uninitialized data
...
The gfxSVGGlyphsDocument constructor reads the expected uncompressed size (origLen) from the last 4 bytes of the gzip buffer rather than validating against inflate()'s actual output size. The assertion is checking the correct length MOZ_ASSERT(size_t(s.next_out - outBuf.Elements()) == origLen) but it will be compiled out in release build.
A malicious web font can append trailing bytes after a valid gzip stream to claim a larger uncompressed size, causing large uninitialized heap memory to be parsed by ParseDocument(). The attached poc.py creates such a font file.
Memory Sanitizer
We believe with memory sanitizer, this should be caught inside ParseDocument() when it parses uninitialized data.
However, we had trouble compiling the latest firefox with memory sanitizer.
First issue was that during building, a build script from Rust dependency will trigger memsan, but we were able to overcome this by
setting MSAN_OPTIONS="halt_on_error=0:exit_code=0".
0:30.30 Uninitialized bytes in PosixSpawnImpl at offset 72 inside [0x705000000050, 73)
0:30.30 ==662739==WARNING: MemorySanitizer: use-of-uninitialized-value
0:30.30 #0 0x5617f77d5d6b in posix_spawnp (.../firefox/obj-ff-msan/release/build/serde_core-046b9c04a07c7857/build-script-build+0x130d6b)
0:30.30 #1 0x5617f7872611 in std::sys::pal::unix::process::process_inner::_$LT$impl$u20$std..sys..pal..unix..process..process_common..Command$GT$::spawn::h735dbfc7e9b6bcc1 std.d9e466a2d75004a2-cgu.0
0:30.31 #2 0x5617f7860c00 in std::process::Command::output::ha4b1eeae4c97dae9 (.../firefox/obj-ff-msan/release/build/serde_core-046b9c04a07c7857/build-script-build+0x1bbc00)
The second issue was from gfx/angle (maybe clang version mismatch?):
3:14.74 .../firefox/gfx/angle/checkout/src/common/angleutils.h:478:44: error: use of undeclared identifier '__msan_scoped_disable_interceptor_checks'; did you mean 'MsanScopedDisableInterceptorChecks'?
3:14.74 478 | MsanScopedDisableInterceptorChecks() { __msan_scoped_disable_interceptor_checks(); }
3:14.74 |
We marked this as security since it's very easy to trigger the potential UBI from arbitrary web content, but please feel free to unhide it if it is safe. Please let us know if we missed anything or if you have any feedback! Thank you!
Actual results:
UBI or Assertion failure
Expected results:
Only the populated part of the buffer is used.
Updated•7 months ago
|
| Assignee | ||
Comment 1•7 months ago
|
||
Thank you for the very clear explanation of the issue. I'll try to put together a fix.
Jonathan, this is relevant to web font loading; maybe your thing.
| Assignee | ||
Updated•7 months ago
|
| Assignee | ||
Comment 2•7 months ago
|
||
With this change, the assert is no longer necessary, because origLen is
no longer used to partse the SVG document.
| Assignee | ||
Comment 3•7 months ago
|
||
Jonathan is reviewer of the patch, which will is enough opportunity for him to see what's going on with this Bug.
Comment 4•7 months ago
|
||
Thanks for the report (zzjas98) and the patch (Brad) -- makes sense to me.
While this is definitely a bug, and can result in accessing uninitialized memory in the outBuf array, it's not immediately clear to me how bad of a risk this is. We're potentially passing an uninitialized (or maliciously-crafted!) buffer to ParseDocument(), but I would hope the SVG document parser is reasonably robust against "bad" data in its source document.
(In any case, we should certainly fix this.)
| Assignee | ||
Comment 5•7 months ago
|
||
Comment on attachment 9533473 [details]
(secure)
Security Approval Request
- How easily could an exploit be constructed based on the patch?: Challenging. There are two ways to build an exploit that I can imagine:
- If SVG document parsing can be exploited with arbitrary data, but that would be its own security Bug.
- If SVG document parsing is vulnerable to extraction of the raw data (likely would also be its own security Bug), and the exploiter could position the SVG data adjacent to target memory. Seems challenging.
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
- Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
- If not all supported branches, which bug introduced the flaw?: None
- Do you have backports for the affected branches?: No
- If not, how different, hard to create, and risky will they be?: Trivial to create; probably a clean rebase.
- How likely is this patch to cause regressions; how much testing does it need?: Very unlikely. It removes an assert from debug builds, but that assert was checking an exploitable calculation. This uses the actual source of truth for the stream parsing.
- Is the patch ready to land after security approval is given?: Yes
- Is Android affected?: Yes
Comment 6•7 months ago
|
||
In taking a stab as a security rating I'm going to set aside the "expoit ParseDocument()" case as Brad did above. That would be a completely separate bug, if possible, and could be exploited directly by including the attack in the correctly-inflated data without using this flaw.
Scooping up big chunks of memory here is easy, but it has to successfully parse as something that the attacker could recover from their document. Would be much harder to conduct a "fishing expedition" in that memory compared to an equivalent image decoder bug that incorporates the data as "noise" into an image that can be read back from a <canvas>. Can't really say it's impossible, but it does seem unlikely.
Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
There were two parts to that question 😉
Updated•7 months ago
|
Comment 7•7 months ago
|
||
Comment on attachment 9533473 [details]
(secure)
Clearing flag as it was rated sec-low
Updated•7 months ago
|
Comment 9•7 months ago
|
||
Comment 10•7 months ago
|
||
The patch landed in nightly and beta is affected.
:bradwerth, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
- If no, please set
status-firefox147towontfix.
For more information, please visit BugBot documentation.
| Assignee | ||
Updated•7 months ago
|
Updated•7 months ago
|
Updated•6 months ago
|
Updated•5 months ago
|
Updated•5 months ago
|
Updated•2 months ago
|
Description
•