Note: There are a few cases of duplicates in user autocompletion which are being worked on.

HSTS header not honored for subordinate (img) resource loads

RESOLVED FIXED in Firefox 17

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: geekboy, Assigned: keeler)

Tracking

Trunk
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17 fixed, firefox18 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Jonathan Mayer reports:

HSTS flags might not be set for subordinate loads (non-document page elements).  Here are steps for reproducing:

1) Navigate to http://www.eff.org
The page includes the element <img src="https://www.eff.org/files/https.png" width="1" height="1"  border="0">.  In its response to the image request, EFF's HTTPS server includes the HSTS header Strict-Transport-Security: max-age=2628000.

2) Re-navigate to http://www.eff.org
There's no redirect to https://www.eff.org

This same usage results in a redirect with Chrome's HSTS implementation.

Comment 1

7 years ago
That's... odd.  There's nothing that should be different about images in ProcessSTSHeader.

And if I follow the steps to reproduce in comment 0, I get redirected to https://www.eff.org, in a Jan 18 nightly.
(Reporter)

Comment 2

7 years ago
I'm able to confirm the same (non-redirecting) behavior as Jonathan in 4.0b9 on Mac OS X 10.6.6, but I am too confused.  Using the web console, I could confirm that the image is loaded over HTTPS and served with a valid "Strict-transport-security" HTTP header in the response.

Perhaps there's a caching issue at play here somewhere?  I'll dive into a debug/logging build and see if I can reproduce.

Comment 3

7 years ago
Just to make sure, is comment 2 with a clean profile?  Comment 1 was on 10.6.6 with a clean nightly build and a clean profile.
(Reporter)

Comment 4

7 years ago
bz: good call.  Forgot to start fresh.  

Comment 2 was with an old profile; with a clean profile HSTS seems to work as desired.  I still want to look into it to see if something in the permissions DB when awry during a beta upgrade.
(Reporter)

Updated

7 years ago
Assignee: nobody → sstamm
(Reporter)

Comment 5

7 years ago
I was able to reproduce this effect when in permanent private browsing mode (never remember history).  This could be a side-effect that bug 557598 would fix by supporting HSTS better in private mode.  Following up with the reporter.

Comment 6

7 years ago
Able to reliably reproduce after trashing the innards of a profile.  Weird.
Created attachment 638532 [details] [diff] [review]
patch

Here's what I've found. Say you're in private browsing mode and you've never visited example.com. Say example.com sends an hsts header without includeSubdomains. The sts service adds a temporary permission for example.com (in the private browsing table). Since includeSubdomains is not set, it deletes the subdomain permission. However, since there is nothing in the permission manager about example.com, this takes the code path that deletes the entry in the private browsing table.
The fix is to set the appropriate flags in RemovePermission based on what permission you're modifying.
Assignee: sstamm → dkeeler
Status: NEW → ASSIGNED
Attachment #638532 - Flags: review?(sstamm)
(Reporter)

Comment 8

5 years ago
David: would this issue still exist if we landed your patch for bug 760307 (hsts preload list)?
(Reporter)

Comment 9

5 years ago
Comment on attachment 638532 [details] [diff] [review]
patch

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

I'm not a peer so I'm not supposed to r+, but this looks fine to me.  Redirecting review to a peer.
Attachment #638532 - Flags: review?(sstamm)
Attachment #638532 - Flags: review?(honzab.moz)
Attachment #638532 - Flags: feedback+
The hsts preload list patch does actually fix this - should I roll the test for this one into the test for that one? Or just add it as a separate test? (Or should we continue moving forward with this as a separate patch?)
(Reporter)

Comment 11

5 years ago
If you think we'll land the HSTS preload in the next few weeks, we can merge the two bits of work (tests and all).  Otherwise, we should land this first with its own tests.
Personally, I'd start with a test anyway.  There would definitely be a review request from me to add a test anyway.
(Reporter)

Comment 13

5 years ago
mayhemer: the patch flagged r? for you already has a test, so this patch is ready for review.  

I think David was asking if he should keep the test for this bug separate from the ones in the other bug or if he should combine them all (if we decide to combine the two bits of work).
(In reply to Sid Stamm [:geekboy] from comment #13)
> mayhemer: the patch flagged r? for you already has a test, so this patch is
> ready for review.  

Ah, I had to take a look at the patch first :)  Now I can see it.
Comment on attachment 638532 [details] [diff] [review]
patch

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

r=honzab

Since author is not a PSM module peer/owner this needs a secondary review.
Attachment #638532 - Flags: review?(honzab.moz)
Attachment #638532 - Flags: review?(bsmith)
Attachment #638532 - Flags: review+
Attachment #638532 - Flags: review?(bsmith) → review+
Created attachment 648746 [details] [diff] [review]
patch

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9e658d0da601
Also, I had to change the test slightly to properly clean up state, so if Brian or Honza could sign off that that's okay, that'd be great.
Attachment #638532 - Attachment is obsolete: true
Attachment #648746 - Flags: review?(bsmith)
(In reply to David Keeler from comment #10)
> The hsts preload list patch does actually fix this - should I roll the test
> for this one into the test for that one? Or just add it as a separate test?
> (Or should we continue moving forward with this as a separate patch?)

Sorry about the delay here. Now I am confused about what needs to be done and what needs to be reviewed. Do we still need this patch, or just the test?
Created attachment 673922 [details] [diff] [review]
test

We just need the test now.
Attachment #648746 - Attachment is obsolete: true
Attachment #648746 - Flags: review?(bsmith)
Attachment #673922 - Flags: review?(bsmith)
Comment on attachment 673922 [details] [diff] [review]
test

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

Please land this on -aurora and -beta too. You can use a=testonly to avoid bothering release drivers for approval.
Attachment #673922 - Flags: review?(bsmith) → superreview+
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/35e15835ddc9

I'll make sure it makes it to central before uplifting.
https://hg.mozilla.org/mozilla-central/rev/35e15835ddc9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/f748ad6a38fc
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/417cbc243c6c
status-firefox17: --- → fixed
status-firefox18: --- → fixed
Depends on: 804690
You need to log in before you can comment on or make changes to this bug.