Closed Bug 1416344 Opened 3 years ago Closed 3 years ago

network.http.referer.XOriginTrimmingPolicy to above 0 or network.http.referer.trimmingPolicy==2 crashes tabs

Categories

(Core :: Networking: HTTP, defect)

58 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 blocking fixed
firefox59 --- fixed

People

(Reporter: thesweetlilycake, Assigned: tnguyen)

References

Details

(5 keywords)

Crash Data

User Story

Crashes are happening with network.http.referer.XOriginTrimmingPolicy set to 1 or 2, or (from bug 1416378) if network.http.referer.trimmingPolicy is set to 2. This is due to bug 1410186 which turned the asserts for Maybe<> isSome into release asserts. isCrossOrigin can apparently be uninitialized which means
1) we don't have any tests for this, and 
2) we were randomly not sending the right referrer in older versions.

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171110100139

Steps to reproduce:

Set network.http.referer.XOriginTrimmingPolicy to either 1 or 2.
Open up Google.com
Tab should now crash.


Actual results:

The tab crashed when trying to open the site.


Expected results:

The site should open and not crash.
Forgot to mention, this started happening with the latest nightly version.
Crash Signature: [@ mozilla::Maybe<T>::operator* ]
Component: Untriaged → Networking: HTTP
Keywords: crash, crashreportid
Product: Firefox → Core
(In reply to Lily Rose from comment #2)
> Crash report:
> https://crash-stats.mozilla.com/report/index/bd18fa79-3055-41b2-8516-
> 4e5720171110#tab-details
> 
> Sorry for the noise.

No problem, thank you for the report!

This would be reading uninitialized memory on non-Nightly browsers, so I'm going to classify it as a security bug.  Bug 1360973 introduced this code.

I think the right thing to do here is just check whether `isCrossOrigin` is initialized with a useful value...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Was just going to file this since I saw it in Nightly crash stats: http://bit.ly/2AA1fh0. Crashes seem to have started using 20171110100139 but not 100% sure.

411 crashes/23 installs.

Possible regression based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f63559d7e6a570e4e73ba367964099394248e18d&tochange=864174ac0707207f7fc698ed6c3c47c043e99395 (note there was no Windows build on the 9th?)
The crashes are because bug 1410186 landed, which release-asserts on misuses of mozilla::Maybe<T>.  We would compute bogus data in this function prior to that bug.

I guess network.http.referer.XOriginTrimmingPolicy doesn't normally get set (even in tests?), which is why we've never noticed this before.
Group: core-security → network-core-security
Duplicate of this bug: 1416378
See dupe bug 1416378 (and the reason I'm here): this crash can also be triggered by using the network.http.referer.trimmingPolicy preference set to 2. And looking at the code it can now crashes on line 1815 if the page itself uses a same-origin referrer policy
  if (mReferrerPolicy == REFERRER_POLICY_SAME_ORIGIN && *isCrossOrigin) {

I'm going to unhide because this is not a security problem for the default Firefox configuration (these buried prefs aren't used by most folks). It could be a privacy problem for people who have been relying on these custom referrer policy settings. The problem gets worse in 58 because all those folks will crash now.

We should double-check that a page-set referrerPolicy doesn't also cause the same crash. I would have thought we had tests for those since it's a newish feature, but those are checked in the same messy compound 'if' statements.

Christoph: should this bug be moved to the "DOM: Security" component because it's related to referrer policy, or leave it in networking?
Blocks: 1410186, 1360973
User Story: (updated)
Flags: needinfo?(ckerschb)
Keywords: privacy, regression
Summary: network.http.referer.XOriginTrimmingPolicy to above 0 crashes tabs → network.http.referer.XOriginTrimmingPolicy to above 0 or network.http.referer.trimmingPolicy==2 crashes tabs
Group: network-core-security
This is the #1 non-ShutdownKill Windows crash in Nightly 20171110100139, with 856 crashes, which is *very* high. I.e. this Nightly is probably 10x crashier than typical.

Jason, can you please find an owner?

In the meantime, I'm going to back out bug 1410186 to fix the crashes.
Flags: needinfo?(jduell.mcbugs)
(In reply to Daniel Veditz [:dveditz] from comment #7)
> See dupe bug 1416378 (and the reason I'm here): this crash can also be
> triggered by using the network.http.referer.trimmingPolicy preference set to
> 2. And looking at the code it can now crashes on line 1815 if the page
> itself uses a same-origin referrer policy
>   if (mReferrerPolicy == REFERRER_POLICY_SAME_ORIGIN && *isCrossOrigin) {
> 
> I'm going to unhide because this is not a security problem for the default
> Firefox configuration (these buried prefs aren't used by most folks). It
> could be a privacy problem for people who have been relying on these custom
> referrer policy settings. The problem gets worse in 58 because all those
> folks will crash now.

Given the high crash rates, are we sure this is the case?

> We should double-check that a page-set referrerPolicy doesn't also cause the
> same crash. I would have thought we had tests for those since it's a newish
> feature, but those are checked in the same messy compound 'if' statements.

Perhaps this is triggered by page policy and that is why the crash rates are so high.  We really need to make fixing this a priority.

Given the very high crash rates I'm going to move this back to hidden for now.  If we can prove it doesn't happen based on page policy we can unhide again.  Sorry if this is inappropriate and for the flag churn.
Group: core-security
Note that it looks like crashes spiked to 600+ on one build and then dropped to zero. Crashes will be misleading because it makes the browser entirely unusable for anyone with these settings. Most affected users will have to switch browsers (downgrade to beta, or to another browser entirely). Some nightly users might notice SetReferrerWithPolicy() in their crash stack and remember they tweaked these settings and thus be able to unbreak themselves. Either way the crashes will stop quickly but that won't mean users are OK.

Note also that various privacy help pages provide a user.js people can copy into their profile, and the user may not even realize these particular settings are tweaked. They will show up as "default" settings in about:config, too, so there really won't be much of a clue.

Liz: hopefully this will be resolved quickly, but can we make this block Fx58?
(In reply to Ben Kelly [:bkelly] from comment #9)
> Given the very high crash rates I'm going to move this back to hidden for now.

The crash is terrible as a DoS, but it's not exploitable.

The underlying problem is that bug 1360973 removed the initialization of isCrossOrigin in Fx55. People who had set only network.http.referer.trimmingPolicy were fine because that value will be used for all referrers regardless of the crossorigin state. If network.http.referer.XOriginTrimmingPolicy was set then the cross-origin state does matter, which means an uninitialized boolean was controlling whether we would send the default referrer or a more private one. Since we think the default referrer is perfectly fine for most users I don't think anyone is at risk if this bug is public.

You raise the possibility that some of the crashes might be coming from a page with an explicit ReferrerPolicy because of the high number. You might be underestimating the number of Firefox users with a concern for privacy, but let's assume that's true. That could mean that a page that was trying to strip private path information out of referrers might actually be sending it sometimes. That could be bad, but on the other hand the ReferrerPolicy spec is still not universally supported so sites with SUPER sensitive information still need to use whatever whitewashing techniques they've used in the past.

There are web platform tests for all the ReferrerPolicy settings so I'd be surprised if a page policy was triggering this, unless we've disabled all those tests.
Flags: needinfo?(lhenry)
tnguyen: you did the review for bug 1360973. Could you take another look at the logic there? Ehsan is away.
Flags: needinfo?(tnguyen)
I will take a look at this.
Flags: needinfo?(tnguyen)
Assignee: nobody → tnguyen
marking as blocking per comment 10
Flags: needinfo?(lhenry)
Duplicate of this bug: 1416698
Crash Signature: [@ mozilla::Maybe<T>::operator* ] → [@ mozilla::Maybe<T>::operator* ] [@ mozilla::net::HttpBaseChannel::SetReferrerWithPolicy]
(In reply to Daniel Veditz [:dveditz] from comment #11)
> (In reply to Ben Kelly [:bkelly] from comment #9)
> > Given the very high crash rates I'm going to move this back to hidden for now.
> 
> The crash is terrible as a DoS, but it's not exploitable.

If you want to make it visible again, then I don't object.  It just seemed the crash rate being so high was new information that might suggest a bigger problem than initial analysis suggested.  I figured better safe than sorry.

> You raise the possibility that some of the crashes might be coming from a
> page with an explicit ReferrerPolicy because of the high number. You might
> be underestimating the number of Firefox users with a concern for privacy,
> but let's assume that's true. That could mean that a page that was trying to
> strip private path information out of referrers might actually be sending it
> sometimes. That could be bad, but on the other hand the ReferrerPolicy spec
> is still not universally supported so sites with SUPER sensitive information
> still need to use whatever whitewashing techniques they've used in the past.

Sorry, I thought you raised that possibility in comment 7.  Maybe I misunderstood.

Anyway, happy to see this opened up again if everyone is convinced its not a legit security issue.
It's a legit privacy issue, but not one where informing attackers enables additional abuse. Making it public can inform users at risk about the problem in case they need to take steps.
Group: core-security
Duplicate of this bug: 1416605
Attachment #8927794 - Attachment is obsolete: true
Bkelly, could you take a look at the patch?
Comment on attachment 8928056 [details]
Bug 1416344 - refactor computing referrer policy and remove uninitilized maybe value

https://reviewboard.mozilla.org/r/199286/#review204342


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/protocol/http/HttpBaseChannel.cpp:1594
(Diff revision 1)
> +      }
> +    }
> +    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> +    rv = ssm->CheckSameOriginURI(triggeringURI, mURI, false);
> +    return (NS_FAILED(rv));
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8928056 [details]
Bug 1416344 - refactor computing referrer policy and remove uninitilized maybe value

Sorry, since this is completely in necko I think a necko peer should probably review it.  Also, I am partially out of the office today with a sick kid.
Attachment #8928056 - Flags: review?(bkelly) → review?(valentin.gosu)
Comment on attachment 8928056 [details]
Bug 1416344 - refactor computing referrer policy and remove uninitilized maybe value

https://reviewboard.mozilla.org/r/199286/#review204500

It looks good. Just a few nits. Thanks for the patch!

::: netwerk/protocol/http/HttpBaseChannel.cpp:1587
(Diff revision 2)
> +  }
> +  if (triggeringURI) {
> +    if (LOG_ENABLED()) {
> +      nsAutoCString triggeringURISpec;
> +      rv = triggeringURI->GetAsciiSpec(triggeringURISpec);
> +      if (!NS_FAILED(rv)) {

if (NS_SUCCEEDED(rv)) is better. Or you can not check it at all. It's quite rare that GetAsciiSpec fails, and doesn't really improve the logging.

::: netwerk/test/unit/test_referrer_cross_origin.js:11
(Diff revision 2)
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function test_policy(test) {
> +  do_print("Running test: " + test.toSource());
> +
> +  let prefs = Cc["@mozilla.org/preferences-service;1"]

You can use Services.prefs instead

::: netwerk/test/unit/test_referrer_cross_origin.js:309
(Diff revision 2)
> +
> +];
> +
> +function run_test() {
> +  gTests.forEach(test => test_policy(test));
> +}

You might want to clearUserPref after running the test
Attachment #8928056 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8928056 [details]
Bug 1416344 - refactor computing referrer policy and remove uninitilized maybe value

https://reviewboard.mozilla.org/r/199286/#review204684


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/protocol/http/HttpBaseChannel.cpp:1586
(Diff revision 3)
> +    }
> +  }
> +  if (triggeringURI) {
> +    if (LOG_ENABLED()) {
> +      nsAutoCString triggeringURISpec;
> +      rv = triggeringURI->GetAsciiSpec(triggeringURISpec);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Pushed by tnguyen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db708f0e6796
refactor computing referrer policy and remove uninitilized maybe value r=valentin
https://hg.mozilla.org/mozilla-central/rev/db708f0e6796
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(jduell.mcbugs)
(In reply to Daniel Veditz [:dveditz] from comment #7)
> Christoph: should this bug be moved to the "DOM: Security" component because
> it's related to referrer policy, or leave it in networking?

Sorry for being late to the party. I guess it doesn't matter anymore. FWIW, probably we should track all referrer policy bugs within DOM:Security and also have them block the Meta Bug 1409600.
Flags: needinfo?(ckerschb)
IIUC, Fx58 was already fixed by way of backing out bug 1410186. Please set the status back to affected and request Beta approval on the patch if I've got that wrong, though.
That backed out the assert, but I think the underlying problem still exists in FF58.
Comment on attachment 8929961 [details] [diff] [review]
refactor computing referrer policy and remove uninitilized maybe value

Yes, I agree with Ben, the underlying problem still need to be fixed.

Approval Request Comment
[Feature/Bug causing the regression]: 1410186, 1360973 
[User impact if declined]: Crash
[Is this code covered by automated tests?]: Yes,
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: We may need to make sure the referrer header is sent correctly (and no crash) if network.http.referer.XOriginTrimmingPolicy is set to 1 or 2, or network.http.referer.trimmingPolicy is to 2.
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No.
[Why is the change risky/not risky?]: We refactored the code but kept the same logic (basically from if statement to switch). Automated tests are added to make sure that
[String changes made/needed]: No
Attachment #8929961 - Flags: approval-mozilla-beta?
Comment on attachment 8929961 [details] [diff] [review]
refactor computing referrer policy and remove uninitilized maybe value

Since bug 1410186 was backout from 58, there is no need to take this in 58 at this moment. Beta58-.
Attachment #8929961 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Gerry Chang [:gchang] from comment #39)
> Comment on attachment 8929961 [details] [diff] [review]
> refactor computing referrer policy and remove uninitilized maybe value
> 
> Since bug 1410186 was backout from 58, there is no need to take this in 58
> at this moment. Beta58-.

Sorry, but I don't think this is not correct.

Bug 1410186 added an assertion which brought this problem to our attention.  The problem is unitialized memory being used.  Backing out the assertion did not start initializing the memory in this case.  We need to fix this.

Possibly this would be clearer if the title of this bug was changed to reflect the underlying problem, etc.
Flags: needinfo?(gchang)
I had a short talk with gchang and the uplift request was rejected because we don't have a strong enough reason. Given that the uninitialized memory problem existed from several previous versions of FF (not only FF58, but from FF55) but that's not a security risk that attacker could rely on (in fact if the preferences XOriginTrimmingPolicy or TrimmingPolicy was set, an uninitialized boolean would offer default or even more private referrer header). Besides, the crashes only occurred after we landed 1410186, but it was backed out from FF58. So, let's fix this from 59 (unless we are intending to uplift 1410186 again on 58). For me, I am ok with either way :)
Ok.  All I'll add is that we are at the beginning of the release cycle, so this doesn't seem that risky to me.  But ok.
Flags: needinfo?(gchang)
You need to log in before you can comment on or make changes to this bug.