Disallow % symbol in hostname for nsStandardURL

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: jkratzer, Assigned: valentin)

Tracking

(Blocks 4 bugs, {assertion, testcase})

Trunk
mozilla67
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(5 attachments)

(Reporter)

Description

4 months ago
Posted file 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.
Flags: in-testsuite?
Component: Networking: Cookies → Permission Manager

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

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika) → needinfo?(ehsan)

Comment 3

3 months ago

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?

Flags: needinfo?(ehsan) → needinfo?(annevk)

Comment 4

3 months ago

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

Blocks: url
Flags: needinfo?(annevk) → needinfo?(valentin.gosu)

Comment 5

3 months ago

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

Component: Permission Manager → Networking
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Priority: -- → P2
Whiteboard: [necko-triaged]

Comment 10

3 months ago

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.

Attachment #9036907 - Attachment description: Bug 1517025 - Use mock API server so fetchSignedObjects doesn't fail because of BAD URL r=Gijs! → Bug 1517025 - Change default pref URLs to localhost instead of %(server) r=Gijs!

Comment 13

3 months ago
Pushed by valentin.gosu@gmail.com:
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
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.

Flags: needinfo?(valentin.gosu)
Attachment #9036907 - Attachment description: Bug 1517025 - Change default pref URLs to localhost instead of %(server) r=Gijs! → Bug 1517025 - Use mock API server so fetchSignedObjects doesn't fail because of BAD URL r=Gijs!

Comment 19

2 months ago
Pushed by valentin.gosu@gmail.com:
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

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Updated

2 months ago
Depends on: 1527236
Summary: Assertion failure: false (NS_SUCCEEDED(GetPrincipalFromOrigin(aOrigin, getter_AddRefs(dbgPrincipal)))), at /builds/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp:3221 → Disallow % symbol in hostname for nsStandardURL

Did you mean to re-land the crashtest?

Flags: needinfo?(valentin.gosu)

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

Flags: needinfo?(valentin.gosu)
Duplicate of this bug: 1311107
You need to log in before you can comment on or make changes to this bug.