1580317 - UBSan runtime error: [@mozilla::image::ShouldUseHeap]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P2)

Description

[Christoph Diehl [:posidron]]

Attachment: clusterfuzz-testcase-minimized-ImageGIF-5731113032482816

Preemptively marking this s-s.

Task

Item	Description
Crash Type	Integer-overflow
Sanitizer	undefined (UBSAN)
Platform	linux
Job Type	libfuzzer_ubsan_firefox
Fuzz Target	ImageGIF
Reliably Reproduces	YES

Environment

UBSAN_OPTIONS="allocator_release_to_os_interval_ms=500:halt_on_error=1:handle_sigbus=1:handle_sigfpe=1:handle_sigill=1:print_stacktrace=1:print_summary=1:print_suppressions=0:strip_path_prefix=/workspace/:use_sigaltstack=1"

Callstack

Running command: /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_firefox_008d6de92713224b5a96fd56953fbf4e5d586f6e/revisions/ImageGIF -close_fd_mask=3 -timeout=25 -rss_limit_mb=2048 -runs=100 /fuzz-0
Running Fuzzer tests...
INFO: Seed: 2935404131
INFO: Loaded 1 modules   (400761 inline 8-bit counters): 400761 [0x7f01c13f2cb8, 0x7f01c1454a31),
INFO: Loaded 1 PC tables (400761 PCs): 400761 [0x7f01c1454a38,0x7f01c1a721c8),
/mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_firefox_008d6de92713224b5a96fd56953fbf4e5d586f6e/revisions/ImageGIF: Running 1 inputs 100 time(s) each.
Running: /fuzz-0
/src/mozilla-central/image/imgFrame.cpp:90:33: runtime error: signed integer overflow: 115712 * 28928 cannot be represented in type 'int'
    #0 0x7f01bb793b91 in mozilla::image::ShouldUseHeap(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, bool) mozilla-central/image/imgFrame.cpp:90:33
    #1 0x7f01bb773cf0 in mozilla::image::AllocateBufferForImage(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::SurfaceFormat, bool) mozilla-central/image/imgFrame.cpp:107:14
    #2 0x7f01bb773a1c in mozilla::image::imgFrame::InitForDecoder(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::SurfaceFormat, bool, mozilla::Maybe<mozilla::image::AnimationParams> const&, bool) mozilla-central/image/imgFrame.cpp:244:17
    #3 0x7f01bb73c60f in mozilla::image::Decoder::AllocateFrameInternal(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::SurfaceFormat, mozilla::Maybe<mozilla::image::AnimationParams> const&, mozilla::image::RawAccessFrameRef&&) mozilla-central/image/Decoder.cpp:368:9
    #4 0x7f01bb73c16c in mozilla::image::Decoder::AllocateFrame(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::SurfaceFormat, mozilla::Maybe<mozilla::image::AnimationParams> const&) mozilla-central/image/Decoder.cpp:266:19
    #5 0x7f01bb76d105 in mozilla::image::SurfaceSink::Configure(mozilla::image::SurfaceConfig const&) mozilla-central/image/SurfacePipe.cpp:63:35
    #6 0x7f01b9b0be90 in nsresult mozilla::image::RemoveFrameRectFilter<mozilla::image::SurfaceSink>::Configure<mozilla::image::SurfaceConfig>(mozilla::image::RemoveFrameRectConfig const&, mozilla::image::SurfaceConfig const&) mozilla-central/image/SurfaceFilters.h:823:25
    #7 0x7f01bb7ce746 in mozilla::Maybe<mozilla::image::SurfacePipe> mozilla::image::SurfacePipeFactory::MakePipe<mozilla::image::RemoveFrameRectConfig, mozilla::image::SurfaceConfig>(mozilla::image::RemoveFrameRectConfig const&, mozilla::image::SurfaceConfig const&) mozilla-central/image/SurfacePipeFactory.h:262:25
    #8 0x7f01bb7caf42 in mozilla::image::SurfacePipeFactory::CreateSurfacePipe(mozilla::image::Decoder*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::SurfaceFormat, mozilla::Maybe<mozilla::image::AnimationParams> const&, _qcms_transform*, mozilla::image::SurfacePipeFlags) mozilla-central/image/SurfacePipeFactory.h:241:20
    #9 0x7f01bb7bbcd3 in mozilla::image::nsGIFDecoder2::BeginImageFrame(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, unsigned short, bool) mozilla-central/image/decoders/nsGIFDecoder2.cpp:195:29
    #10 0x7f01bb7bce5f in mozilla::image::nsGIFDecoder2::FinishImageDescriptor(char const*) mozilla-central/image/decoders/nsGIFDecoder2.cpp:849:7
    #11 0x7f01bb7bcc84 in mozilla::image::nsGIFDecoder2::ReadImageDescriptor(char const*) mozilla-central/image/decoders/nsGIFDecoder2.cpp:744:12
    #12 0x7f01bb7c5e96 in mozilla::image::nsGIFDecoder2::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::$_1::operator()(mozilla::image::nsGIFDecoder2::State, char const*, unsigned long) const mozilla-central/image/decoders/nsGIFDecoder2.cpp:470:20
    #13 0x7f01bb7c5ccd in mozilla::Maybe<mozilla::Variant<mozilla::image::TerminalState, mozilla::image::Yield> > mozilla::image::StreamingLexer<mozilla::image::nsGIFDecoder2::State, 16ul>::BufferedRead<mozilla::image::nsGIFDecoder2::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::$_1>(mozilla::image::SourceBufferIterator&, mozilla::image::nsGIFDecoder2::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::$_1) /src/mozilla-central/image/StreamingLexer.h
    #14 0x7f01bb7bc427 in mozilla::Variant<mozilla::image::TerminalState, mozilla::image::Yield> mozilla::image::StreamingLexer<mozilla::image::nsGIFDecoder2::State, 16ul>::Lex<mozilla::image::nsGIFDecoder2::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::$_1>(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*, mozilla::image::nsGIFDecoder2::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::$_1) mozilla-central/image/StreamingLexer.h:469:26
    #15 0x7f01bb7bc1b7 in mozilla::image::nsGIFDecoder2::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*) mozilla-central/image/decoders/nsGIFDecoder2.cpp:445:17
    #16 0x7f01bb734be7 in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) mozilla-central/image/Decoder.cpp:133:19
    #17 0x7f01bb743304 in mozilla::image::AnonymousDecodingTask::Run() mozilla-central/image/IDecodingTask.cpp:178:36
    #18 0x7f01bb7574ad in mozilla::image::ImageOps::DecodeToSurface(mozilla::image::ImageOps::ImageBuffer*, nsTSubstring<char> const&, unsigned int, mozilla::Maybe<mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> > const&) mozilla-central/image/ImageOps.cpp:224:9
    #19 0x7f01bb757277 in mozilla::image::ImageOps::DecodeToSurface(already_AddRefed<nsIInputStream>, nsTSubstring<char> const&, unsigned int, mozilla::Maybe<mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> > const&) mozilla-central/image/ImageOps.cpp:196:10
    #20 0x7f01b9b42a7a in DecodeToSurfaceRunnableFuzzing::Go() mozilla-central/image/test/fuzzing/TestDecoders.cpp:54:16
    #21 0x7f01b9b429c8 in DecodeToSurfaceRunnableFuzzing::Run() mozilla-central/image/test/fuzzing/TestDecoders.cpp:49:5
    #22 0x7f01b9fb623a in nsThreadSyncDispatch::Run() mozilla-central/xpcom/threads/nsThreadSyncDispatch.h:35:51
    #23 0x7f01b9fa7f86 in nsThread::ProcessNextEvent(bool, bool*) mozilla-central/xpcom/threads/nsThread.cpp:1225:14
    #24 0x7f01b9fa99a6 in NS_ProcessNextEvent(nsIThread*, bool) mozilla-central/xpcom/threads/nsThreadUtils.cpp:486:10
    #25 0x7f01ba7590ba in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) mozilla-central/ipc/glue/MessagePump.cpp:303:20
    #26 0x7f01ba6b5db1 in MessageLoop::Run() mozilla-central/ipc/chromium/src/base/message_loop.cc:290:3
    #27 0x7f01b9fa62fe in nsThread::ThreadFunc(void*) mozilla-central/xpcom/threads/nsThread.cpp:458:11
    #28 0x7f01d141dc23 in _pt_root (/mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_firefox_008d6de92713224b5a96fd56953fbf4e5d586f6e/revisions/firefox/libnspr4.so+0x48c23)
    #29 0x7f01d10066b9 in start_thread (/mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_firefox_008d6de92713224b5a96fd56953fbf4e5d586f6e/revisions/lib/libpthread.so.0+0x76b9)
    #30 0x7f01d002e41c in clone (/mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_firefox_008d6de92713224b5a96fd56953fbf4e5d586f6e/revisions/lib/libc.so.6+0x10741c)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /src/mozilla-central/image/imgFrame.cpp:90:33 in

Comment 1

[Jessie [:jbonisteel] pls NI]

Does this look actionable?

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Yes, this is straightforward and a clear oversight (we do the right thing using CheckedInt elsewhere).

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

This should have been avoided by https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/image/SurfaceCache.cpp#1633.

Comment 4

[Timothy Nikkel (:tnikkel)]

The failing line is stride * width, but it seems like it should be stride * height.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1580317. Fix calculation of buffer size. r?aosmond

Comment 6

[Timothy Nikkel (:tnikkel)]

I don't think we need checkedint here or to do any checks that this doesn't over flow because every call site to this already checks that SurfaceCache::IsLegalSize passes. We do potentially add 3 here to the stride to make it 0 mod 4, but that should be fine because SurfaceCache::IsLegalSize checks widthheight4, and that is 0 mod 4, so if byte per pixel is less than 4 we can add 3 to the width and it still won't overflow.

Comment 7

[Timothy Nikkel (:tnikkel)]

I would guess this is DOS at most since the only decision this affects is where we allocate the memory, not how much.

Comment 8

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

This landed last Wednesday, sorry for the delay.
https://hg.mozilla.org/integration/autoland/rev/4975f8c1003b
https://hg.mozilla.org/mozilla-central/rev/4975f8c1003b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR68 approval when you get a chance.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9095256 [details]
Bug 1580317. Fix calculation of buffer size. r?aosmond

Beta/Release Uplift Approval Request

• User impact if declined: The user may get heap or volatile buffers used incorrectly, which may cause performance issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is trivial and well understood. We already are calculating the wrong size for determining whether or not we should use the heap. This corrects it. The choice to use the heap or not is purely performance related.
• String changes made/needed:

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9095256 [details]
Bug 1580317. Fix calculation of buffer size. r?aosmond

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: The user may get heap or volatile buffers used incorrectly, which may cause performance issues.
• Fix Landed on Version: 71.0
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is trivial and well understood. We already are calculating the wrong size for determining whether or not we should use the heap. This corrects it. The choice to use the heap or not is purely performance related.
• String or UUID changes made by this patch:

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9095256 [details]
Bug 1580317. Fix calculation of buffer size. r?aosmond

Fix for sec-high issue, let's take this for beta 12 (and ESR)

Comment 13

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

https://hg.mozilla.org/releases/mozilla-beta/rev/bfd2fa321e12

Comment 14

[Timothy Nikkel (:tnikkel)]

This should not be marked as sec-high, not sure what the procedure is for changing that, leaving the keyword alone in case there is some procedure.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/31549293df4b58aa025e343da54726de02bc2761

Comment 16

[Daniel Veditz [:dveditz]]

(In reply to Timothy Nikkel (:tnikkel) from comment #14)

This should not be marked as sec-high, not sure what the procedure is for changing that

security bug fixes are supposed to get sec-approval before landing (https://wiki.mozilla.org/Security/Bug_Approval_Process) so you could have asked as part of that process. Outside of that, for any kind of similar question you can ping folks in #security (irc, slack) or mail [e-mail] to ask someone to double-check and fix it.

(In reply to Timothy Nikkel (:tnikkel) from comment #4)

The failing line is stride * width, but it seems like it should be stride * height.

The patch here seems to be fixing a different bug (wrong value used in calculation) rather than the one originally reported (undefined behavior due to integer overflow). You'd need a different testcase but the integer overflow issue is still there. That's a problem for a couple of reasons: 1) we'll continue to get this UBSan warning, adding noise and slowing down fuzzing (if we crash here it may keep the fuzzer from getting deeper), and 2) the oss-fuzz project is still going to publish this issue and likely find and file a separate issue triggering the overflow using height instead of width.

Should we reopen this bug to track the issue oss-fuzz is going to disclose or open a new one for that?

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Daniel Veditz [:dveditz] from comment #16)

(In reply to Timothy Nikkel (:tnikkel) from comment #14)

This should not be marked as sec-high, not sure what the procedure is for changing that

security bug fixes are supposed to get sec-approval before landing (https://wiki.mozilla.org/Security/Bug_Approval_Process) so you could have asked as part of that process. Outside of that, for any kind of similar question you can ping folks in #security (irc, slack) or mail [e-mail] to ask someone to double-check and fix it.

Right, I know this. I didn't seek approval because this is not a security issue.

(In reply to Timothy Nikkel (:tnikkel) from comment #4)

The failing line is stride * width, but it seems like it should be stride * height.

The patch here seems to be fixing a different bug (wrong value used in calculation) rather than the one originally reported (undefined behavior due to integer overflow). You'd need a different testcase but the integer overflow issue is still there. That's a problem for a couple of reasons: 1) we'll continue to get this UBSan warning, adding noise and slowing down fuzzing (if we crash here it may keep the fuzzer from getting deeper), and 2) the oss-fuzz project is still going to publish this issue and likely find and file a separate issue triggering the overflow using height instead of width.

Should we reopen this bug to track the issue oss-fuzz is going to disclose or open a new one for that?

I covered this is comment 6 already. Short answer: we already check these values on every path into this function.

Comment 18

[Christoph Diehl [:posidron]]

If this will keep triggering the same UBSan error but with height instead and since it has been determined that this is not a security issue, then we could potentially blacklist it in the future except Timothy might have a better idea to not let this UBSan error pop up during future fuzzing runs and slow the process down.
This bug report was marked as fixed in oss-fuzz.

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Christoph Diehl [:posidron] from comment #18)

If this will keep triggering the same UBSan error but with height instead...

It should not. If you trigger another issue like this please file and cc me.

Comment 20

[Timothy Nikkel (:tnikkel)]

The problem was that height * height was overflowing. The calculation height * height is just wrong. Every path to the "height * height" calculation already checked that width * height didn't overflow (but that still means that height * height can overflow is height is very large and width is very small), so correcting the calculation to width * height should remove the possibility of overflow.

Comment 21

[Timothy Nikkel (:tnikkel)]

Sorry, the erroneous calculation was widthwidth, not heightheight.

Comment 22

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt