Closed Bug 1248452 Opened 6 years ago Closed 6 years ago

Change one-off checks for HTTP Strict Transport Security upgrades to use consistent codepath

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

My patches in bug 1247733 (which change a piece of state that under the hood when promoting a nsIURI for HTTP Strict Transport Security / HSTS) currently cause 2 test-failures, because we have hardcoded checks elsewhere in the codebase that try to do similar types of nsIURI promotion.

These hardcoded checks need to be upgraded in the same way as bug 1247733 -- but really, they should just make use of the same helper-function (HttpBaseChannel::GetSecureUpgradedURI) that we're changing in bug 1247733.
Try run showing the test failures, in test_websocket.html and csp/test_upgrade_insecure_cors.html:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b4dd29b5a09
Assignee: nobody → dholbert
BTW: both of these test-failures are the same sort of problem as in bug 1247733 -- we're comparing two nsIURI values for equality, and the comparison is failing because they have different mDefaultPort values. (because one of them has had its scheme tweaked without its default-port being tweaked)
This first patch rips out the custom URI-upgrading logic in NS_IsHSTSUpgradeRedirect(), and replaces it with a call to HttpBaseChannel::GetSecureUpgradedURI(), which is the canonical place where this logic lives now.

This fixes the csp/test_upgrade_insecure_cors.html test failure.
Attachment #8719571 - Flags: review?(jduell.mcbugs)
(oops, had wrong bug number in commit message; fixed now.)
Attachment #8719571 - Attachment is obsolete: true
Attachment #8719571 - Flags: review?(jduell.mcbugs)
Attachment #8719575 - Flags: review?(jduell.mcbugs)
WebSocketChannel::AsyncOnChannelRedirect has a bunch of code that's just trying to detect HSTS upgrades.

This second patch replaces that custom code with a single call to NS_IsHSTSUpgradeRedirect, the same function we fixed in part 1.  This function is explicitly designed to check for this sort of thing, so it's what we should be using here -- and we rely on it in one other AsyncOnChannelRedirect implementation, in nsCORSListenerProxy:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp?rev=642aa364f5ae#640

This patch fixes the test_websocket.html test failure.
Attachment #8719577 - Flags: review?(jduell.mcbugs)
BTW: my intention is for this bug's patches to land first, and then I'll land bug 1247733's patches on top of them.

By itself, this bug is purely refactoring / code-sharing, and shouldn't change behavior.  And then once we add bug 1247733's patches on top, all of the shared codepaths will benefit from its fix, instead of just one. (and tests won't break)
Try run with just this bug's patches:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecae314cba86

Try run with the patches here as well as bug 1247733:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7057cdcc8465

Both runs are green, modulo some intermittents (particularly bug 845176, which is effectively a perma-fail & is unrelated to this bug, per bug 845176 comment 159)
Comment on attachment 8719575 [details] [diff] [review]
part 1: Rewrite NS_IsHSTSUpgradeRedirect to use HttpBaseChannel::GetSecureUpgradedURI

Review of attachment 8719575 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsNetUtil.cpp
@@ -1922,5 @@
> -  bool isHttps;
> -  if (NS_FAILED(newURI->SchemeIs("https", &isHttps)) || !isHttps) {
> -    return false;
> -  }
> -

Unless I'm missing something, by removing the check that newURI is https, and then blindly forcing https for upgradedURI (by calling GetSecureUpgradedURI), this code would now mistake a circular redirect (from http to same http URI) as an HSTS upgrade.  I think we may need to keep this check to avoid that.
> this code would now mistake a circular redirect
> (from http to same http URI) as an HSTS upgrade.

I'm missing how that would happen. In particular:
 - GetSecureUpgradedURI will always produce an HTTPS uri. (or fail)
 - This function that I'm modifying here (NS_IsHSTSUpgradeRedirect) will check newURI against the thing that was returned by GetSecureUpgradedURI, as the last thing it does.
 - So, an HTTP to HTTP redirect would fail that comparison -- "upgradedURI->Equals(newURI, &res))" would produce 'false' in 'res'.
Comment on attachment 8719575 [details] [diff] [review]
part 1: Rewrite NS_IsHSTSUpgradeRedirect to use HttpBaseChannel::GetSecureUpgradedURI

Review of attachment 8719575 [details] [diff] [review]:
-----------------------------------------------------------------

Right you are
Attachment #8719575 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8719577 [details] [diff] [review]
part 2: Rewrite WebSocketChannel::AsyncOnChannelRedirect to use NS_IsHSTSUpgradeRedirect instead of custom HSTS detection code

Canceling review on part 2 - jduell points out in IRC that the contextual code has changed here, and it looks like the problematic Equals() call that I was replacing here is now gone. (replaced with some flag checks)

So we might be good with just "part 1" here. (in which case I'll drop the "part 1" label and call this bug done)
Attachment #8719577 - Flags: review?(jduell.mcbugs)
Comment on attachment 8719577 [details] [diff] [review]
part 2: Rewrite WebSocketChannel::AsyncOnChannelRedirect to use NS_IsHSTSUpgradeRedirect instead of custom HSTS detection code

Yeah, the part 2 code seems to have been fixed independently in bug 1247806.
Attachment #8719577 - Attachment is obsolete: true
Comment on attachment 8719575 [details] [diff] [review]
part 1: Rewrite NS_IsHSTSUpgradeRedirect to use HttpBaseChannel::GetSecureUpgradedURI

One self-critique (tiny enough that I'm just going to fix it before landing, without bothering another round of review):

>+++ b/netwerk/base/nsNetUtil.cpp
[...]
>+  HttpBaseChannel::GetSecureUpgradedURI(oldURI, getter_AddRefs(upgradedURI));
> 
>   bool res;
>   return NS_SUCCEEDED(upgradedURI->Equals(newURI, &res)) && res;

For optimal propriety, I should technically be checking the nsresult return value of HttpBaseChannel::GetSecureUpgradedURI here, and return false if it failed, without bothering with an Equals() comparison.
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/a94e767c3952
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.