-
Notifications
You must be signed in to change notification settings - Fork 867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added mixed content check for *.onion origins. #15436
Conversation
// Treat .onions as https:// | ||
// onion -> https: not blocked | ||
// onion -> http: blocked | ||
return IsMixedContent("https", resource_url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url::kHttpsScheme
} | ||
{ | ||
content::WebContentsConsoleObserver console_observer(contents); | ||
ASSERT_FALSE(content::ExecJs(contents, "fetch('https://example.onion')")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you also add a test that images like <img src="http://example.com/favicon.jpg">
are blocked but not if it's example.onion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this probably needs an example URL that doesn't support https at all (http://example.com can be upgraded to HTTPS). so something like <img src="http://http.badssl.com/favicon.ico"
>`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test I'm using embedded_test_server() it supports http only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i originally wrote https but fixed it - now these URLs are all http. or do you mean if http://http.badssl.com/favicon.ico
redirects to HTTPS, it wouldn't work for the test server for some reason?
|
||
if (!security_origin->Host().EndsWith(kOnion)) | ||
return absl::nullopt; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also check here that security_origin
scheme is either http or https.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(mixed content checking should only happen if scheme is http/https)
if (!security_origin->Host().EndsWith(kOnion)) | ||
return absl::nullopt; | ||
|
||
if (resource_url.Host().EndsWith(kOnion)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should also check here if resource_url
scheme is http or https.
actually i would just recommend a separate method isOnion
or something, which checks that:
- hostname ends with
.onion
- scheme is http or https
since the same check must be applied to both security_origin
and resource_url
suggested test plan for qa:
|
chromium_src/third_party/blink/renderer/core/loader/mixed_content_checker.h
Show resolved
Hide resolved
Looks like this one would close brave/brave-browser#25939 and brave/brave-browser#1135 from what I'm seeing |
i don't think it would close brave/brave-browser#1135 since there's more to secure origins than just mixed content (geolocation allowed, etc.) |
730b03e
to
352b6e4
Compare
Thank you for the correction, I wasn't aware of that aspect of secure origins so seems like I've got some new "known unknowns" to read up on. |
3185d94
to
4fdae91
Compare
} | ||
{ | ||
content::WebContentsConsoleObserver console_observer(contents); | ||
ASSERT_FALSE(content::ExecJs(contents, "fetch('http://example.onion')")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also should change this to an onion site that sends access-control-allow-origin: *
, ex: http://static01.nyt2sl3njavkomjxqe2e5nz6bsqv56yqbwkvhpmfn5jwh4pyccmjibad.onion/ads/tpc-check.html
I created two test pages to confirm that the Tor Browser behaves as we expected (it does):
To run this test, I had to customize the following in Tor Browser:
|
@fmarier awesome! did you test the mixed content autoupgrade case? (i.e., secure page embeds an HTTP image that supports HTTPS. in chrome it should autoupgrade the request to HTTPS. you can try with this URL: http://mixed.badssl.com/image.jpg.) i'm guessing tor browser doesn't do the autoupgrade, but might be nice to have in brave. |
The Tor Browser is actually running with HTTPS-only mode now (like Firefox private browsing I believe). I had to explicitly disable that feature in order to see the behavior of the mixed content blocker :) |
HTTPS-Only Mode (HOM) in Tor Windows only enforces HTTPS on top-level (non-onion) domains, so it's a good idea to make sure that, when HOM is enabled, the mixed-content blocker blocks the loading of insecure (HTTP, non-onion images) on http://[address].onion pages. (FWIW, Firefox private browsing actually runs HTTPS-by-Default, which is somewhat weaker than HTTPS-Only Mode.) |
chromium_src/content/browser/renderer_host/mixed_content_navigation_throttle.cc
Show resolved
Hide resolved
this is pretty much good to go for me except i think this test page should not have the any other concerns @fmarier @arthuredelstein ? |
Looks good to me. Thanks @boocmp ! |
} | ||
{ | ||
content::WebContentsConsoleObserver console_observer(contents); | ||
const GURL resource_url = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems unnecessary since we're not using resource_url
in this block.
|
||
// We are hiding IsMixedContent(const String& origin_protocol, const KURL&) | ||
// because we want to enforce mixed content checks on .onion origins. | ||
// Publically available protocol-only overload of this method allows to skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "Publicly"
I don't know how to configure embedded http test server to support "upgrade-insecure-requests", The client request contains "upgrade-insecure-requests: 1" header, but without meta tag it doesn't work. It works only on embedded https server. |
oh i see, the problem is that the test server is HTTP. assuming there is no way to run an HTTPS test server, this is fine as-is assuming @fmarier has a test case for QA with autoupgrading mixed content. |
should be ok to merge? |
patches/third_party-blink-renderer-core-loader-mixed_content_checker.h.patch
Show resolved
Hide resolved
chromium_src/third_party/blink/renderer/platform/loader/fetch/https_state.cc
Show resolved
Hide resolved
chromium_src/third_party/blink/renderer/core/loader/mixed_content_checker.cc
Show resolved
Hide resolved
{ | ||
content::WebContentsConsoleObserver console_observer(contents); | ||
ASSERT_FALSE(content::ExecJs(contents, "fetch('https://example.com')")); | ||
EXPECT_TRUE(console_observer.messages().empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this observer doesn't wait for messages. Does this line check anything at all?
@@ -98,6 +100,11 @@ class BraveContentBrowserClientTest : public InProcessBrowserTest { | |||
"brave_webtorrent.html?chrome://settings"); | |||
} | |||
|
|||
void SetUpCommandLine(base::CommandLine* command_line) override { | |||
InProcessBrowserTest::SetUpCommandLine(command_line); | |||
command_line->AppendSwitch("ignore-certificate-errors"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please replace this with content::ContentMockCertVerifier
.
see for details: https://bravesoftware.slack.com/archives/C7VLGSR55/p1636702893335800
Resolves brave/brave-browser#25939
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: