Closed Bug 1334468 Opened 3 years ago Closed 3 years ago

Crash in mozilla::OriginAttributes::CreateSuffix when entering ',s."' in url bar with privacy.firstparty.isolate=true

Categories

(Core :: Security: CAPS, defect, critical)

51 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

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)
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)
Severity: normal → critical
Flags: needinfo?(florin.mezei)
Keywords: crash
The issue is with Nightly.
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.
Ok, i reproduce it too in that case.
Flags: needinfo?(florin.mezei)
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 ]
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 54 Branch → 51 Branch
CC'ing some people that do QA work on the First Party Isolation feature, including Kamil who's the QA owner.
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
Hi Tim and Jonathan,
Could you please have a look soon? Thank you!
Flags: needinfo?(tihuang)
Flags: needinfo?(jhao)
Peter, just out of curiosity.. did you enable FPI via the "privacy.firstparty.isolate" pref yourself?
(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.
(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.
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)
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: nobody → jhao
Status: NEW → ASSIGNED
Hi Olli, what do you think about this fix?
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-
Attachment #8832826 - Attachment is obsolete: true
Attachment #8833154 - Attachment is obsolete: true
Attachment #8833154 - Flags: review?(honzab.moz)
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)
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)
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)
Depends on: 1337629
Thanks, Valentin, for the information.  I opened bug 1337629 and assigned it to you.
Flags: needinfo?(honzab.moz)
(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.
Hi Patrick, would you please take a look at the patch?  Thanks.
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+
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5124565a51dc
A crashtest about restricted characters in URI. r=mcmanus
backed out for failing on own test like https://treeherder.mozilla.org/logviewer.html#?job_id=78782240&repo=autoland
Flags: needinfo?(jhao)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/180684f70381
Backed out changeset 5124565a51dc for failing on own tests
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/faa9045f4249
A crashtest about restricted characters in URI. r=mcmanus
https://hg.mozilla.org/mozilla-central/rev/faa9045f4249
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Might as well land this on Aurora too since bug 1337629 got uplifted there.
Flags: needinfo?(jhao) → in-testsuite+
Whiteboard: [checkin-needed-aurora]
Is there an alternative fix for this crash that'd be more upliftable to 52 for the ESR?
Flags: needinfo?(jhao)
(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.
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.
(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.
(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)
You need to log in before you can comment on or make changes to this bug.