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

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Hanno Boeck]

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

Comment 1

[Johann Hofmann [:johannh]]

Snorp, can you please look into this?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1524992 - Treat command line URIs as external r?mconley

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1524992 - Don't trigger crash in about:crash* when opened from external apps r?mconley

Depends on D20890

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Patches up.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

ni?ing Gijs for https://phabricator.services.mozilla.com/D20890#566736

Comment 6

[:Gijs (he/him)]

Responded on phab.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2785f1f139e8
Backed out 2 changesets for linting opt failure on a CLOSED TREE

Comment 9

[Daniel Varga [:dvarga]]

Backed out 2 changesets (Bug 1524992) for linting opt failure on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=998c1f756e51d9c8c983cf0585971aef40162f63

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=230776458&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/2785f1f139e8018efae1426f19af1671f52ee550

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Cristina Coroiu [:ccoroiu]]

Backed out 2 changesets (bug 1524992) for multiple failures and timeout

Backout: https://hg.mozilla.org/integration/autoland/rev/5943b02bfa38cc5a3724766d0f7e896ade3eb643

Failure push:https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Cmochitests%2Cwith%2Ce10s%2Ctest-android-em-7.0-x86_64%2Fdebug-mochitest-e10s-2%2Cm-e10s%282%29&revision=fb1d7e57e253c6922a51d436dc0796e8411b24eb

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

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

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 13

[:Gijs (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) (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?

Comment 14

[(not currently active) Ted Mielczarek]

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.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

I got a wip for using marionette in profileserver.py here: https://hg.mozilla.org/try/rev/2b675268913a36262ecb76004dd795208251e6df

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1524992 - Use Marionette for Linux profile runs r?mshal

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Andreea Pavel [:apavel]]

Backed out for failing valgrind

Push with failures https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=231744705&revision=face9d1e886872033b70fbdff5a40eaa244db3fb

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

Backout: https://hg.mozilla.org/integration/autoland/rev/88ffdaa924cd625629d50fdbc97923ee9ee42b0f

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

(In reply to James Willcox (:snorp) ([e-mail]) (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

Comment 26

[Hanno Boeck]

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

Comment 27

[:Gijs (he/him)]

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

Comment 28

[Hanno Boeck]

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

Comment 29

[:Gijs (he/him)]

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

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

(In reply to James Willcox (:snorp) ([e-mail]) (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.

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1524992 - Allow external data: URIs in tests r?ckerschb

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

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1524992 - Allow external data: URIs in tests r?ckerschb

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

Comment 34

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1524992 - Followup to add newline

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e239591d8bf7
Allow external data: URIs in tests r=ckerschb

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24b03e0b300b
Allow external data: URIs in tests r=ckerschb

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51fd0edf687f
Followup to add newline r=mbrubeck,rbarker

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7e2fa30d85f
Treat command line URIs as external r=mconley

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f47a4e7c43f
Don't trigger crash in about:crash* when opened from external apps r=mconley

Comment 40

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e239591d8bf7
https://hg.mozilla.org/mozilla-central/rev/24b03e0b300b
https://hg.mozilla.org/mozilla-central/rev/51fd0edf687f
https://hg.mozilla.org/mozilla-central/rev/d7e2fa30d85f
https://hg.mozilla.org/mozilla-central/rev/7f47a4e7c43f