Closed Bug 1556347 Opened 1 year ago Closed 6 months ago

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

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox69 --- wontfix

People

(Reporter: decoder, Assigned: decoder)

References

Details

(Keywords: crash, testcase, Whiteboard: [necko-triaged])

Attachments

(3 files, 2 obsolete files)

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

Attached file Testcase (obsolete) —
Attached file Testcase (obsolete) —
Attachment #9069320 - Attachment is obsolete: true

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

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

STR:

  1. nc -C -l 8000 < test4.txt
  2. navigate Firefox to localhost:8000
Attachment #9069321 - Attachment is obsolete: true
Attachment #9069335 - Attachment description: canned HTTP response → testcase (canned HTTP response)
Group: core-security → network-core-security
Keywords: sec-low

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

Priority: -- → P2
Whiteboard: [necko-triaged]

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?

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

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.

Assignee: nobody → choller
Group: network-core-security

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.

Keywords: sec-low

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

Keywords: leave-open
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ed264b9dc87
Set MOZ_HEADLESS for gtests and headless fuzzing. r=froydnj

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

Flags: needinfo?(choller)

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.

Flags: needinfo?(choller)
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/439344dd1379
Set MOZ_HEADLESS for gtests and headless fuzzing. r=froydnj

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.

Flags: needinfo?(fbraun)

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.

Flags: needinfo?(fbraun)

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

Flags: needinfo?(choller)

Comment 22 points out what needs to be done here.

Flags: needinfo?(choller)

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

Flags: needinfo?(fbraun)

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]

Flags: needinfo?(fbraun)

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

Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED

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?

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

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

You need to log in before you can comment on or make changes to this bug.