Closed Bug 1262333 Opened 4 years ago Closed 4 years ago

heap-buffer-overflow read in [@mozilla::image::Downscaler::CommitRow]

Categories

(Core :: ImageLib, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 1249578

People

(Reporter: tsmith, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-bounds, sec-high, testcase, Whiteboard: [gfx-noted])

Attachments

(3 files)

Attached file call_stack.txt
This may be a dup of bug 1249578.

==64847==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x604000297980 at pc 0x7f24c4a5bcd1 bp 0x7f24a01f1960 sp 0x7f24a01f1958
READ of size 8 at 0x604000297980 thread T36 (ImgDecoder #2)
    #0 0x7f24c4a5bcd0 in mozilla::image::Downscaler::CommitRow() /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/Downscaler.cpp:230
...
See attached log.
Attached file test_case.html
Attached image test_case.ico
Hmm, I don't get an asserts when loading this (haven't tried with ASAN or valgrind yet).
If I make the img 40px x 40px then I can trigger the assert from bug 1249578. Which makes sense since the ico file says the embedded png is 256x256, but it really is 32x32.
(In reply to Timothy Nikkel (:tnikkel) from comment #4)
> If I make the img 40px x 40px then I can trigger the assert from bug
> 1249578. Which makes sense since the ico file says the embedded png is
> 256x256, but it really is 32x32.

So it sounds like the assert is incomplete or this is a different bug all together?
(In reply to Tyson Smith [:tsmith] from comment #5)
> (In reply to Timothy Nikkel (:tnikkel) from comment #4)
> > If I make the img 40px x 40px then I can trigger the assert from bug
> > 1249578. Which makes sense since the ico file says the embedded png is
> > 256x256, but it really is 32x32.
> 
> So it sounds like the assert is incomplete or this is a different bug all
> together?

Nevermind, I was testing on a retina screen. If I test on non-retina screen then it asserts. This makes sense since 224 * 2 is larger than 256 (the size the ico claims to be). So this is likely the same issue basic issue, although this testcase makes things go much more wrong than bug 1249578 (but we already knew that was possible).
Assignee: nobody → tnikkel
Whiteboard: [gfx-noted]
Timothy, have you managed to look at this yet? Thanks.
Flags: needinfo?(tnikkel)
Sec high, assuming 49 is affected here at least.
I already looked at this. It's the same issue as bug 1249578. Fixing bug 1249578 should fix this bug.
Flags: needinfo?(tnikkel)
Depends on: 1249578
Bug 1249578 has landed. Can you confirm that this issue is fixed and close the bug, Tyson? Thanks.
Flags: needinfo?(twsmith)
Looks good.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(twsmith)
Resolution: --- → FIXED
Group: gfx-core-security → core-security-release
Resolution: FIXED → DUPLICATE
Duplicate of bug: 1249578
No longer depends on: 1249578
Removing tracking nom and status flags as this is well tracked in the duplicate bug and the issue is verified as fixed.
Blocks: grizzly
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.