Last Comment Bug 627234 - HSTS header not honored for subordinate (img) resource loads
: HSTS header not honored for subordinate (img) resource loads
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: David Keeler [:keeler] (use needinfo?)
:
:
Mentors:
Depends on: 804690
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-19 15:55 PST by Sid Stamm [:geekboy or :sstamm]
Modified: 2012-10-24 01:45 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
patch (4.74 KB, patch)
2012-07-02 16:26 PDT, David Keeler [:keeler] (use needinfo?)
honzab.moz: review+
brian: review+
mozbugs: feedback+
Details | Diff | Splinter Review
patch (5.04 KB, patch)
2012-08-03 09:39 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
test (3.05 KB, patch)
2012-10-22 10:41 PDT, David Keeler [:keeler] (use needinfo?)
brian: superreview+
Details | Diff | Splinter Review

Description Sid Stamm [:geekboy or :sstamm] 2011-01-19 15:55:52 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2011-01-19 18:13:46 PST
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.
Comment 2 Sid Stamm [:geekboy or :sstamm] 2011-01-20 08:05:45 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2011-01-20 08:09:18 PST
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.
Comment 4 Sid Stamm [:geekboy or :sstamm] 2011-01-20 08:14:01 PST
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.
Comment 5 Sid Stamm [:geekboy or :sstamm] 2011-01-20 10:53:02 PST
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 Jonathan Mayer 2011-01-20 14:06:56 PST
Able to reliably reproduce after trashing the innards of a profile.  Weird.
Comment 7 David Keeler [:keeler] (use needinfo?) 2012-07-02 16:26:02 PDT
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.
Comment 8 Sid Stamm [:geekboy or :sstamm] 2012-07-05 10:03:15 PDT
David: would this issue still exist if we landed your patch for bug 760307 (hsts preload list)?
Comment 9 Sid Stamm [:geekboy or :sstamm] 2012-07-05 10:06:58 PDT
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.
Comment 10 David Keeler [:keeler] (use needinfo?) 2012-07-05 10:18:26 PDT
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?)
Comment 11 Sid Stamm [:geekboy or :sstamm] 2012-07-05 10:22:20 PDT
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.
Comment 12 Honza Bambas (:mayhemer) 2012-07-05 12:53:15 PDT
Personally, I'd start with a test anyway.  There would definitely be a review request from me to add a test anyway.
Comment 13 Sid Stamm [:geekboy or :sstamm] 2012-07-05 13:02:12 PDT
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).
Comment 14 Honza Bambas (:mayhemer) 2012-07-05 13:04:52 PDT
(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 15 Honza Bambas (:mayhemer) 2012-07-05 13:41:20 PDT
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.
Comment 16 David Keeler [:keeler] (use needinfo?) 2012-08-03 09:39:59 PDT
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.
Comment 17 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-21 13:11:43 PDT
(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?
Comment 18 David Keeler [:keeler] (use needinfo?) 2012-10-22 10:41:13 PDT
Created attachment 673922 [details] [diff] [review]
test

We just need the test now.
Comment 19 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-22 10:53:36 PDT
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.
Comment 20 David Keeler [:keeler] (use needinfo?) 2012-10-22 13:12:23 PDT
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/35e15835ddc9

I'll make sure it makes it to central before uplifting.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-10-22 19:00:21 PDT
https://hg.mozilla.org/mozilla-central/rev/35e15835ddc9

Note You need to log in before you can comment on or make changes to this bug.