Closed Bug 1247807 Opened 4 years ago Closed 4 years ago

Mixed Content UI broken when using CSP directive 'upgrade-insecure-requests'

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → mozilla
Blocks: 663570
Status: NEW → ASSIGNED
We introduced the problem with Bug 663570 (Implement Meta CSP) because for Meta CSP we had to distinguish between preloads and actual loads, because the meta CSP might have been applied only speculatively. Before Meta CSP, we didn't have to distinguish between preloads and actual loads for upgrade-insecure-requests within mixedContentBlocker.

We are performing the right upgrade from http to https within the backend [1,2,3], because we are relying on logic within the loadInfo. We even have good testcoverage for that (csp/test_upgrade_insecure.html) where we make sure that no *http* requests hits the server.

Within nsMixedContentBlocker [4] however, we are relying on logic that lives on the document. We are performing the right check, but unfortunately mUpgradeInsecurePreloads holds the wrong value on the document.

Problem is, that if a CSP is delivered through the header we *always* have to upgrade from http to https. Not depending on any speculatively applied CSP.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#2269
[2] http://mxr.mozilla.org/mozilla-aurora/source/netwerk/base/nsNetUtil.cpp#2269
[3] http://mxr.mozilla.org/mozilla-beta/source/netwerk/protocol/http/nsHttpChannel.cpp#339
[4] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#670
Attachment #8718648 - Flags: review?(bzbarsky)
Comment on attachment 8718648 [details] [diff] [review]
bug_1247807_mixed_content_ui_bug.patch

>+      mUpgradeInsecurePreloads = mUpgradeInsecureRequests;

Shouldn't this only happen if !mUpgradeInsecurePreloads?  That is, if it's already true I don't think we should be changing it here...

If it _can't_ be true, there should be a comment explaining why.

r=me with that.
Attachment #8718648 - Flags: review?(bzbarsky) → review+
In fact it should not be possible, but nevertheless I agree with you. Let's play it safe and only set it if |!mUpgradeInsecurePreloads?|. Thanks Boris!
Attachment #8718648 - Attachment is obsolete: true
Attachment #8719047 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/60f8b2bcbf46
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8719047 [details] [diff] [review]
bug_1247807_mixed_content_ui_bug.patch

Approval Request Comment
Since the introduction of meta CSP (Bug 663570) we show a broken mixed content UI if the directive 'upgrade-insecure-requests' is used. The backend (security wise) is working as exptected - we have a wide test coverage for that (see e.g. csp/test_upgrade_insecure.html).

[Feature/regressing bug #]:
Bug 663570

[User impact if declined]:
As described above, users might be confused because the see a broken UI even though the upgrade from http to https is performed correctly.

[Describe test coverage new/current, TreeHerder]:
Manual testing for the UI, backend is covered by test_upgrade_insecure.html

[Risks and why]:
Low, only affects pages that use the CSP directive 'upgrade-insecure-requests'.

[String/UUID change made/needed]:
No.
Attachment #8719047 - Flags: approval-mozilla-beta?
Attachment #8719047 - Flags: approval-mozilla-aurora?
Comment on attachment 8719047 [details] [diff] [review]
bug_1247807_mixed_content_ui_bug.patch

Small change, improve the feature, taking it.
Should be in 45 beta 7
Attachment #8719047 - Flags: approval-mozilla-beta?
Attachment #8719047 - Flags: approval-mozilla-beta+
Attachment #8719047 - Flags: approval-mozilla-aurora?
Attachment #8719047 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.