Closed Bug 1580317 Opened 1 year ago Closed 1 year ago

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

Categories

(Core :: ImageLib, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 70+ fixed
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 + fixed
firefox71 + fixed

People

(Reporter: posidron, Assigned: tnikkel)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: csectype-intoverflow, oss-fuzz, sec-other, Whiteboard: [disclosure deadline 2019-12-09][adv-main70-][adv-esr68.2-][post-critsmash-triage])

Attachments

(3 files)

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
Group: core-security → gfx-core-security
Blocks: ubsan
Whiteboard: [disclosure deadline 2019-12-09]

Does this look actionable?

Flags: needinfo?(aosmond)

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

Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Flags: needinfo?(aosmond)
Priority: -- → P2

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

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.

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

Regressed by: 1427639
Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

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

Assignee: aosmond → tnikkel

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:
Flags: needinfo?(aosmond)
Attachment #9095256 - Flags: approval-mozilla-beta?

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:
Attachment #9095256 - Flags: approval-mozilla-esr68?

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)

Attachment #9095256 - Flags: approval-mozilla-esr68?
Attachment #9095256 - Flags: approval-mozilla-esr68+
Attachment #9095256 - Flags: approval-mozilla-beta?
Attachment #9095256 - Flags: approval-mozilla-beta+

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.

(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 security@mozilla.org 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?

Flags: needinfo?(tnikkel)
Flags: needinfo?(cdiehl)
Keywords: sec-highsec-other

(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 security@mozilla.org 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.

Flags: needinfo?(tnikkel)

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.

Flags: needinfo?(cdiehl)

(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.

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.

Sorry, the erroneous calculation was widthwidth, not heightheight.

Whiteboard: [disclosure deadline 2019-12-09] → [disclosure deadline 2019-12-09][adv-main70+]
Whiteboard: [disclosure deadline 2019-12-09][adv-main70+] → [disclosure deadline 2019-12-09][adv-main70+][adv-esr68.2+]
Flags: qe-verify-
Whiteboard: [disclosure deadline 2019-12-09][adv-main70+][adv-esr68.2+] → [disclosure deadline 2019-12-09][adv-main70+][adv-esr68.2+][post-critsmash-triage]
Whiteboard: [disclosure deadline 2019-12-09][adv-main70+][adv-esr68.2+][post-critsmash-triage] → [disclosure deadline 2019-12-09][adv-main70-][adv-esr68.2-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.