Closed Bug 1495321 Opened 2 years ago Closed 2 years ago

Some http:// website pages (on sites using http/2) are shown as secure ones after visiting/opening https:// versions after landing patch from bug #832834

Categories

(Core :: Security: PSM, defect, P1)

64 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 + verified

People

(Reporter: Virtual, Assigned: keeler)

References

(Regression, )

Details

(5 keywords, Whiteboard: [psm-assigned])

Attachments

(2 files)

Attached video screencast.mp4
STR:
1. Open in first tab this website page  - http://fanfox.net/
2. Open in second tab this website page - https://fanfox.net/
3. Open in third tab this website page  - http://fanfox.net/
and see that Mozilla Firefox Nightly 64.0a1 shows in third tab that http://fanfox.net/ website page is secure (green padlock and "Secure Connection" in site security connection details)
Regression caused by:
bug #832834

Regression pushlog range:
https://hg.mozilla.org/integration/autoland/json-pushes?changeset=2f4adf14e6231a1668558dd78ecbe56a421591b6&full=1
Blocks: 832834
Group: firefox-core-security → core-security
Has Regression Range: --- → yes
Component: Site Identity and Permission Panels → Security: PSM
Flags: needinfo?(dkeeler)
Product: Firefox → Core
Group: core-security → core-security-release
Keywords: sec-high
Summary: Mozilla Firefox Nightly 64.0a1 shows some http:// website pages as secure ones after visiting/opening https:// versions → Some http:// website pages are shown as secure ones after visiting/opening https:// versions after landing patch from bug #832834
This is basically bug 1040323 again. "But why didn't tests catch this?" you ask? Well, we never added them.
http://fanfox.net is sending an alt-svc header indicating that its content is available at https://fanfox.net. When we load http://fanfox.net the second time, we use the alternative location https://fanfox.net (and note that the certificate is in fact valid for fanfox.net). Since that channel has a securityInfo, we use that to update the site identity drop-down (when we shouldn't).
Assignee: nobody → dkeeler
Flags: needinfo?(dkeeler)
Priority: -- → P1
Whiteboard: [psm-assigned]
(In reply to Dana Keeler [:keeler] (she/her) (use needinfo) from comment #2)
> This is basically bug 1040323 again. "But why didn't tests catch this?" you
> ask? Well, we never added them.

Let's fix that this time around :)
Flags: in-testsuite?
Resources that aren't secure (e.g. http) can be routed over secure transports
(e.g. http/2 alt-svc, https proxies). For display purposes (the site identity
widget) we don't want to treat these as secure. Bug 1040323 addressed this exact
issue but didn't include tests. Thus, when nsSecureBrowserUIImpl was
reimplemented in bug 832834, this aspect was neglected. This time, there is a
test.
Comment on attachment 9014466 [details]
bug 1495321 - only allow https URIs to be considered secure in nsSecureBrowserUIImpl r?Gijs

(Gijs did r+ this, but I maybe phabricator can't set flags in secure bugs? or something else is broken? ¯\_(ツ)_/¯ )

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Fairly easily, as long as they know of a target server using h2/alt-svc and http or a victim that's using an http-over-tls proxy. I think the danger here is that users will see the lock, think "oh, it's safe to enter my credit card info here", and then they've leaked it to anyone listening on the wire.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: none

If not all supported branches, which bug introduced the flaw?: Bug 832834

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: n/a

How likely is this patch to cause regressions; how much testing does it need?: There might be regressions in similar edge-cases (although at this point the code is designed to fail safe by falling back to saying "not secure" if anything goes wrong). There is a test, but it would be great to have more eyes on this implementation in general.
Attachment #9014466 - Flags: sec-approval?
Attachment #9014466 - Flags: review+
Comment on attachment 9014466 [details]
bug 1495321 - only allow https URIs to be considered secure in nsSecureBrowserUIImpl r?Gijs

This is trunk-only, so it can go ahead and land whenever.
Flags: needinfo?(dkeeler)
Attachment #9014466 - Flags: sec-approval?
(In reply to Dana Keeler [:keeler] (she/her) (use needinfo) from comment #5)
> Comment on attachment 9014466 [details]
> bug 1495321 - only allow https URIs to be considered secure in
> nsSecureBrowserUIImpl r?Gijs
> 
> (Gijs did r+ this, but I maybe phabricator can't set flags in secure bugs?
> or something else is broken? ¯\_(ツ)_/¯ )

Phabricator stopped syncing r+ to bugzilla altogether ( https://groups.google.com/d/msg/firefox-dev/AWCxezSC-FI/664Oz9OXBwAJ ).
So the issue I've been having is that adding a mochitest-browser-chrome test changes the chunking, which is causing one chunk to take 60 minutes on OS X debug, which is the timeout limit, so the test run fails. I'm not sure what the right solution here is, but as a workaround, I put the new tests in browser/base/content/test/siteIdentity/browser_navigation_failures.js for now.
Here's that try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e41891a510717a4b5c76e282b44ebda64c09cca
Since OS X tests are backed up about 6 hours, we'll know sometime later tonight if that worked.
Flags: needinfo?(dkeeler)
(In reply to Dana Keeler [:keeler] (she/her) (use needinfo) from comment #8)
> So the issue I've been having is that adding a mochitest-browser-chrome test
> changes the chunking, which is causing one chunk to take 60 minutes on OS X
> debug, which is the timeout limit, so the test run fails. I'm not sure what
> the right solution here is, but as a workaround, I put the new tests in
> browser/base/content/test/siteIdentity/browser_navigation_failures.js for
> now.
> Here's that try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4e41891a510717a4b5c76e282b44ebda64c09cca
> Since OS X tests are backed up about 6 hours, we'll know sometime later
> tonight if that worked.

Ryan, who can extend the timeout limit for the macOS debug test suites on m-c and integration branches + try? Other people are likely to start bumping into this, too.
Flags: needinfo?(ryanvm)
We can either bump the timeout or add more chunks. Deferring to Joel on which is more preferable.
Flags: needinfo?(ryanvm) → needinfo?(jmaher)
I would prefer to add more chunks, we already do 16 on linux- I would vote for 12 here.
Flags: needinfo?(jmaher)
https://hg.mozilla.org/mozilla-central/rev/3a34d4624a1a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: sec-bounty?
Flags: in-testsuite? → in-testsuite+
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 64.0a1 (2018-10-12). Thank you very much. \o/
Status: RESOLVED → VERIFIED
Flags: sec-bounty? → sec-bounty+
Keywords: sec-highsec-moderate
Summary: Some http:// website pages are shown as secure ones after visiting/opening https:// versions after landing patch from bug #832834 → Some http:// website pages (on sites using http/2) are shown as secure ones after visiting/opening https:// versions after landing patch from bug #832834
No longer blocks: 832834
Regressed by: 832834
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.