1556347 - Crash in GTK through [@ ensure_stock_image_widget] by redirecting to moz-icon protocol

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20190602-afb588ff0931 .

For detailed crash information, see attachment.

To reproduce the issue, perform the following steps:

1. Download the attached testcase, save as "test.bin".
   2a. Build with --enable-fuzzing (requires Clang and ASan, also build gtests using ./mach gtest dontruntests).
   2b. Alternatively you can download builds from TC using python -mfuzzfetch -a --fuzzing --tests gtest (see https://github.com/MozillaSecurity/fuzzfetch).
2. Run MOZ_RUN_GTEST=1 LIBFUZZER=1 FUZZER=NetworkHttp objdir/dist/bin/firefox test.bin

Dragana mentioned we should file this as s-s because it is unclear if we should allow redirecting to moz-icon in the first place and what the security implications might be (the crash itself is a NULL deref but there might be other implications).

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 4

[:Gijs (he/him)]

(In reply to Christian Holler (:decoder) from comment #0)

Dragana mentioned we should file this as s-s because it is unclear if we should allow redirecting to moz-icon in the first place and what the security implications might be (the crash itself is a NULL deref but there might be other implications).

We should not, see bug 1222924. From the stack, it looks like we can crash in moz-icon's channel::init which does non-trivial work, whereas I suspect (not 100% sure) that the security check which would fail would come from asyncopen?

Comment 5

[Frederik Braun [:freddy]]

Confirmed. Loading a simplified test case prints a Gtk-Warning on the console

(firefox:14863): Gtk-WARNING **: 13:58:40.132: Error loading theme icon '//moz-icon:/UTF-8/' for stock: Symbol »//moz-icon:/UTF-8/« does not exist in theme

Comment 6

[Frederik Braun [:freddy]]

Attachment: testcase (canned HTTP response)

STR:

1. nc -C -l 8000 < test4.txt
2. navigate Firefox to localhost:8000

Comment 7

[Honza Bambas (:mayhemer)]

P2 as this is not a new regression. Please increase if you feel this should be addressed sooner.

Comment 8

[Frederik Braun [:freddy]]

Honza: I'm fearful of the implications for PUT or POST requests, as well as path traversal as we've had problems with these issues in the past (thinking of bug 1243178 and bug 1432870). But this is uninformed talking. Would be great if you could either give me some pointers for further reading to help me understand this space better - maybe you've been thinking about these cases as well?

Comment 9

[Honza Bambas (:mayhemer)]

My comment 7 is there from the POV of a mere triager. I will discuss this issue with others at our regular networking meetings.

Comment 10

[Honza Bambas (:mayhemer)]

Crashing here, for ref:
https://searchfox.org/mozilla-central/rev/227f5329f75bd8b16c6b146a7414598a420260cb/image/decoders/icon/gtk/nsIconChannel.cpp#119

Comment 11

[Honza Bambas (:mayhemer)]

The conclusion:

• this is a bug in nsIconChannel where the crash originates
• I would first fix this crash (find its actual cause)
• if we then allow the redirect to happen, then it's another bug to file and fix separately (I don't know from the top of my head if redirect load checks will block it or not)
• if there is a possible security problem in nsIconChannel::Init (which just initialize the channel - but does not start the load, or at least should not!) then it's possibly a third bug to fix

Note that loading a url that redirects to the provided moz-icon url on a linux (ubuntu) machine gets me Corrupted Content Error
Note that loading the moz-icon url directly in the address bar shows an actual icon (loads)

So, that indicates that if we fix the crash we are then likely safe.

Comment 12

[Christian Holler (:decoder)]

Attachment: Bug 1556347 - Set MOZ_HEADLESS for gtests and headless fuzzing. r?froydnj

Comment 13

[Christian Holler (:decoder)]

Not a sec bug. The problem here is that neither headless fuzzing nor gtests set MOZ_HEADLESS, which is guarding code that requires e.g. a GTK window.

Comment 14

[Frederik Braun [:freddy]]

Setting leave-open as decoder's patch (comment 12) is not fixing the GTK warning (which I observed in non-headless).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ed264b9dc87
Set MOZ_HEADLESS for gtests and headless fuzzing. r=froydnj

Comment 16

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 3ed264b9dc87 (bug 1556347) for Gtest failures at /build/src/toolkit/xre/nsAppRunner.cpp

Backout: https://hg.mozilla.org/integration/autoland/rev/8f20684a3fcd5c73f94bca0506182564a9cfb41f

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3ed264b9dc879feabccc1699ba05b3a87ee84bf8

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=265942036&repo=autoland&lineNumber=1427

[task 2019-09-10T15:59:22.407Z] 15:59:22 INFO - Calling ['/builds/worker/workspace/build/venv/bin/python', '-u', '/builds/worker/workspace/build/tests/gtest/rungtests.py', '--enable-webrender', '--xre-path=/builds/worker/workspace/build/application/firefox', '--cwd=/builds/worker/workspace/build/tests/gtest', '--symbols-path=/builds/worker/workspace/build/symbols', '--utility-path=tests/bin', '/builds/worker/workspace/build/application/firefox/firefox'] with output_timeout 1000
[task 2019-09-10T15:59:22.660Z] 15:59:22 INFO - gtest INFO | Running gtest
[task 2019-09-10T15:59:22.728Z] 15:59:22 INFO - [1094, Unnamed thread 7fac1a4925e0] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 198
[task 2019-09-10T15:59:22.729Z] 15:59:22 INFO - [1094, Unnamed thread 7fac1a4925e0] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 198
[task 2019-09-10T15:59:22.729Z] 15:59:22 INFO - [1094, Unnamed thread 7fac1a4925e0] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 198
[task 2019-09-10T15:59:22.729Z] 15:59:22 INFO - Assertion failure: gfxPlatform::IsHeadless(), at /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:3772
[task 2019-09-10T15:59:46.975Z] 15:59:46 INFO - #01: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:4715]
[task 2019-09-10T15:59:46.975Z] 15:59:46 INFO - #02: XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:4809]
[task 2019-09-10T15:59:46.975Z] 15:59:46 INFO - #03: _fini
[task 2019-09-10T15:59:46.976Z] 15:59:46 INFO - #04: libc.so.6 + 0x20830
[task 2019-09-10T15:59:46.976Z] 15:59:46 INFO - #05: _fini
[task 2019-09-10T15:59:46.976Z] 15:59:46 INFO - ExceptionHandler::GenerateDump cloned child 1102
[task 2019-09-10T15:59:46.976Z] 15:59:46 INFO - ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2019-09-10T15:59:46.977Z] 15:59:46 INFO - ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2019-09-10T15:59:47.000Z] 15:59:47 ERROR - gtest TEST-UNEXPECTED-FAIL | gtest | test failed with return code -11
[task 2019-09-10T15:59:50.784Z] 15:59:50 ERROR - Return code: 1
[task 2019-09-10T15:59:50.784Z] 15:59:50 ERROR - No tests run or test summary not found
[task 2019-09-10T15:59:50.784Z] 15:59:50 INFO - TinderboxPrint: gtest-gtest<br/><em class="testfail">T-FAIL</em>
[task 2019-09-10T15:59:50.784Z] 15:59:50 ERROR - # TBPL FAILURE #
[task 2019-09-10T15:59:50.784Z] 15:59:50 WARNING - setting return code to 2
[task 2019-09-10T15:59:50.784Z] 15:59:50 ERROR - The gtest suite: gtest ran with return status: FAILURE
[task 2019-09-10T15:59:50.785Z] 15:59:50 INFO - Running post-action listener: _package_coverage_data
[task 2019-09-10T15:59:50.785Z] 15:59:50 INFO - Running post-action listener: _resource_record_post_action
[task 2019-09-10T15:59:50.785Z] 15:59:50 INFO - Running post-action listener: process_java_coverage_data
[task 2019-09-10T15:59:50.785Z] 15:59:50 INFO - [mozharness: 2019-09-10 15:59:50.785756Z] Finished run-tests step (success)
[task 2019-09-10T15:59:50.786Z] 15:59:50 INFO - Running post-run listener: _resource_record_post_run

Comment 17

[Christian Holler (:decoder)]

I can't reproduce this locally but the assertion failing here is the one I added. It is plausible that there is a code path in some configuration that calls gfxPlatform::IsHeadless() before we call our setenv. That's in fact the reason why I added the assertion, so yay.

I will try to move the setenv to be really early in XRE_mainInit.

Comment 18

[Razvan Maries]

This push also caused exception failures like: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Ctest-android-em-7.0-x86_64%2Fdebug-geckoview-gtest-1proc%2C%28gtest%29&revision=3ed264b9dc879feabccc1699ba05b3a87ee84bf8&selectedJob=265967078

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/439344dd1379
Set MOZ_HEADLESS for gtests and headless fuzzing. r=froydnj

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/439344dd1379

Comment 21

[Christian Holler (:decoder)]

Freddy, can you please state what else needs to be done in this bug? Depending on what action points are identified here, it might warrant a new bug.

Comment 22

[Frederik Braun [:freddy]]

We shouldn't allow redirects to moz-icon:// in the first place. This should be stopped much, much sooner than in nsIconChannel.
It is likely there are other protocols which we disallow from loading in frames (and top-level navigation) that might actually work through a redirection.

Comment 23

[BugBot [:suhaib / :marco]]

The leave-open keyword is there and there is no activity for 6 months.
:decoder, maybe it's time to close this bug?

Comment 24

[Christian Holler (:decoder)]

Comment 22 points out what needs to be done here.

Comment 25

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddy] from comment #22)

We shouldn't allow redirects to moz-icon:// in the first place. This should be stopped much, much sooner than in nsIconChannel.
It is likely there are other protocols which we disallow from loading in frames (and top-level navigation) that might actually work through a redirection.

What did you have in mind? As noted in comment 4 / comment 5, we do already prevent redirects, but those will still run Init().

Comment 26

[Frederik Braun [:freddy]]

I don't recall from what I meant 6 months ago :(
Honza's first step in comment 11 seems most reasonble. We should figure out how not to trigger the gtk warning and crash in [@ ensure_stock_image_widget]

Comment 27

[Christian Holler (:decoder)]

(In reply to Frederik Braun [:freddy] from comment #26)

I don't recall from what I meant 6 months ago :(
Honza's first step in comment 11 seems most reasonble. We should figure out how not to trigger the gtk warning and crash in [@ ensure_stock_image_widget]

I have already fixed those things, see comment 20. I'm going to close this as fixed now if there is nothing more to be done.

Comment 28

[Frederik Braun [:freddy]]

I thought what you did was that we do not hit the bug when fuzzing. The Gtk warning is still present for actual non-headless mode, isn't it?

Comment 29

[Christian Holler (:decoder)]

(In reply to Frederik Braun [:freddy] from comment #28)

I thought what you did was that we do not hit the bug when fuzzing. The Gtk Error is still present for actual non-headless mode, isn't it?

I'm not sure, I never managed to reproduce this outside fuzzing. If you can still reproduce this somehow with the full browser in a non-headless scenario, then maybe we should reopen and fix it.

Comment 30

[Frederik Braun [:freddy]]

I can still get the Gtk Warning in normal nightly builds with the test case from comment 6, but no crash.