1517025 - Disallow % symbol in hostname for nsStandardURL

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev 83d06ab87e74.

Assertion failure: false (NS_SUCCEEDED(GetPrincipalFromOrigin(aOrigin, getter_AddRefs(dbgPrincipal)))), at /builds/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp:3221

Unfortunately we still do not have working symbols on debug builds so no minidump is available.

Comment 1

[Johann Hofmann [:johannh]]

Nika, can you take a quick look at this? I'm not sure if this is really actionable from the testcase alone.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Hmm - that may be caused by bug 1517014. It looks like the string which that test case is generating produces a string which doesn't round-trip back into a nsIPrincipal, which could be caused by some validation failing somewhere?

I don't think this should be causing any serious problems, but it would be nice to figure out what is happening with it.

Comment 3

[(no longer active)]

Bug 1517014 is completely innocent here.

The real problem here is that we parse the URL of the iframe as https://b%9a/ at https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/dom/base/nsFrameLoader.cpp#275.

There, src is u"/\b%9ª", encoding is encoding_rs::WINDOWS_1252_INIT, and base_uri is "https://bug1517025.bmoattachments.org/attachment.cgi?id=9033787". This causes us to load an iframe from origin https://b%9a/ but since the URI isn't valid UTF-8 we later on fail to parse the URI and the permission manager asserts because of that. Chrome seems to parse the original URI correctly here.

I'm pretty sure this is a URI parsing bug. Anne, what do you think?

Comment 4

[Anne (:annevk)]

This URL should fail to parse as % cannot be present in a host name (except when it's followed by two hex digits and can be decoded): https://url.spec.whatwg.org/#host-parsing. (The non-UTF-8 encoding only matters for the query component of a URL, by the way.)

(The reason it's treated as a host is because /\ becomes // which makes it a scheme-relative URL.)

Comment 5

[(no longer active)]

(In reply to Anne (:annevk) from comment #4)

(The reason it's treated as a host is because /\ becomes // which makes it a scheme-relative URL.)

Oh, fun. TIL!

Comment 6

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82cbb99b7f5e4e6cea7b3dd1760991af940a4a81

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1517025 - Do not allow percent symbol in URL hostnames r=kershaw!

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1517025 - Use mock API server so fetchSignedObjects doesn't fail because of BAD URL r=Gijs!

Depends on D16694

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1517025 - Remove EXPECTED-FAIL for URL percent encoding tests r=kershaw!

Depends on D16695

Comment 10

[(no longer active)]

Valentin, do you mind adding a crashtest based on the original test case attached to the bug as well? I think that would be useful to have too.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1517025 - Add crashtest with invalid URL r=kershaw!

Depends on D16696

Comment 12

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68cbd50bcebc118ff7adda4ee923a022ef70f111

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/419cb778d531
Do not allow percent symbol in URL hostnames r=kershaw
https://hg.mozilla.org/integration/autoland/rev/48c7d643d2fa
Change default pref URLs to localhost instead of %(server) r=Gijs
https://hg.mozilla.org/integration/autoland/rev/c29889dea969
Remove EXPECTED-FAIL for URL percent encoding tests r=kershaw
https://hg.mozilla.org/integration/autoland/rev/1e173178e49f
Add crashtest with invalid URL r=kershaw

Comment 14

[Eliza Balazs [:ebalazs_]]

Backed out 4 changesets (Bug 1517025) for multiple failures e.g.: toolkit/components/telemetry/tests/unit/test_PingAPI.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&classifiedState=unclassified&revision=1e173178e49f41ecb99f5179308c0591b960ef6f&selectedJob=223036442

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=223031965&repo=autoland&lineNumber=24354
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=223036442&repo=autoland&lineNumber=12564
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=223030817&repo=autoland&lineNumber=1757

Backout push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&classifiedState=unclassified&revision=22471aa9be27f7897495d4bb4b2d6fd1900e620b

Comment 15

[Valentin Gosu [:valentin] (he/him)]

14:20:15     INFO -  PID 10716 | JavaScript strict warning: jar:file:///Z:/task_1548078859/build/application/firefox/omni.ja!/components/nsSearchService.js, line 3807: ReferenceError: reference to undefined property "result"
14:20:15     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "result"" {file: "jar:file:///Z:/task_1548078859/build/application/firefox/omni.ja!/components/nsSearchService.js" line: 3807}]"
14:20:15  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryReportingPolicy.js | test_canSend - [test_canSend : 257] A promise chain failed to handle a rejection: Phase "OS.File: Waiting for clients before profileBeforeChange" is finished, it is too late to register completion condition "Search service: shutting down" - stack: addBlocker@resource://gre/modules/AsyncShutdown.jsm:675:15
14:20:15     INFO -  SRCH_SVC_addObservers@jar:file:///Z:/task_1548078859/build/application/firefox/omni.ja!/components/nsSearchService.js:4689:5
14:20:15     INFO -  _asyncInit@jar:file:///Z:/task_1548078859/build/application/firefox/omni.ja!/components/nsSearchService.js:2774:5
14:20:15     INFO -  async*task@jar:file:///Z:/task_1548078859/build/application/firefox/omni.ja!/components/nsSearchService.js:3804:17
14:20:15     INFO -  async*SRCH_SVC_init@jar:file:///Z:/task_1548078859/build/application/firefox/omni.ja!/components/nsSearchService.js:3801:23
14:20:15     INFO -  observe@resource://gre/modules/TelemetryEnvironment.jsm:1213:9
14:20:15     INFO -  test_firstRun@Z:/task_1548078859/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryReportingPolicy.js:76:3
14:20:15     INFO -  async*run_next_test/_run_next_test/<@Z:\\task_1548078859\\build\\tests\\xpcshell\\head.js:1436:22

According to the stack trace, this exception seems to be coming from here and the code is only expecting XPCOM error results here

Not sure this new behaviour is caused by my patch, but I'll look into it.

Comment 16

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=861cd91da4825a3a14cbf074c7cdc1596057ee14

Comment 17

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2355f186c28201e5d3e8b7668644421b26b4119a

Comment 18

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f8ac8fc1dba6afc90dacdb5b3a4c0a9adfc37ed

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b799d86aeff
Do not allow percent symbol in URL hostnames r=kershaw
https://hg.mozilla.org/integration/autoland/rev/c4c141df786b
Use mock API server so fetchSignedObjects doesn't fail because of BAD URL r=Gijs
https://hg.mozilla.org/integration/autoland/rev/41e7f49db5a6
Remove EXPECTED-FAIL for URL percent encoding tests r=kershaw

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2b799d86aeff
https://hg.mozilla.org/mozilla-central/rev/c4c141df786b
https://hg.mozilla.org/mozilla-central/rev/41e7f49db5a6

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Did you mean to re-land the crashtest?

Comment 22

[Valentin Gosu [:valentin] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

Did you mean to re-land the crashtest?

I filed bug 1527126 for that. It was triggering some other assertions on OSX.

Comment 24

[Phabricator Automation]

Comment on attachment 9037193 [details]
Bug 1517025 - Add crashtest with invalid URL r=kershaw!

Revision D16817 was moved to bug 1527126. Setting attachment 9037193 [details] to obsolete.