From 9de06fdbc504e3f35d15de73809d87732dfae2e4 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 29 Aug 2022 11:55:35 +0200 Subject: [PATCH 1/3] fix: crash loading non-standard schemes in iframes --- patches/chromium/.patches | 1 + ...ding_non-standard_schemes_in_iframes.patch | 78 +++++++++++++++++++ spec-main/api-protocol-spec.ts | 23 +++++- spec/fixtures/pages/iframe-protocol.html | 11 +++ 4 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 patches/chromium/fix_crash_loading_non-standard_schemes_in_iframes.patch create mode 100644 spec/fixtures/pages/iframe-protocol.html diff --git a/patches/chromium/.patches b/patches/chromium/.patches index d53c82faf37f4..958972ef5a076 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -118,3 +118,4 @@ revert_spellcheck_fully_launch_spell_check_delayed_initialization.patch add_electron_deps_to_license_credits_file.patch feat_add_set_can_resize_mutator.patch fix_revert_emulationhandler_update_functions_to_early_return.patch +fix_crash_loading_non-standard_schemes_in_iframes.patch diff --git a/patches/chromium/fix_crash_loading_non-standard_schemes_in_iframes.patch b/patches/chromium/fix_crash_loading_non-standard_schemes_in_iframes.patch new file mode 100644 index 0000000000000..39874d41f490c --- /dev/null +++ b/patches/chromium/fix_crash_loading_non-standard_schemes_in_iframes.patch @@ -0,0 +1,78 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Shelley Vohr +Date: Mon, 29 Aug 2022 11:44:57 +0200 +Subject: fix: crash loading non-standard schemes in iframes + +This fixes a crash that occurs when loading non-standard schemes from +iframes or webviews. This was happening because +ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin contains explicit +exceptions to allow built-in non-standard schemes, but does not check +for non-standard schemes registered by the embedder. + +Upstream, https://bugs.chromium.org/p/chromium/issues/detail?id=1081397 +contains several paths forward - here I chose to swap out the +CHECK in navigation_request.cc from policy->CanAccessDataForOrigin to +policy->CanCommitOriginAndUrl. + +Upstreamed at https://chromium-review.googlesource.com/c/chromium/src/+/3856266. + +diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc +index b1120c9270a255e4b6070c0d2b5f49e6f99d935d..407888cb17d3e609ad7aaa2ed1c079642df60bca 100644 +--- a/content/browser/renderer_host/navigation_request.cc ++++ b/content/browser/renderer_host/navigation_request.cc +@@ -6517,10 +6517,11 @@ std::pair NavigationRequest:: + if (IsForMhtmlSubframe()) + return origin_with_debug_info; + +- int process_id = GetRenderFrameHost()->GetProcess()->GetID(); +- auto* policy = ChildProcessSecurityPolicyImpl::GetInstance(); +- CHECK( +- policy->CanAccessDataForOrigin(process_id, origin_with_debug_info.first)); ++ CanCommitStatus can_commit = GetRenderFrameHost()->CanCommitOriginAndUrl( ++ origin_with_debug_info.first, GetURL(), IsSameDocument(), IsPdf(), ++ GetUrlInfo().is_sandboxed); ++ CHECK_EQ(CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL, can_commit); ++ + return origin_with_debug_info; + } + +diff --git a/content/browser/renderer_host/render_frame_host_impl.h b/content/browser/renderer_host/render_frame_host_impl.h +index b1259618ff4dc1848cf007d853e6f3e70b08829f..3d4e6c4c7e0348c52c3f0ccd0f6c19e0e1ec15a5 100644 +--- a/content/browser/renderer_host/render_frame_host_impl.h ++++ b/content/browser/renderer_host/render_frame_host_impl.h +@@ -2550,6 +2550,17 @@ class CONTENT_EXPORT RenderFrameHostImpl + HandleAXEvents(tree_id, std::move(updates_and_events), reset_token); + } + ++ // Returns whether the given origin and URL is allowed to commit in the ++ // current RenderFrameHost. The |url| is used to ensure it matches the origin ++ // in cases where it is applicable. This is a more conservative check than ++ // RenderProcessHost::FilterURL, since it will be used to kill processes that ++ // commit unauthorized origins. ++ CanCommitStatus CanCommitOriginAndUrl(const url::Origin& origin, ++ const GURL& url, ++ bool is_same_document_navigation, ++ bool is_pdf, ++ bool is_sandboxed); ++ + protected: + friend class RenderFrameHostFactory; + +@@ -2879,17 +2890,6 @@ class CONTENT_EXPORT RenderFrameHostImpl + // relevant. + void ResetWaitingState(); + +- // Returns whether the given origin and URL is allowed to commit in the +- // current RenderFrameHost. The |url| is used to ensure it matches the origin +- // in cases where it is applicable. This is a more conservative check than +- // RenderProcessHost::FilterURL, since it will be used to kill processes that +- // commit unauthorized origins. +- CanCommitStatus CanCommitOriginAndUrl(const url::Origin& origin, +- const GURL& url, +- bool is_same_document_navigation, +- bool is_pdf, +- bool is_sandboxed); +- + // Returns whether a subframe navigation request should be allowed to commit + // to the current RenderFrameHost. + bool CanSubframeCommitOriginAndUrl(NavigationRequest* navigation_request); diff --git a/spec-main/api-protocol-spec.ts b/spec-main/api-protocol-spec.ts index 980c289cb1e7f..7988724baf5b4 100644 --- a/spec-main/api-protocol-spec.ts +++ b/spec-main/api-protocol-spec.ts @@ -9,7 +9,7 @@ import * as fs from 'fs'; import * as qs from 'querystring'; import * as stream from 'stream'; import { EventEmitter } from 'events'; -import { closeWindow } from './window-helpers'; +import { closeAllWindows, closeWindow } from './window-helpers'; import { emittedOnce } from './events-helpers'; import { WebmGenerator } from './video-helpers'; import { delay } from './spec-helpers'; @@ -216,6 +216,8 @@ describe('protocol module', () => { const normalPath = path.join(fixturesPath, 'pages', 'a.html'); const normalContent = fs.readFileSync(normalPath); + afterEach(closeAllWindows); + it('sends file path as response', async () => { registerFileProtocol(protocolName, (request, callback) => callback(filePath)); const r = await ajax(protocolName + '://fake-host'); @@ -239,6 +241,25 @@ describe('protocol module', () => { expect(r.headers).to.have.property('x-great-header', 'sogreat'); }); + it('can load iframes with custom protocols', (done) => { + registerFileProtocol('custom', (request, callback) => { + const filename = request.url.substring(9); + const p = path.join(__dirname, 'fixtures', 'pages', filename); + callback({ path: p }); + }); + + const w = new BrowserWindow({ + show: false, + webPreferences: { + nodeIntegration: true, + contextIsolation: false + } + }); + + w.loadFile(path.join(__dirname, 'fixtures', 'pages', 'iframe-protocol.html')); + ipcMain.once('loaded-iframe-custom-protocol', () => done()); + }); + it.skip('throws an error when custom headers are invalid', (done) => { registerFileProtocol(protocolName, (request, callback) => { expect(() => callback({ diff --git a/spec/fixtures/pages/iframe-protocol.html b/spec/fixtures/pages/iframe-protocol.html new file mode 100644 index 0000000000000..a283115b19382 --- /dev/null +++ b/spec/fixtures/pages/iframe-protocol.html @@ -0,0 +1,11 @@ + + + + From 188c2feaffeeaba9285082da877df4dda4c8bcb7 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 6 Sep 2022 12:28:28 +0200 Subject: [PATCH 2/3] test: move fixture to correct location --- {spec => spec-main}/fixtures/pages/iframe-protocol.html | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename {spec => spec-main}/fixtures/pages/iframe-protocol.html (100%) diff --git a/spec/fixtures/pages/iframe-protocol.html b/spec-main/fixtures/pages/iframe-protocol.html similarity index 100% rename from spec/fixtures/pages/iframe-protocol.html rename to spec-main/fixtures/pages/iframe-protocol.html From 7c8daf837950280acb097e03209c653f33aad8b5 Mon Sep 17 00:00:00 2001 From: PatchUp <73610968+patchup[bot]@users.noreply.github.com> Date: Tue, 13 Sep 2022 23:45:29 +0000 Subject: [PATCH 3/3] chore: update patches --- ...crash_loading_non-standard_schemes_in_iframes.patch | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/patches/chromium/fix_crash_loading_non-standard_schemes_in_iframes.patch b/patches/chromium/fix_crash_loading_non-standard_schemes_in_iframes.patch index 39874d41f490c..84d7eb40925bc 100644 --- a/patches/chromium/fix_crash_loading_non-standard_schemes_in_iframes.patch +++ b/patches/chromium/fix_crash_loading_non-standard_schemes_in_iframes.patch @@ -17,10 +17,10 @@ policy->CanCommitOriginAndUrl. Upstreamed at https://chromium-review.googlesource.com/c/chromium/src/+/3856266. diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc -index b1120c9270a255e4b6070c0d2b5f49e6f99d935d..407888cb17d3e609ad7aaa2ed1c079642df60bca 100644 +index 37434a26db44ed035fcbebd9febbda10efa859da..060b310d38db85944e37b8a202493212106d8946 100644 --- a/content/browser/renderer_host/navigation_request.cc +++ b/content/browser/renderer_host/navigation_request.cc -@@ -6517,10 +6517,11 @@ std::pair NavigationRequest:: +@@ -6573,10 +6573,11 @@ std::pair NavigationRequest:: if (IsForMhtmlSubframe()) return origin_with_debug_info; @@ -37,10 +37,10 @@ index b1120c9270a255e4b6070c0d2b5f49e6f99d935d..407888cb17d3e609ad7aaa2ed1c07964 } diff --git a/content/browser/renderer_host/render_frame_host_impl.h b/content/browser/renderer_host/render_frame_host_impl.h -index b1259618ff4dc1848cf007d853e6f3e70b08829f..3d4e6c4c7e0348c52c3f0ccd0f6c19e0e1ec15a5 100644 +index 6aff64db8cc09f95d658fe9e0bd54c0b4c6ff433..e1dda0c951f9ea6f28b6d43ab2b9d4481f5d7773 100644 --- a/content/browser/renderer_host/render_frame_host_impl.h +++ b/content/browser/renderer_host/render_frame_host_impl.h -@@ -2550,6 +2550,17 @@ class CONTENT_EXPORT RenderFrameHostImpl +@@ -2557,6 +2557,17 @@ class CONTENT_EXPORT RenderFrameHostImpl HandleAXEvents(tree_id, std::move(updates_and_events), reset_token); } @@ -58,7 +58,7 @@ index b1259618ff4dc1848cf007d853e6f3e70b08829f..3d4e6c4c7e0348c52c3f0ccd0f6c19e0 protected: friend class RenderFrameHostFactory; -@@ -2879,17 +2890,6 @@ class CONTENT_EXPORT RenderFrameHostImpl +@@ -2892,17 +2903,6 @@ class CONTENT_EXPORT RenderFrameHostImpl // relevant. void ResetWaitingState();