1900104 - URL parsing changes allowing OriginAttributes spoofing

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

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

In bug 1873973, we changed the parsing in nsStandardURL such that the ^ character is no longer escaped within paths. This unfortunately opens a security hole in our OriginAttributes infrastructure, which could potentially allow someone to spoof origin attributes.

In bug 1172080, we changed the separating character in origin strings from the ! character to the ^ character because the ! character was not guaranteed to be escaped by nsStandardURL, leading to a potential valid URL appearing to have originattributes present. Now that we do not escape ^ in file paths this same bug can occur again but with the ^ character, allowing evil paths like (e.g. file:///tmp/foo^userContextId=1).

See also the comment https://searchfox.org/mozilla-central/rev/5ff3324fe989a19c6fa5ac5b923089ef4ce2ebb2/caps/ContentPrincipal.cpp#176-182 which has extra comments about the escaping requirements on Networking URLs.

This situation is potentially even worse for Thunderbird, as they support ORIGIN_IS_FULL_SPEC, which means that more URI types include the full path within their originNoSuffix definition.

Unfortunately, changing the separating character (as we did last time) or making it mandatory for all origin serializations will be quite difficult, as these origin strings are serialized into the profile in a large number of places.

Comment 1

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

Bug 1873973 has now been backed out, which should fix this regression.

Leaving a ni? for :valentin so we can consider what to do about the original issue.

We also probably want to land some new tests which explicitly check that the ^ character is escaped in file:/// URI paths and such, with some documentation of the security invariants which that escaping is preserving, so that we don't land a patch like that again without fixing origins and origin attributes first.

Comment 2

[Valentin Gosu [:valentin]]

Thank you for letting me know about file URLs. I didn't know the full path is included in the origin.
I was considering if we could allow ^ just for non-file URLs, but I don't know if it's worth the risk - if someone could change the scheme of the URL they could get around those restrictions.

(In reply to Nika Layzell [:nika] (ni? for response) from comment #0)

Unfortunately, changing the separating character (as we did last time) or making it mandatory for all origin serializations will be quite difficult, as these origin strings are serialized into the profile in a large number of places.

I guess we can put off this change for a while until we figure out either a migration path to a new separator character, or something better.
Let's use this bug to land some tests for the file origin with ^.
Not sure if it's still sec-high since Bug 1873973 was backed out.

Comment 3

[Frederik Braun [:freddy]]

Agreed. People get sad with "high" severity bugs on their team dashboards. Thank you Nika for finding out and everyone else for their support in getting this backed out and keeping the conversation going.

As a next step, I suggest we identify an unused character (sequence). Throwing some out here..

• The spec parser removes all leading/trailing "C0 controls" which are U+0000 NULL to U+001F INFORMATION SEPARATOR ONE, inclusive.
• Additionally, one could use "%%" or other invalid percent encodings.

This theoretical spec reading would need to be tested against real life, of course.

Other ideas:

• Make originAttributes nested URLs. Some sort of originAttributes-Scheme
• Make originAttributes use URL query parameter but with a super obscure prefix. e.g., originattribute1=value1 would be http://example.com/?~oa~originattribute1=value1

Comment 4

[Valentin Gosu [:valentin]]

I think a plain space character may work. There are a couple others we always encode, such as <>.

But as Nika pointed out, changing the serialization may prove tricky at this time, as we have the origins serialized in the profile in a lot of different places.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

:sekim, since you are the author of the regressor, bug 1873973, could you take a look?

For more information, please visit BugBot documentation.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox128 (nightly). We have limited time to fix this, the soft freeze is in 3 days. However, the bug still isn't assigned and has low severity.

:ghess, could you please find an assignee and increase the severity for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 7

[Andrew McCreight [:mccr8]]

Fixed by backout.

Comment 8

[Sunil Mayya]

Moving this to priority-queue-next as from the above discussion we will plan to fix this soon.

Comment 9

[edgul]

IIUC, the regression has been resolved by the backout and other discussion is around the original issue so I am closing.
Please re-open if this is not the case.

Comment 10

[Daniel Veditz [:dveditz]]

sec-other undersells the risk. if file:// is the only one we know about maybe sec-moderate works if people disagree with sec-high