Closed Bug 1270537 Opened 4 years ago Closed 4 years ago
woff2: heap-buffer-overflow write in [@Reconstruct
19.06 KB, application/octet-stream
4.15 KB, text/plain
90.38 KB, patch
|Details | Diff | Splinter Review|
Found while fuzzing woff2 commit 2bc6acf6dfa2f6dd2f8da63b4fe2e27851e45e11 ==61217==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62600000e960 at pc 0x0000004a79b6 bp 0x7ffd6de31c50 sp 0x7ffd6de31400 WRITE of size 10320 at 0x62600000e960 thread T0 #0 0x4a79b5 in __asan_memcpy /home/user/Desktop/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:421 #1 0x51f620 in woff2::Buffer::Read(unsigned char*, unsigned long) /home/user/code/woff2/./src/./buffer.h:86:7 #2 0x51f620 in woff2::(anonymous namespace)::ReconstructGlyf(unsigned char const*, woff2::Table*, unsigned int*, woff2::Table*, unsigned int*, woff2::(anonymous namespace)::WOFF2FontInfo*, woff2::WOFF2Out*) /home/user/code/woff2/src/woff2_dec.cc:500 #3 0x51f620 in woff2::(anonymous namespace)::ReconstructFont(unsigned char*, unsigned int, woff2::(anonymous namespace)::RebuildMetadata*, woff2::(anonymous namespace)::WOFF2Header*, unsigned long, woff2::WOFF2Out*) /home/user/code/woff2/src/woff2_dec.cc:917 #4 0x51f620 in woff2::ConvertWOFF2ToTTF(unsigned char const*, unsigned long, woff2::WOFF2Out*) /home/user/code/woff2/src/woff2_dec.cc:1282 #5 0x572252 in fuzz(char*) /home/user/code/woff2/src/woff2_decompress.cc:36:19 #6 0x5739a2 in main /home/user/code/woff2/src/woff2_decompress.cc:52:10 #7 0x7f4e38ee4ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287 #8 0x420f75 in _start (/home/user/Desktop/woff2/woff2_decompress+0x420f75)
Rod, this appears to be a problem in the woff2 code; could you look into this, please?
Tracking for 49 since this is sec-critical. Does this affect other branches? Thanks.
woff2 commit a15a8ab86b934cf436f8fac4a8a11ddf8b59d792 seems to fix the issue. However the following commit 8f3ff26c376cca6a19bb1b819ff21309a2cf42d1 (May 17th) fixes another out of bounds issue. Someone else is clearly fuzzing this so we should keep an eye open for incoming fixes.
Liz, this affects everything.
We should simply pull an updated version of the woff2 code; I don't think it makes sense to cherry-pick this specific fix, given that there have been others fixed upstream as well, even though they may not have been filed as bugs against us.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8756458 [details] [diff] [review] Update woff2 code to latest upstream This is a straightforward update direct from the upstream repo.
Attachment #8756458 - Flags: review?(milan)
Comment on attachment 8756458 [details] [diff] [review] Update woff2 code to latest upstream Review of attachment 8756458 [details] [diff] [review]: ----------------------------------------------------------------- Upstream pull seems like a reasonable thing to do. I imagine we wouldn't want uplifts though, and the trains change in about 10 days, so that should be fast enough.
Attachment #8756458 - Flags: review?(milan) → review+
Comment on attachment 8756458 [details] [diff] [review] Update woff2 code to latest upstream [Security approval request comment] How easily could an exploit be constructed based on the patch? Hard to say; I don't think there's an immediately-obvious indicator here, but OTOH all the changes here already exist upstream, with commit messages that could give a hint where to poke at things (e.g. "Stop trusting header totalSfntSize and glyf origLength") to find a weakness. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, this is simply pulling the latest upstream code. Which older supported branches are affected by this flaw? Beta, aurora; esr45 has older woff2 code as part of the ots library, so we'd need to test that separately to determine if it is similarly affected. If not all supported branches, which bug introduced the flaw? The woff2 module was introduced in bug 1227058. Prior to that we had code for woff2 support within OTS, which may or may not have had similar flaws. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Beta and aurora have the same version of woff2, so should be trivial. How likely is this patch to cause regressions; how much testing does it need? Seems unlikely to cause problems, this is tested upstream. --- Given that we have the same library version on aurora and beta, I think we should consider backporting this at least to aurora; whether we want it on beta this late in the cycle is less clear to me, though I think it'd be pretty safe to take.
This is too late for 47. We're making release builds in less than a week. This can check in on June 21, 2016, two weeks into the next cycle. We'll want branch patches at that time as well, including an ESR45 patch if that is affected.
Whiteboard: [checkin on 6/21]
Attachment #8756458 - Flags: sec-approval? → sec-approval+
(This sec-approval+ only applies to checkin on 6/21)
Comment on attachment 8756458 [details] [diff] [review] Update woff2 code to latest upstream I agree with Al's assessment that if this isn't easily exploitable, it might already be too late for uplift to Beta47.
Attachment #8756458 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Hi Jonathan, can we also get an ESR45 patch for this one? Thanks! I'd like the Aurora48 and ESR45 patch to land at the same time if possible.
The woff2 code we have on esr45 is substantially different from trunk, so I don't know whether the issue here is applicable. Leaving needinfo? in place for now while I investigate....
The testcase here does not trigger any ASAN errors when attempting to load via @font-face in esr45. Given that the woff2 decoding implementation has undergone a substantial refactoring, including having been split out from the OTS library to the separate woff2 module, it's not feasible to identify the exact "same" code there, or a specific fix that could be uplifted. I think we should consider esr45 unaffected -- at least by this specific issue. If we -do- want to update esr45 as a precaution (given that the version of woff2 code we're using there is no longer maintained, and might have latent issues even though this particular testcase doesn't break it), we'd need to take a full update of the ots library and add the separate woff2 module, as per bug 1227058. Personally, I don't think that degree of churn is justified here, given that the testcase doesn't appear to actually affect an esr45 build. Ritu, let me know if you want to do anything further with esr45, in view of the above. Also, given that "aurora48" is now "beta48", should we be landing this on all of trunk/aurora49/beta48 (i.e. reversing the beta- that was given late in the beta47 cycle)?
Flags: needinfo?(jfkthame) → needinfo?(rkothari)
It is okay to land this now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ee3b0069e4841323c512d563ea84df73e5c313 Bug 1270537 - Update woff2 code to latest upstream. r=milan
Hi Dan, I am ok with making this wontfix for esr45 as per Jonathan's comment 16. Are you ok with that? Thanks!
We're heading into beta 6 (of 10) late next week. So it's a bit late in the cycle, but we could give this a try on beta. We would have until the RC build (7/25) to find and fix any regressions. Jonathan, is that ok with you?
Comment on attachment 8756458 [details] [diff] [review] Update woff2 code to latest upstream This landed already on m-c, let's get it at least to aurora for now. The beta 6 build will be July 7 so we still have time to uplift this to beta.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21) > We're heading into beta 6 (of 10) late next week. So it's a bit late in the > cycle, but we could give this a try on beta. We would have until the RC > build (7/25) to find and fix any regressions. Jonathan, is that ok with you? Yes, OK with me.
Comment on attachment 8756458 [details] [diff] [review] Update woff2 code to latest upstream Sec-crit, Beta48+
Attachment #8756458 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main48+]
You need to log in before you can comment on or make changes to this bug.