Closed Bug 1524992 Opened 1 year ago Closed 1 year ago

about:crashparent can be accessed via command line and via mail client links

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: hanno, Assigned: snorp)

References

Details

Attachments

(6 files)

One can call special urls like
about:crashparent
via the command line.

Many mail clients will open links in HTML mails directly with a browser, independent of the protocol. (Tested with evolution and claws-mail, doesn't work in thunderbird.)
Thus this means this can often be triggered by an email. So you can send someone an email and a click in the mail (via css this can be a link overlaying the whole mail) will lead to a browser crash.

(Looking at other browsers: Chrome has similar special URLs, but doesn't allow accessing them via the command line. If this is required for testing I recommend disabling command-line opening of about urls by default and put it behind a configuration flag.)

Related: #1507702

Snorp, can you please look into this?

Blocks: 1462702
Flags: needinfo?(snorp)

Patches up.

Flags: needinfo?(snorp)

Responded on phab.

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5da75d979ef
Treat command line URIs as external r=mconley
https://hg.mozilla.org/integration/autoland/rev/998c1f756e51
Don't trigger crash in about:crash* when opened from external apps r=mconley
Backout by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2785f1f139e8
Backed out 2 changesets for linting opt failure on a CLOSED TREE
Flags: needinfo?(snorp)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd215cd3c8d7
Treat command line URIs as external r=mconley
https://hg.mozilla.org/integration/autoland/rev/fb1d7e57e253
Don't trigger crash in about:crash* when opened from external apps r=mconley

This failed because the PGO build opens data:text/html,<script>Quitter.quit()</script> from the command line, which is unsurprisingly not allowed for external loads. I'm not quite sure how to proceed. Gijs, do you have suggestions?

Flags: needinfo?(snorp) → needinfo?(gijskruitbosch+bugs)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #12)

This failed because the PGO build opens data:text/html,<script>Quitter.quit()</script> from the command line, which is unsurprisingly not allowed for external loads. I'm not quite sure how to proceed. Gijs, do you have suggestions?

The addon that provides Quitter should provide its own (moz-extension) URL that does what the data URL does here, assuming that works. Or, even better, we should run the test against the test http server and request an https URL. That'll probably provide a more realistic PGO target in terms of optimizing when we load NSS (!)

:glandium or :ted, where does that code live?

Flags: needinfo?(ted)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(gijskruitbosch+bugs)

The code that launches the browser for the PGO profiling run is here. The source for the quitter extension is here, but note that due to it using an experiment API we have a signed copy of the XPI checked-in to the tree in that directory, which is what actually gets used in CI.

Flags: needinfo?(ted)

Yeah, updating the quitter extension is incredibly annoying because of the signing requirement. Maybe now is the time to move the PGO stuff to use Marionette? Ted, you thought this would be relatively easy in bug 1393449. Do we have anyone who could do this now? Maybe mshal?

Flags: needinfo?(ted)
Flags: needinfo?(mshal)
Flags: needinfo?(mh+mozilla)

So the first restart of Firefox doesn't have to be triggered from inside the application? I ask because you aren't using the in_app flag for restart().

Flags: needinfo?(snorp)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] (away 02/28 - 03/03) from comment #18)

So the first restart of Firefox doesn't have to be triggered from inside the application? I ask because you aren't using the in_app flag for restart().

No, I think it does need in_app. I'll update.

Flags: needinfo?(snorp)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0b9322c7a59
Treat command line URIs as external r=mconley
https://hg.mozilla.org/integration/autoland/rev/ae1e20a595c4
Don't trigger crash in about:crash* when opened from external apps r=mconley
https://hg.mozilla.org/integration/autoland/rev/face9d1e8868
Use Marionette for Linux profile runs r=mshal
Flags: needinfo?(mshal)
Flags: needinfo?(ted)

smaug, I'm not sure I really understand why data: URIs would not be allowed with LOAD_NORMAL_EXTERNAL. Do you know? Is it possible to relax this restriction without breaking a bunch of other stuff? The goal here is to disallow about:crashparent from the command line by marking command line loads as external, but it's breaking data:<script>Quitter.quit()</script> used by automation.

Flags: needinfo?(snorp) → needinfo?(bugs)

I don't know the answer from top of my head. Is this perhaps about bug 1401895?
ckerschb might recall data: handling better.

Flags: needinfo?(bugs) → needinfo?(ckerschb)

Hmm, that bug suggests data: from command line should already be blocked, but we're clearly using it in automation. Maybe we set a pref that allows it?

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #22)

smaug, I'm not sure I really understand why data: URIs would not be allowed with LOAD_NORMAL_EXTERNAL. Do you know? Is it possible to relax this restriction without breaking a bunch of other stuff? The goal here is to disallow about:crashparent from the command line by marking command line loads as external, but it's breaking data:<script>Quitter.quit()</script> used by automation.

FWIW, the relevant code for top-level data: URI blocking is here [1]. We want to block external data: URI loads to prevent phishing attacks - imagine you click a link from within thunderbird - in that case the GetLoadTriggeredFromExternal flag is set and we block the load.

There is a pref |security.data_uri.block_toplevel_data_uri_navigations| which you can flip in case you need to allow data: URI loading in our testsuite.

[1] https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#36

Flags: needinfo?(ckerschb)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)

There is a pref |security.data_uri.block_toplevel_data_uri_navigations| which you can flip in case you need to allow data: URI loading in our testsuite.

Would it be a good idea to have such a switch as well for about-URIs?

(In reply to Hanno Boeck from comment #26)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)

There is a pref |security.data_uri.block_toplevel_data_uri_navigations| which you can flip in case you need to allow data: URI loading in our testsuite.

Would it be a good idea to have such a switch as well for about-URIs?

No. The data: stuff is mostly about principal inheritance, spoofing, and all the fun ways that data URIs were "special"; "about:blank" is a perfectly valid URL to want to open from the commandline, and "about:home" and "about:newtab" and similar might also be passed in some situations (though maybe we mostly use explicit other flags for that today, I don't know off-hand).

Anyway, we can't ban about: on the commandline point-blank. We should just stop it for these specific URLs.

(In reply to :Gijs (he/him) from comment #27)

Anyway, we can't ban about: on the commandline point-blank. We should just stop it for these specific URLs.

Okay, my question was more related to the option, not to what exactly to ban.

I just think it's valuable to have a possibility to still run the "crashing" URLs from the commandline in testing situations while blocking that option for normal installs. (I have some tests personally that make use of it).

(In reply to Hanno Boeck from comment #28)

(In reply to :Gijs (he/him) from comment #27)

Anyway, we can't ban about: on the commandline point-blank. We should just stop it for these specific URLs.

Okay, my question was more related to the option, not to what exactly to ban.

I just think it's valuable to have a possibility to still run the "crashing" URLs from the commandline in testing situations while blocking that option for normal installs. (I have some tests personally that make use of it).

Oh, I see, sorry for misreading. I hadn't considered it from that angle. I guess that's up to :snorp...

Flags: needinfo?(snorp)

(In reply to :Gijs (he/him) from comment #29)

(In reply to Hanno Boeck from comment #28)

(In reply to :Gijs (he/him) from comment #27)

Anyway, we can't ban about: on the commandline point-blank. We should just stop it for these specific URLs.

Okay, my question was more related to the option, not to what exactly to ban.

I just think it's valuable to have a possibility to still run the "crashing" URLs from the commandline in testing situations while blocking that option for normal installs. (I have some tests personally that make use of it).

Oh, I see, sorry for misreading. I hadn't considered it from that angle. I guess that's up to :snorp...

I'm willing to do whatever. I figured treating command line urls as external (LOAD_FLAGS_FROM_EXTERNAL), then not (optionally?) not actually crashing in about:crash* would be the way to go, but maybe I'm barking up the wrong tree? If I want something more specific, should I just filter those urls out in browser.js?

Flags: needinfo?(snorp)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #22)

smaug, I'm not sure I really understand why data: URIs would not be allowed with LOAD_NORMAL_EXTERNAL. Do you know? Is it possible to relax this restriction without breaking a bunch of other stuff? The goal here is to disallow about:crashparent from the command line by marking command line loads as external, but it's breaking data:<script>Quitter.quit()</script> used by automation.

FWIW, the relevant code for top-level data: URI blocking is here [1]. We want to block external data: URI loads to prevent phishing attacks - imagine you click a link from within thunderbird - in that case the GetLoadTriggeredFromExternal flag is set and we block the load.

There is a pref |security.data_uri.block_toplevel_data_uri_navigations| which you can flip in case you need to allow data: URI loading in our testsuite.

[1] https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#36

It appears then that this is not working as expected, because we're successfully using data: from the command line in testing without setting any special pref (AFAICT). This makes sense, as GetLoadTriggeredFromExternal() will return false without my patch from this bug. It seems like the solution might be to just set the pref to allow this in the test harness that need it? I'll give that a try.

We use this to tell the Quitter extension to quit in the PGO
profile generation and Valgrind tests.

We use this to tell the Quitter extension to quit in the PGO
profile generation and Valgrind tests.

Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e239591d8bf7
Allow external data: URIs in tests r=ckerschb
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24b03e0b300b
Allow external data: URIs in tests r=ckerschb
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51fd0edf687f
Followup to add newline r=mbrubeck,rbarker
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7e2fa30d85f
Treat command line URIs as external r=mconley
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f47a4e7c43f
Don't trigger crash in about:crash* when opened from external apps r=mconley
You need to log in before you can comment on or make changes to this bug.