From c30d379838759230edc8dc757dba31acba7f290f Mon Sep 17 00:00:00 2001 From: Agi Sferro Date: Thu, 15 Oct 2020 00:43:22 +0000 Subject: [PATCH] Bug 1667471 - Add header filter to GeckoSession.loader. r=snorp,esawin This allows apps to modify the header filtering behavior, and introduces a safe default (CORS safelisted). Deprecated `loadUri` methods still maintain the old behavior so that we don't inadvertently introduce bugs in apps. Differential Revision: https://phabricator.services.mozilla.com/D91983 --- mobile/android/geckoview/api.txt | 3 + .../geckoview/test/NavigationDelegateTest.kt | 102 +++++++++++++++--- .../org/mozilla/geckoview/GeckoSession.java | 61 ++++++++++- .../mozilla/geckoview/doc-files/CHANGELOG.md | 6 +- .../modules/geckoview/GeckoViewNavigation.jsm | 58 ++++++---- 5 files changed, 195 insertions(+), 35 deletions(-) diff --git a/mobile/android/geckoview/api.txt b/mobile/android/geckoview/api.txt index 3147eb222147..50a49a8fec42 100644 --- a/mobile/android/geckoview/api.txt +++ b/mobile/android/geckoview/api.txt @@ -777,6 +777,8 @@ package org.mozilla.geckoview { field public static final int FINDER_FIND_LINKS_ONLY = 8; field public static final int FINDER_FIND_MATCH_CASE = 2; field public static final int FINDER_FIND_WHOLE_WORD = 4; + field public static final int HEADER_FILTER_CORS_SAFELISTED = 1; + field public static final int HEADER_FILTER_UNRESTRICTED_UNSAFE = 2; field public static final int LOAD_FLAGS_ALLOW_POPUPS = 8; field public static final int LOAD_FLAGS_BYPASS_CACHE = 1; field public static final int LOAD_FLAGS_BYPASS_CLASSIFIER = 16; @@ -859,6 +861,7 @@ package org.mozilla.geckoview { method @NonNull public GeckoSession.Loader data(@NonNull byte[], @Nullable String); method @NonNull public GeckoSession.Loader data(@NonNull String, @Nullable String); method @NonNull public GeckoSession.Loader flags(int); + method @NonNull public GeckoSession.Loader headerFilter(int); method @NonNull public GeckoSession.Loader referrer(@NonNull GeckoSession); method @NonNull public GeckoSession.Loader referrer(@NonNull Uri); method @NonNull public GeckoSession.Loader referrer(@NonNull String); diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt index a0a01d13842c..ab2c273cc294 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt @@ -1372,7 +1372,8 @@ class NavigationDelegateTest : BaseSessionTest() { } private fun loadUriHeaderTest(headers: Map, - additional: Map) { + additional: Map, + filter: Int = GeckoSession.HEADER_FILTER_CORS_SAFELISTED) { // First collect default headers with no override sessionRule.session.loadUri("$TEST_ENDPOINT/anything") sessionRule.session.waitForPageStop() @@ -1381,26 +1382,40 @@ class NavigationDelegateTest : BaseSessionTest() { val defaultBody = JSONObject(defaultContent) val defaultHeaders = defaultBody.getJSONObject("headers").asMap() - val expected = defaultHeaders.plus(additional) + val expected = HashMap(additional) + for (key in defaultHeaders.keys) { + expected[key] = defaultHeaders[key] + if (additional.containsKey(key)) { + // TODO: Bug 1671294, headers should be replaced, not appended + expected[key] += ", " + additional[key] + } + } // Now load the page with the header override sessionRule.session.load(Loader() .uri("$TEST_ENDPOINT/anything") - .additionalHeaders(headers)) + .additionalHeaders(headers) + .headerFilter(filter)) sessionRule.session.waitForPageStop() val content = sessionRule.session.evaluateJS("document.body.children[0].innerHTML") as String val body = JSONObject(content) val actualHeaders = body.getJSONObject("headers").asMap() - assertThat("Headers should match", actualHeaders, equalTo(expected)) + assertThat("Headers should match", expected as Map, + equalTo(actualHeaders)) } @Test fun loadUriHeader() { // Basic test loadUriHeaderTest( mapOf("Header1" to "Value", "Header2" to "Value1, Value2"), - mapOf("Header1" to "Value", "Header2" to "Value1, Value2") + mapOf() + ) + loadUriHeaderTest( + mapOf("Header1" to "Value", "Header2" to "Value1, Value2"), + mapOf("Header1" to "Value", "Header2" to "Value1, Value2"), + GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE ) // Empty value headers are ignored @@ -1426,16 +1441,34 @@ class NavigationDelegateTest : BaseSessionTest() { "what" to "what\r\nhost:amazon.com", "Header3" to "Value1, Value2, Value3" ), + mapOf() + ) + loadUriHeaderTest( mapOf("Header1" to "Value", - "Header2" to "Value1, Value2", - "Header3" to "Value1, Value2, Value3") + "Header2" to "Value1, Value2", + "this\r\nis invalid" to "test value", + "test key" to "this\r\n is a no-no", + "what" to "what\r\nhost:amazon.com", + "Header3" to "Value1, Value2, Value3" + ), + mapOf("Header1" to "Value", + "Header2" to "Value1, Value2", + "Header3" to "Value1, Value2, Value3"), + GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE ) + loadUriHeaderTest( + mapOf("Header1" to "Value", + "Header2" to "Value1, Value2", + "what" to "what\r\nhost:amazon.com"), + mapOf() + ) loadUriHeaderTest( mapOf("Header1" to "Value", "Header2" to "Value1, Value2", "what" to "what\r\nhost:amazon.com"), - mapOf("Header1" to "Value", "Header2" to "Value1, Value2") + mapOf("Header1" to "Value", "Header2" to "Value1, Value2"), + GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE ) loadUriHeaderTest( @@ -1451,22 +1484,42 @@ class NavigationDelegateTest : BaseSessionTest() { // Connection and Host cannot be overriden, no matter the case spelling loadUriHeaderTest( mapOf("Header1" to "Value1", "ConnEction" to "test", "connection" to "test2"), - mapOf("Header1" to "Value1") + mapOf() + ) + loadUriHeaderTest( + mapOf("Header1" to "Value1", "ConnEction" to "test", "connection" to "test2"), + mapOf("Header1" to "Value1"), + GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE ) loadUriHeaderTest( mapOf("Header1" to "Value1", "connection" to "test2"), - mapOf("Header1" to "Value1") + mapOf() + ) + loadUriHeaderTest( + mapOf("Header1" to "Value1", "connection" to "test2"), + mapOf("Header1" to "Value1"), + GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE ) loadUriHeaderTest( mapOf("Header1 " to "Value1", "host" to "test2"), - mapOf("Header1" to "Value1") + mapOf() + ) + loadUriHeaderTest( + mapOf("Header1 " to "Value1", "host" to "test2"), + mapOf("Header1" to "Value1"), + GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE ) loadUriHeaderTest( mapOf("Header1" to "Value1", "host" to "test2"), - mapOf("Header1" to "Value1") + mapOf() + ) + loadUriHeaderTest( + mapOf("Header1" to "Value1", "host" to "test2"), + mapOf("Header1" to "Value1"), + GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE ) // Adding white space at the end of a forbidden header still prevents override @@ -1483,6 +1536,31 @@ class NavigationDelegateTest : BaseSessionTest() { mapOf("abc\ra\n" to "amazon.com"), mapOf() ) + + // CORS Safelist test + loadUriHeaderTest( + mapOf("Accept-Language" to "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5", + "Accept" to "text/html", + "Content-Language" to "de-DE, en-CA", + "Content-Type" to "multipart/form-data; boundary=something"), + mapOf("Accept-Language" to "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5", + "Accept" to "text/html", + "Content-Language" to "de-DE, en-CA", + "Content-Type" to "multipart/form-data; boundary=something"), + GeckoSession.HEADER_FILTER_CORS_SAFELISTED + ) + + // CORS safelist doesn't allow Content-type image/svg + loadUriHeaderTest( + mapOf("Accept-Language" to "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5", + "Accept" to "text/html", + "Content-Language" to "de-DE, en-CA", + "Content-Type" to "image/svg; boundary=something"), + mapOf("Accept-Language" to "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5", + "Accept" to "text/html", + "Content-Language" to "de-DE, en-CA"), + GeckoSession.HEADER_FILTER_CORS_SAFELISTED + ) } @Test(expected = GeckoResult.UncaughtException::class) diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java index 45a8fed55c07..e61488b0deb5 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java @@ -1460,6 +1460,32 @@ public class GeckoSession { */ public static final int LOAD_FLAGS_REPLACE_HISTORY = 1 << 6; + /** + * Filter headers according to the CORS safelisted rules. + * + * See + * CORS-safelisted request header + * . + */ + public static final int HEADER_FILTER_CORS_SAFELISTED = 1; + /** + * Allows most headers. + * + * Note: the Host and Connection + * headers are still ignored. + * + * This should only be used when input is hard-coded from the app or when + * properly sanitized, as some headers could cause unexpected consequences + * and security issues. + * + * Only use this if you know what you're doing. + */ + public static final int HEADER_FILTER_UNRESTRICTED_UNSAFE = 2; + + @Retention(RetentionPolicy.SOURCE) + @IntDef(value = {HEADER_FILTER_CORS_SAFELISTED, HEADER_FILTER_UNRESTRICTED_UNSAFE}) + /* package */ @interface HeaderFilter {} + /** * Main entry point for loading URIs into a {@link GeckoSession}. * @@ -1502,6 +1528,7 @@ public class GeckoSession { private GeckoBundle mHeaders; private @LoadFlags int mLoadFlags = LOAD_FLAGS_NONE; private boolean mIsDataUri; + private @HeaderFilter int mHeaderFilter = HEADER_FILTER_CORS_SAFELISTED; private static @NonNull String createDataUri(@NonNull final byte[] bytes, @Nullable final String mimeType) { @@ -1603,8 +1630,12 @@ public class GeckoSession { /** * Add headers for this load. * - * Note: the Host and Connection - * headers are ignored. + * Note: only CORS safelisted headers are allowed by default. To modify this + * behavior use {@link #headerFilter}. + * + * See + * CORS-safelisted request header + * . * * @param headers a Map containing headers that will * be added to this load. @@ -1624,6 +1655,20 @@ public class GeckoSession { return this; } + /** + * Modify the header filter behavior. By default only CORS safelisted headers are allowed. + * + * @param filter one of the + * {@link GeckoSession#HEADER_FILTER_CORS_SAFELISTED HEADER_FILTER_*} + * constants. + * @return this {@link Loader} instance. + */ + @NonNull + public Loader headerFilter(final @HeaderFilter int filter) { + mHeaderFilter = filter; + return this; + } + /** * Set the load flags for this load. * @param flags the load flags to use, an OR-ed value of @@ -1679,6 +1724,7 @@ public class GeckoSession { final GeckoBundle msg = new GeckoBundle(); msg.putString("uri", request.mUri); msg.putInt("flags", loadFlags); + msg.putInt("headerFilter", request.mHeaderFilter); if (request.mReferrerUri != null) { msg.putString("referrerUri", request.mReferrerUri); @@ -1721,6 +1767,7 @@ public class GeckoSession { public void loadUri(final @NonNull String uri, final @Nullable Map additionalHeaders) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .additionalHeaders(additionalHeaders)); } @@ -1736,6 +1783,7 @@ public class GeckoSession { public void loadUri(final @NonNull String uri, final @LoadFlags int flags) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .flags(flags)); } @@ -1753,6 +1801,7 @@ public class GeckoSession { final @LoadFlags int flags) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .referrer(referrer) .flags(flags)); } @@ -1772,6 +1821,7 @@ public class GeckoSession { final @LoadFlags int flags, final @Nullable Map additionalHeaders) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .referrer(referrer) .flags(flags) .additionalHeaders(additionalHeaders)); @@ -1793,6 +1843,7 @@ public class GeckoSession { final @LoadFlags int flags) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .referrer(referrer) .flags(flags)); } @@ -1814,6 +1865,7 @@ public class GeckoSession { final @LoadFlags int flags, final @Nullable Map additionalHeaders) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .referrer(referrer) .additionalHeaders(additionalHeaders) .flags(flags)); @@ -1853,6 +1905,7 @@ public class GeckoSession { @Deprecated public void loadUri(final @NonNull Uri uri) { load(new Loader() + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .uri(uri)); } @@ -1867,6 +1920,7 @@ public class GeckoSession { public void loadUri(final @NonNull Uri uri, final @Nullable Map additionalHeaders) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .additionalHeaders(additionalHeaders)); } @@ -1881,6 +1935,7 @@ public class GeckoSession { public void loadUri(final @NonNull Uri uri, final @LoadFlags int flags) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .flags(flags)); } @@ -1897,6 +1952,7 @@ public class GeckoSession { final @LoadFlags int flags) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .referrer(referrer) .flags(flags)); } @@ -1915,6 +1971,7 @@ public class GeckoSession { final @LoadFlags int flags, final @Nullable Map additionalHeaders) { load(new Loader() .uri(uri) + .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE) .referrer(referrer) .flags(flags) .additionalHeaders(additionalHeaders)); diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md index e7445eb48b6b..12ec1b794020 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md @@ -34,6 +34,9 @@ exclude: true - ⚠️ Deprecated [`GeckoSession#loadUri`][83.6] variants in favor of [`GeckoSession#load`][83.7]. See docs for [`Loader`][83.8]. ([bug 1667471]({{bugzilla}}1667471)) +- Added [`Loader#headerFilter`][83.9] to override the default header filtering + behavior. + ([bug 1667471]({{bugzilla}}1667471)) [83.1]: {{javadoc_uri}}/WebExtension.MetaData.html#temporary [83.2]: {{javadoc_uri}}/MediaSession.Delegate.html#onMetadata-org.mozilla.geckoview.GeckoSession-org.mozilla.geckoview.MediaSession-org.mozilla.geckoview.MediaSession.Metadata- @@ -43,6 +46,7 @@ exclude: true [83.6]: {{javadoc_uri}}/GeckoSession.html#loadUri-java.lang.String-org.mozilla.geckoview.GeckoSession-int-java.util.Map- [83.7]: {{javadoc_uri}}/GeckoSession.html#load-org.mozilla.geckoview.GeckoSession.Loader- [83.8]: {{javadoc_uri}}/GeckoSession.Loader.html +[83.9]: {{javadoc_uri}}/GeckoSession.Loader.html#headerFilter-int- ## v82 - ⚠️ [`WebNotification.source`][79.2] is now `@Nullable` to account for @@ -832,4 +836,4 @@ to allow adding gecko profiler markers. [65.24]: {{javadoc_uri}}/CrashReporter.html#sendCrashReport-android.content.Context-android.os.Bundle-java.lang.String- [65.25]: {{javadoc_uri}}/GeckoResult.html -[api-version]: 0abbabc4fa9445edc2f37ba174b189be25c2b2fa +[api-version]: cd2ba68c049db26ce20ba332ef2ba46cbde4910a diff --git a/mobile/android/modules/geckoview/GeckoViewNavigation.jsm b/mobile/android/modules/geckoview/GeckoViewNavigation.jsm index bbad3d200fee..ecaadfdcd07c 100644 --- a/mobile/android/modules/geckoview/GeckoViewNavigation.jsm +++ b/mobile/android/modules/geckoview/GeckoViewNavigation.jsm @@ -37,24 +37,9 @@ const BAD_HEADERS = ["connection", "host"]; // in the header name or value const FORBIDDEN_HEADER_CHARACTERS = ["\n", "\r"]; -function validateHeader(key, value) { - if (!key) { - // Key cannot be empty - return false; - } - - for (const c of FORBIDDEN_HEADER_CHARACTERS) { - if (key.includes(c) || value?.includes(c)) { - return false; - } - } - - if (BAD_HEADERS.includes(key.toLowerCase().trim())) { - return false; - } - - return true; -} +// Keep in sync with GeckoSession.java +const HEADER_FILTER_CORS_SAFELISTED = 1; +const HEADER_FILTER_UNRESTRICTED_UNSAFE = 2; // Create default ReferrerInfo instance for the given referrer URI string. const createReferrerInfo = aReferrer => { @@ -143,6 +128,32 @@ class GeckoViewNavigation extends GeckoViewModule { this._initialAboutBlank = true; } + validateHeader(key, value, filter) { + if (!key) { + // Key cannot be empty + return false; + } + + for (const c of FORBIDDEN_HEADER_CHARACTERS) { + if (key.includes(c) || value?.includes(c)) { + return false; + } + } + + if (BAD_HEADERS.includes(key.toLowerCase().trim())) { + return false; + } + + if ( + filter == HEADER_FILTER_CORS_SAFELISTED && + !this.window.windowUtils.isCORSSafelistedRequestHeader(key, value) + ) { + return false; + } + + return true; + } + // Bundle event handler. async onEvent(aEvent, aData, aCallback) { debug`onEvent: event=${aEvent}, data=${aData}`; @@ -158,7 +169,14 @@ class GeckoViewNavigation extends GeckoViewModule { this.browser.gotoIndex(aData.index); break; case "GeckoView:LoadUri": - const { uri, referrerUri, referrerSessionId, flags, headers } = aData; + const { + uri, + referrerUri, + referrerSessionId, + flags, + headers, + headerFilter, + } = aData; let navFlags = convertFlags(flags); // For performance reasons we don't call the LoadUriDelegate.loadUri @@ -215,7 +233,7 @@ class GeckoViewNavigation extends GeckoViewModule { if (headers) { additionalHeaders = ""; for (const [key, value] of Object.entries(headers)) { - if (!validateHeader(key, value)) { + if (!this.validateHeader(key, value, headerFilter)) { Cu.reportError(`Ignoring invalid header '${key}'='${value}'.`); continue; }