Skip to content

Commit

Permalink
Bug 1828375 - Do gradual ORB transition. r=sefeng,necko-reviewers
Browse files Browse the repository at this point in the history
Add a separate check for spec breaking allows of certain MIME
types. Having this separated out means that we can make the rest of
the implementation behave exactly like spec.

Some tradeoffs that we need in the current state are:

* Allowing "application/dash+xml"
* Allowing "application/vnd.apple.mpegurl"
* Allowing "text/vtt"
* Allow all MIME types beginning with "audio/mpeg"
* Allow "text/plain" when there is a no-sniff header.

Differential Revision: https://phabricator.services.mozilla.com/D176821
  • Loading branch information
farre committed May 10, 2023
1 parent 2ed8854 commit 5c3d5f0
Show file tree
Hide file tree
Showing 8 changed files with 107 additions and 5 deletions.
1 change: 1 addition & 0 deletions netwerk/mime/nsMimeTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@
#define VIDEO_MATROSKA "video/x-matroska"
#define APPLICATION_OGG "application/ogg"
#define APPLICATION_MPEGURL "application/vnd.apple.mpegurl"
#define APPLICATION_DASH_XML "application/dash+xml"
/* x-uuencode-apple-single. QuickMail made me do this. */
#define UUENCODE_APPLE_SINGLE "x-uuencode-apple-single"
Expand Down
3 changes: 3 additions & 0 deletions netwerk/protocol/http/HttpBaseChannel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3234,6 +3234,9 @@ HttpBaseChannel::PerformOpaqueResponseSafelistCheckBeforeSniff() {
case OpaqueResponseBlockedReason::ALLOWED_SAFE_LISTED:
// Step 3.1
return OpaqueResponse::Allow;
case OpaqueResponseBlockedReason::ALLOWED_SAFE_LISTED_SPEC_BREAKING:
LOGORB("Allowed %s in a spec breaking way", contentType.get());
return OpaqueResponse::Allow;
case OpaqueResponseBlockedReason::BLOCKED_BLOCKLISTED_NEVER_SNIFFED:
return BlockOrFilterOpaqueResponse(
mORB, u"mimeType is an opaque-blocklisted-never-sniffed MIME type"_ns,
Expand Down
34 changes: 33 additions & 1 deletion netwerk/protocol/http/OpaqueResponseUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,31 @@ static bool IsOpaqueSafeListedMIMEType(const nsACString& aContentType) {
return nsContentUtils::IsJavascriptMIMEType(typeString);
}

static bool IsOpaqueSafeListedSpecBreakingMIMEType(
const nsACString& aContentType, bool aNoSniff) {
// Avoid trouble with DASH/HLS. See bug 1698040.
if (aContentType.EqualsLiteral(APPLICATION_DASH_XML) ||
aContentType.EqualsLiteral(APPLICATION_MPEGURL) ||
aContentType.EqualsLiteral(TEXT_VTT)) {
return true;
}

// Do what Chromium does. This is from bug 1828375, and we should ideally
// revert this.
if (aContentType.EqualsLiteral(TEXT_PLAIN) && aNoSniff) {
return true;
}

// This is not a good solution, but we need this until we solve Bug 1827684 in
// a better way. Chromium currently allows all "audio/*" and "video/*", but
// from discussion in bug, we want to try only "audio/mpeg".
if (StringBeginsWith(aContentType, "audio/mpeg"_ns)) {
return true;
}

return false;
}

static bool IsOpaqueBlockListedMIMEType(const nsACString& aContentType) {
return aContentType.EqualsLiteral(TEXT_HTML) ||
StringEndsWith(aContentType, "+json"_ns) ||
Expand Down Expand Up @@ -89,7 +114,8 @@ static bool IsOpaqueBlockListedNeverSniffedMIMEType(
aContentType.EqualsLiteral(MULTIPART_SIGNED) ||
aContentType.EqualsLiteral(TEXT_EVENT_STREAM) ||
aContentType.EqualsLiteral(TEXT_CSV) ||
aContentType.EqualsLiteral(TEXT_VTT);
aContentType.EqualsLiteral(TEXT_VTT) ||
aContentType.EqualsLiteral(APPLICATION_DASH_XML);
}

OpaqueResponseBlockedReason GetOpaqueResponseBlockedReason(
Expand All @@ -102,6 +128,12 @@ OpaqueResponseBlockedReason GetOpaqueResponseBlockedReason(
return OpaqueResponseBlockedReason::ALLOWED_SAFE_LISTED;
}

// For some MIME types we deviate from spec and allow when we ideally
// shouldn't. These are returnened before any blocking takes place.
if (IsOpaqueSafeListedSpecBreakingMIMEType(aContentType, aNoSniff)) {
return OpaqueResponseBlockedReason::ALLOWED_SAFE_LISTED_SPEC_BREAKING;
}

if (IsOpaqueBlockListedNeverSniffedMIMEType(aContentType)) {
return OpaqueResponseBlockedReason::BLOCKED_BLOCKLISTED_NEVER_SNIFFED;
}
Expand Down
1 change: 1 addition & 0 deletions netwerk/protocol/http/OpaqueResponseUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class nsHttpResponseHead;

enum class OpaqueResponseBlockedReason : uint32_t {
ALLOWED_SAFE_LISTED,
ALLOWED_SAFE_LISTED_SPEC_BREAKING,
BLOCKED_BLOCKLISTED_NEVER_SNIFFED,
BLOCKED_206_AND_BLOCKLISTED,
BLOCKED_NOSNIFF_AND_EITHER_BLOCKLISTED_OR_TEXTPLAIN,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[img-mime-types-coverage.tentative.sub.html]
[ORB should block the response if Content-Type is: 'application/dash+xml'. ]
expected: FAIL

[ORB should block the response if Content-Type is: 'application/vnd.apple.mpegurl'. ]
expected: FAIL

[ORB should block the response if Content-Type is: 'audio/mpegurl'. ]
expected: FAIL

[ORB should block the response if Content-Type is: 'text/vtt'. ]
expected: FAIL
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<!-- Test verifies that cross-origin, nosniff images are 1) blocked when their
MIME type is covered by ORB and 2) allowed otherwise.
This test is very similar to fetch/orb/img-mime-types-coverage.tentative.sub.html,
except that it focuses on MIME types relevant to ORB.
-->
<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>
<div id=log></div>
<script>
var passes = [
// These are exceptions that allow more MIME types than the ORB spec does.
// This is due to web compat, but might be removed in the future.
// See Bug 1828375
"application/dash+xml",
"application/vnd.apple.mpegurl",
"audio/mpegurl",
"audio/mpeg",
"text/vtt",
]

const get_url = (mime) => {
// www1 is cross-origin, so the HTTP response is ORB-eligible -->
url = "http://{{domains[www1]}}:{{ports[http][0]}}"
url = url + "/fetch/nosniff/resources/image.py"
if (mime != null) {
url += "?type=" + encodeURIComponent(mime)
}
return url
}

passes.forEach(function (mime) {
async_test(function (t) {
var img = document.createElement("img")
img.onerror = t.unreached_func("Unexpected error event")
img.onload = t.step_func_done(function () {
assert_equals(img.width, 96)
})
img.src = get_url(mime)
document.body.appendChild(img)
}, "ORB should allow the response if Content-Type is: '" + mime + "'. ")
})
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,20 @@

const path = "http://{{domains[www1]}}:{{ports[http][0]}}/fetch/orb/resources";

// This is an exception that allow more MIME types than the ORB spec does.
// This is due to web compatibility, but might be removed in the future.
// See Bug 1828375
promise_test(
async () =>
await fetchORB(
`${path}/text.txt`,
null,
contentType("text/plain"),
contentTypeOptions("nosniff")
),
"ORB shouldn't block opaque text/plain with nosniff"
);

// Due to web compatibility we filter opaque Response object from the
// fetch() function in the Fetch specification. See Bug 1823877. This
// might be removed in the future.
Expand Down

0 comments on commit 5c3d5f0

Please sign in to comment.