Closed
Bug 1334468
Opened 7 years ago
Closed 7 years ago
Crash in mozilla::OriginAttributes::CreateSuffix when entering ',s."' in url bar with privacy.firstparty.isolate=true
Categories
(Core :: Security: CAPS, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: pege, Assigned: jhao)
References
(Blocks 1 open bug)
Details
(Keywords: crash)
Crash Data
Attachments
(1 file, 2 obsolete files)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 Build ID: 20170127110036 Steps to reproduce: I entered ',sharebutton.io"' in the URL bar (the string contained in the single quotes) Actual results: The tab crashed reproducibly, "Gah. Your tab just crashed." Expected results: Not exactly sure what should happen when you type it; wanted to enter "sharebutton.io" in order to search for it but mistyped. I'm pretty sure though the tab shouldn't crash. I tried to reduce the entered string, removing parts that don't appear to cause the crash. ',s."' is the shortest form I could come up that still causes a crash.
I can't reproducethe crash with FF54 on Win 7. Do you have some crash reports in about:crashes? Does it crash in safe mode too? https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode
Flags: needinfo?(peter)
Reporter | ||
Comment 2•7 years ago
|
||
Didn't have a crash in about:crashes. When I tried safe mode, and it crashed again, it created one though. Crash report: https://crash-stats.mozilla.com/report/index/ec9e282c-22a1-4b0d-980c-1df922170127
Flags: needinfo?(peter)
Florin, could you find someone from QA with Linux to test this issue, please. (I don't have Linux)
Reporter | ||
Comment 5•7 years ago
|
||
Took a closer look at my setup, it turns out that the crash only occurs if the preference privacy.firstparty.isolate is set to true.
CR FF54: https://crash-stats.mozilla.com/report/index/475f8e39-a8f6-45eb-8622-f22ff2170128 All versions are crashing.
Crash Signature: [@ mozilla::OriginAttributes::CreateSuffix ]
[@ libxul.so@0xed04ca | IPC::detail::OriginAttributesParamTraits<T>::Write ]
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Component: Untriaged → Security: CAPS
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Summary: Tab crash when entering ',s."' in url bar → Crash in mozilla::OriginAttributes::CreateSuffix when entering ',s."' in url bar with privacy.firstparty.isolate=true
Blocks: FirstPartyIsolation
Version: 54 Branch → 51 Branch
Comment 8•7 years ago
|
||
CC'ing some people that do QA work on the First Party Isolation feature, including Kamil who's the QA owner.
Comment 9•7 years ago
|
||
I was able to reproduce this crash on Ubuntu 16.04 64-bit on all Fx Versions (51, 52, 53 and 54 - all latest builds): - Firefox 51.0.1 - bp-d1ad72c3-459a-4776-87dd-7bdd02170130 - Firefox 52 beta 1 - bp-fd0a7b90-df78-4df2-92e9-6e4ec2170130 - latest Developer Edition 53.0a2 - bp-086a4e0f-a7ad-471e-b041-4d1b52170130 - latest Nightly 54.0a1 - bp-950161bd-bf4f-4b36-bbeb-e5c912170130 Also I reproduced on macOS 10.12.2 and Windows 10 64-bit as well: macOS 10.12.2: - bp-c2aa70c4-2850-4172-a932-9ae1b2170130 Windows 10 64-bit: - bp-51a847ab-915d-43bb-a111-930872170130
Comment 10•7 years ago
|
||
Hi Tim and Jonathan, Could you please have a look soon? Thank you!
Flags: needinfo?(tihuang)
Flags: needinfo?(jhao)
Comment 11•7 years ago
|
||
Peter, just out of curiosity.. did you enable FPI via the "privacy.firstparty.isolate" pref yourself?
Reporter | ||
Comment 12•7 years ago
|
||
(In reply to Kamil Jozwiak [:kjozwiak] from comment #11) > Peter, just out of curiosity.. did you enable FPI via the > "privacy.firstparty.isolate" pref yourself? Yes, enabled it myself. I usually use Tor Browser but wanted to test out the feature in Firefox now that it has been uplifted.
Comment 13•7 years ago
|
||
(In reply to peter from comment #12) > Yes, enabled it myself. I usually use Tor Browser but wanted to test out the > feature in Firefox now that it has been uplifted. Thanks :) I just wanted to make sure this wasn't turned on by default by some random add-on without your knowledge, as it does break portions of the web.
Comment 14•7 years ago
|
||
This crash happens here [1]. Actually, it is because of the '"' character in the URL bar, which is an illegal character for a file path. We don't allow illegal characters here since originAttributes will be used as a part of a file name in some storage sub-systems, like indexedDB. However, the '"' character is not a valid domain character. Maybe we could just ignore these illegal characters when creating a first party domain, and change this release assertion to a regular assertion, which only crashes in debug mode. [1] http://searchfox.org/mozilla-central/source/caps/BasePrincipal.cpp#119
Flags: needinfo?(tihuang)
Assignee | ||
Comment 15•7 years ago
|
||
I would suggest that when we get a URL with illegal characters, we let the search engine handles the query instead of actually tring to navigate the page by that URL.
Flags: needinfo?(jhao)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jhao
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
Hi Olli, what do you think about this fix?
Assignee | ||
Comment 18•7 years ago
|
||
This would affect the UnifiedComplete module. It makes _matchedUnknownUrl() return false [1], and thus results in _matchCurrentSearchEngine() being executed [2]. [1] http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/toolkit/components/places/UnifiedComplete.js#1403 [2] http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/toolkit/components/places/UnifiedComplete.js#1076
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8832826 [details] Bug 1334468 - Check if the hostname has illegal characters in FixupURIProtocol(). https://reviewboard.mozilla.org/r/109080/#review110266 This doesn't help if the web page does something like window.location.hostname = ',sharebutton.io"' And tests are needed, whatever we do here.
Attachment #8832826 -
Flags: review?(bugs) → review-
Assignee | ||
Updated•7 years ago
|
Attachment #8832826 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8833154 -
Attachment is obsolete: true
Attachment #8833154 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 21•7 years ago
|
||
The previous patch still didn't take care of the case smaug mentioned, so I deleted that. We might need to eliminate illegal characters in nsStandardURL. In any case, I'd like to confirm with Honza first. Is it OK to ban the characters "/:*?\"<>|\\" in URL hosts?
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 22•7 years ago
|
||
Hi Valentin, Do you think we can forbid characters "/:*?\"<>|\\" in URL? Or is there a better way to solve this bug?
Flags: needinfo?(valentin.gosu)
Comment 23•7 years ago
|
||
I think we can. There is already an interest in reducing the number of allowed hostname characters [1]. Forbidding these ones is a good step in that direction. Please file another bug and assign it to me. [1] https://github.com/whatwg/url/issues/159#issuecomment-270400298
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 24•7 years ago
|
||
Thanks, Valentin, for the information. I opened bug 1337629 and assigned it to you.
Flags: needinfo?(honzab.moz)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to Jonathan Hao [:jhao] from comment #25) > Created attachment 8835841 [details] > Bug 1334468 - A crashtest about restricted characters in URI. > > Review commit: https://reviewboard.mozilla.org/r/111416/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/111416/ This is a crashtest which would crash before bug 1337629 was done, but now wouldn't.
Assignee | ||
Comment 27•7 years ago
|
||
Hi Patrick, would you please take a look at the patch? Thanks.
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8835841 [details] Bug 1334468 - A crashtest about restricted characters in URI. https://reviewboard.mozilla.org/r/111416/#review112952 tests welcome! thanks.
Attachment #8835841 -
Flags: review?(mcmanus) → review+
Comment 29•7 years ago
|
||
Pushed by jhao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5124565a51dc A crashtest about restricted characters in URI. r=mcmanus
Comment 30•7 years ago
|
||
backed out for failing on own test like https://treeherder.mozilla.org/logviewer.html#?job_id=78782240&repo=autoland
Flags: needinfo?(jhao)
Comment 31•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/180684f70381 Backed out changeset 5124565a51dc for failing on own tests
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•7 years ago
|
||
Pushed by jhao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/faa9045f4249 A crashtest about restricted characters in URI. r=mcmanus
Comment 35•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/faa9045f4249
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 36•7 years ago
|
||
Might as well land this on Aurora too since bug 1337629 got uplifted there.
Flags: needinfo?(jhao) → in-testsuite+
Whiteboard: [checkin-needed-aurora]
Comment 37•7 years ago
|
||
Is there an alternative fix for this crash that'd be more upliftable to 52 for the ESR?
Flags: needinfo?(jhao)
Comment 38•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/2bbdf69d4cb3
Whiteboard: [checkin-needed-aurora]
Comment 39•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #37) > Is there an alternative fix for this crash that'd be more upliftable to 52 > for the ESR? The real fix of this crash is in bug 1337629. (The patch of this bug only verifies that fix.) We should consider uplifting the patch of bug 1337629.
Comment 40•7 years ago
|
||
I'm well aware of that. Bug 1337629 also got wontfixed for 52 because the patch was deemed too risky for 52 this late in the cycle. Hence my asking if there was a lower-risk crash fix we could take instead.
Comment 41•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #40) > I'm well aware of that. Bug 1337629 also got wontfixed for 52 because the > patch was deemed too risky for 52 this late in the cycle. Hence my asking if > there was a lower-risk crash fix we could take instead. Okay, thanks for the clarification. Jonathan has an idea to fix this crash while not breaking addons which are using newURI(). Leave the NI to Jonathan to see if he can make a patch quickly.
Assignee | ||
Comment 42•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #37) > Is there an alternative fix for this crash that'd be more upliftable to 52 > for the ESR? I do have an alternative fix, but it's not completely riskless if we just rushed to get it reviewed and uplifted this late in the cycle. From my point of view, the fix in Bug 1337629 is not only more straightforward but also less risky because it has been in Nightly for 13 days and in Aurora for 7 days. There was only one addon breakage reported, but the owner of that addon quickly fixed that.
Flags: needinfo?(jhao)
Comment 43•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/dd602dd794e8
Comment 44•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/dd602dd794e8
status-firefox-esr52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•