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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Lily Kim]

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.

Comment 1

[Lily Kim]

Forgot to mention, this started happening with the latest nightly version.

Comment 2

[Lily Kim]

Crash report: https://crash-stats.mozilla.com/report/index/bd18fa79-3055-41b2-8516-4e5720171110#tab-details

Sorry for the noise.

Comment 3

[Nathan Froyd [:froydnj]]

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

Comment 4

[Marcia Knous [:marcia]]

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

Comment 5

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Daniel Veditz [:dveditz]]

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?

Comment 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 10

[Daniel Veditz [:dveditz]]

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?

Comment 11

[Daniel Veditz [:dveditz]]

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

Comment 12

[Daniel Veditz [:dveditz]]

tnguyen: you did the review for bug 1360973. Could you take another look at the logic there? Ehsan is away.

Comment 13

[Thomas Nguyen (:tnguyen)]

I will take a look at this.

Comment 14

[Julien Cristau [:jcristau]]

marking as blocking per comment 10

Comment 16

[Thomas Nguyen (:tnguyen)]

Attachment: refactor computing referrer policy and remove uninitilized maybe value

Comment 17

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c753bb78ac7f05b5337fb7b280db9e865335085

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 19

[Daniel Veditz [:dveditz]]

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.

Comment 21

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1416344 - refactor computing referrer policy and remove uninitilized maybe value

Review commit: https://reviewboard.mozilla.org/r/199286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199286/

Comment 22

[Thomas Nguyen (:tnguyen)]

Bkelly, could you take a look at the patch?

Comment 23

[Code Review Bot [:reviewbot]]

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 24

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8928056 [details]
Bug 1416344 - refactor computing referrer policy and remove uninitilized maybe value

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199286/diff/1-2/

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 26

[Valentin Gosu [:valentin] (he/him)]

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

Comment 27

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8928056 [details]
Bug 1416344 - refactor computing referrer policy and remove uninitilized maybe value

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199286/diff/2-3/

Comment 28

[Thomas Nguyen (:tnguyen)]

Thanks Valentin for your review.
Triggering try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=072f1a2c3fe274f56d59ad7083e7d6b5415887f8

Comment 29

[Code Review Bot [:reviewbot]]

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]

Comment 30

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8928056 [details]
Bug 1416344 - refactor computing referrer policy and remove uninitilized maybe value

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199286/diff/3-4/

Comment 31

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=165a749c217b25033ba0b4e8942dccd46244c74b&selectedJob=144849163

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db708f0e6796
refactor computing referrer policy and remove uninitilized maybe value r=valentin

Comment 33

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/db708f0e6796

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

That backed out the assert, but I think the underlying problem still exists in FF58.

Comment 37

[Thomas Nguyen (:tnguyen)]

Attachment: refactor computing referrer policy and remove uninitilized maybe value

MozReview-Commit-ID: 7VoRaUSE096

Comment 38

[Thomas Nguyen (:tnguyen)]

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

Comment 39

[Gerry Chang [:gchang]]

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

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 41

[Thomas Nguyen (:tnguyen)]

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

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

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.