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

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla47
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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)

Updated

3 years ago
Assignee: nobody → dholbert
(Assignee)

Comment 2

3 years ago
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)
(Assignee)

Comment 3

3 years ago
Created attachment 8719571 [details] [diff] [review]
part 1: Rewrite NS_IsHSTSUpgradeRedirect to use HttpBaseChannel::GetSecureUpgradedURI

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)
(Assignee)

Comment 4

3 years ago
Created attachment 8719575 [details] [diff] [review]
part 1: Rewrite NS_IsHSTSUpgradeRedirect to use HttpBaseChannel::GetSecureUpgradedURI

(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)
(Assignee)

Comment 5

3 years ago
Created attachment 8719577 [details] [diff] [review]
part 2: Rewrite WebSocketChannel::AsyncOnChannelRedirect to use NS_IsHSTSUpgradeRedirect instead of custom HSTS detection code

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)
(Assignee)

Comment 6

3 years ago
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)
(Assignee)

Comment 7

3 years ago
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 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
> 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 10

3 years ago
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+
(Assignee)

Comment 11

3 years ago
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)
(Assignee)

Comment 12

3 years ago
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
(Assignee)

Comment 14

3 years ago
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.
(Assignee)

Updated

3 years ago
Flags: in-testsuite-

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a94e767c3952
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.