1971704 - Block downloads in sandbox before checking for potentially unsafe downloads

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Tom Schuster]

https://searchfox.org/mozilla-central/rev/0fe159e3c1a09d9cd22b0ceadbe01efc7e8fd788/dom/security/nsContentSecurityUtils.cpp#2242-2248 on the other hand looks worrisome. Why is that before the sandbox check.

bug 1969304 comment 4

We shouldn't even try to check if a download is potentially insecure due to mixed content, we should just block it if the sandbox flags don't allow it.

Currently we also allow downloads triggered by the system principal to bypass the sandbox flags, but not the mixed content blocking. I am not sure if that is really intentional either. It seems backwards to me?

Comment 1

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1558394

Comment 2

[Tom Schuster]

Attachment: Bug 1971704 - Cleanup nsContentSecurityUtils::ClassifyDownload. r?smaug

Comment 3

[Tom Schuster]

Not sure about the rating, here? Maybe sec-low?

Need-info myself so I don't forget to write a test for this.

Comment 4

[Tom Schuster]

Attachment: Bug 1971704 - Rename SANDBOXED_ALLOW_DOWNLOADS to SANDBOXED_DOWNLOADS. r?smaug

Currently, it sounds like the flag means "allow downloads", but if the
flag is set, we should disallow downloads. This aligns with the naming scheme of the other sandboxing flags.

Comment 5

[Phabricator Automation]

Comment on attachment 9494376 [details]
Bug 1971704 - Rename SANDBOXED_ALLOW_DOWNLOADS to SANDBOXED_DOWNLOADS. r?smaug

Revision D253526 was moved to bug 1971805. Setting attachment 9494376 [details] to obsolete.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d7d7fa4d3e0d
https://hg.mozilla.org/integration/autoland/rev/46f7b4cf0509
Cleanup nsContentSecurityUtils::ClassifyDownload. r=smaug

Comment 7

[Tom Schuster]

Landed:
https://hg.mozilla.org/mozilla-central/rev/46f7b4cf0509

Comment 8

[:Gijs (he/him)]

(In reply to Tom Schuster (MoCo) from comment #0)

Currently we also allow downloads triggered by the system principal to bypass the sandbox flags, but not the mixed content blocking. I am not sure if that is really intentional either. It seems backwards to me?

I don't know where this check lives, but fwiw bug 1668530 (and its sibling bugs around uBlock Origin and similar mechanisms, as well as network failures) may be relevant to your interests?

Comment 9

[Tom Schuster]

We ended up removing this check here: https://hg-edge.mozilla.org/mozilla-central/rev/46f7b4cf0509#l1.44. There really shouldn't be a reason for the system principal to bypass the sandbox flags here. It seems like at some point the code was meant to allow the system principal to bypass the mixed content check, but that was broken by some refactoring.

Comment 10

[Tom Schuster]

I have tried writing a test for this, but haven't succeeded so far. Usually it seems like we would already block mixed content downloads from inside iframes via nsMixedContentBlocker::ShouldLoad before ever reaching nsContentSecurityUtils::ClassifyDownload.

Comment 11

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1558394

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for ESR140/ESR128 uplift?

Comment 13

[Tom Schuster]

I don't think we need to uplift this. It's not even 100% clear if this is an actual issue.