Closed
Bug 1382359
Opened 7 years ago
Closed 6 years ago
Treat .onion as a secure context
Categories
(Core :: DOM: Security, enhancement, P1)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: tjr, Assigned: gk)
References
(Depends on 1 open bug)
Details
(Whiteboard: [tor][domsecurity-active][tor 21321][tor 23439])
Attachments
(2 files, 2 obsolete files)
7.09 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
5.03 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
Because .onion urls are self-authenticating, Tor Browser would like to treat them as Secure Contexts so they don't e.g. get insecure password warnings. See https://trac.torproject.org/projects/tor/ticket/21321 for more discussion. The idea is to treat them as 'Potentially Trustworthy URIs' https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy After speaking with Christoph, the plan is that we should have tests and put it behind a pref. And as far as implementing, to replicate the design of http://searchfox.org/mozilla-central/search?q=IsPotentiallyTrustworthyLoopbackURL&path=
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [tor] → [tor][domsecurity-active]
Reporter | ||
Comment 1•7 years ago
|
||
Arthur, do you have a patch we can review for this?
Flags: needinfo?(arthuredelstein)
Assignee | ||
Comment 2•7 years ago
|
||
Sort of: https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.3.0esr-7.5-2&id=a78c67cdffe064f5c0ae0f6284817fe3d1257efc and the test update: https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.3.0esr-7.5-2&id=74588e6ba24c21d8832ef51981779d7ac58c8d51 That is dealing with the scary password warnings and related things. What is missing is the "second half": dealing with mixed content warnings. I have not written that patch yet which is the sole reason for not posting the above links in this ticket earlier (or applying the patches to m-c to get ready-for-final-review-and-check-in patches). We could split that out into a new ticket or block that one on the remaining things we need to do. I am fine with either.
Flags: needinfo?(arthuredelstein)
Comment 4•7 years ago
|
||
(In reply to Tom Ritter [:tjr] from comment #3) > Christoph what do you think? Yes, that looks good to me.
Flags: needinfo?(ckerschb)
Comment 5•7 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4) > (In reply to Tom Ritter [:tjr] from comment #3) > > Christoph what do you think? SOrry, I didn't read precisely and was just checking if the patch looks good. I would rather block on the missing mixed content bits and land everything together.
Assignee | ||
Comment 6•7 years ago
|
||
Sounds good to me. I opened https://trac.torproject.org/projects/tor/ticket/23439 for the mixed content part on our side. I am not sure whether I get to it this month, though. But I definitely want to have everything for this bug ready to get into esr59.
Assignee | ||
Updated•6 years ago
|
Priority: P3 → P2
Whiteboard: [tor][domsecurity-active] → [tor][domsecurity-active][tor 21321][tor 23439]
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #4) > > (In reply to Tom Ritter [:tjr] from comment #3) > > > Christoph what do you think? > > SOrry, I didn't read precisely and was just checking if the patch looks > good. I would rather block on the missing mixed content bits and land > everything together. The mixed content bits are currently under review for inclusion into Tor Browser and are at https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_23439_v2& id=f10bad83876aee09c716086db659ab2bbe9652af (code) and https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_23439_v2&id=a7d9d4cf255368eef139b1dbde7d16933bfcaa30 (test). Christoph, do you think that looks reasonable? I was in particular wondering whether we'd need to adapt other tests, too (not just updating test_isOriginPotentiallyTrustworthy.js and writing a new one for mcb handling of .onions). browser_hasInsecureLoginForms.js maybe? If there are additional things to write which of those would be blockers for Firefox 60 and which could we postpone?
Flags: needinfo?(ckerschb)
Comment 10•6 years ago
|
||
(In reply to Georg Koppen from comment #9) > Christoph, do you think that looks reasonable? Accessing Preferences is expensive, probably worth caching the pref. Please note that the cached variable gets updated if the pref changes. Something like the following is probably better: bool nsMixedContentBlocker::IsPotentiallyTrustworthyOnion(nsIURI* aURL) { static bool sInited = false; static bool sWhiteListOnions = false; if (!sInited) { Preferences::AddBoolVarCache(&sWhiteListOnions, "dom.securecontext.whitelist_onions"); sInited = true; } if (!whiteListOnions) { return false; } nsAutoCString host; nsresult rv = aURL->GetHost(host); NS_ENSURE_SUCCESS(rv, false); return StringEndsWith(host, NS_LITERAL_CSTRING(".onion")); } > I was in particular wondering > whether we'd need to adapt other tests, too (not just updating > test_isOriginPotentiallyTrustworthy.js Test looks good, but as you said, probably worth updating browser_hasInsecureLoginForms.js too. Should be a quick update though.
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 11•6 years ago
|
||
I rebased my code, adapted it and added the tests I have so far (two patches). Please review, while I am waiting on getting my try access restored. I looked a bit at browser_insecureLoginForms.js but did not get it to work as I thought I should. I was thinking about amending the URL list in the first add_task() with a ["http://123456789abcdef.onion", false] pair making sure the whitelist pref is set but the test complains about not knowing what to do with the .onion URL. It seems I need to look closer at the test. I hope I can do it in a separate bug, though, as I fear I won't get to that before the Firefox 60 freeze and would like to see the patches I have so far uplifted for ESR 60.
Attachment #8954715 -
Flags: review?(ckerschb)
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8954716 -
Flags: review?(ckerschb)
Assignee | ||
Comment 13•6 years ago
|
||
Try URL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=242af5f858a381f062c5af54b7211edbfb34d1d7
Comment 14•6 years ago
|
||
Comment on attachment 8954715 [details] [diff] [review] 1382359-code.patch Review of attachment 8954715 [details] [diff] [review]: ----------------------------------------------------------------- Please incorporate my nit and flag me again - Review will be real quick. ::: dom/security/nsContentSecurityManager.cpp @@ +890,1 @@ > } Why not reuse the mixed content blocker code? Something like: if (nsMixedContentBlocker::IsPotentiallyTrustworthyOnion(uri) { *aIsTrustWorthy = true; return NS_OK; }
Attachment #8954715 -
Flags: review?(ckerschb)
Comment 15•6 years ago
|
||
Comment on attachment 8954716 [details] [diff] [review] 1382359-tests.patch Review of attachment 8954716 [details] [diff] [review]: ----------------------------------------------------------------- I also wanna see that one more time, but as I said. Should be quick. ::: browser/base/content/test/siteIdentity/browser_no_mcb_for_onions.js @@ +23,5 @@ > + > +add_task(function* allowOnionMixedContent() { > + Services.prefs.setBoolPref(PREF_BLOCK_DISPLAY, true); > + Services.prefs.setBoolPref(PREF_BLOCK_ACTIVE, true); > + Services.prefs.setBoolPref(PREF_ONION_WHITELIST, true); I know that some tests still use that, but it's outdated. Please use: await SpecialPowers.pushPrefEnv({"set": bla and remove the registerCleanUpFunction.
Attachment #8954716 -
Flags: review?(ckerschb)
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14) > Comment on attachment 8954715 [details] [diff] [review] > 1382359-code.patch > > Review of attachment 8954715 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please incorporate my nit and flag me again - Review will be real quick. > > ::: dom/security/nsContentSecurityManager.cpp > @@ +890,1 @@ > > } > > Why not reuse the mixed content blocker code? > Something like: > if (nsMixedContentBlocker::IsPotentiallyTrustworthyOnion(uri) { > *aIsTrustWorthy = true; > return NS_OK; > } Thanks, good point. That's just an oversight on my end probably due to the long delay between writing both parts of the code changes.
Assignee | ||
Comment 17•6 years ago
|
||
Attachment #8954715 -
Attachment is obsolete: true
Attachment #8955057 -
Flags: review?(ckerschb)
Assignee | ||
Comment 18•6 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15) > Comment on attachment 8954716 [details] [diff] [review] > 1382359-tests.patch > > Review of attachment 8954716 [details] [diff] [review]: > ----------------------------------------------------------------- > > I also wanna see that one more time, but as I said. Should be quick. > > ::: browser/base/content/test/siteIdentity/browser_no_mcb_for_onions.js > @@ +23,5 @@ > > + > > +add_task(function* allowOnionMixedContent() { > > + Services.prefs.setBoolPref(PREF_BLOCK_DISPLAY, true); > > + Services.prefs.setBoolPref(PREF_BLOCK_ACTIVE, true); > > + Services.prefs.setBoolPref(PREF_ONION_WHITELIST, true); > > I know that some tests still use that, but it's outdated. Please use: > await SpecialPowers.pushPrefEnv({"set": bla > and remove the registerCleanUpFunction. Tests updated. However, the `registerCleanUpFunction()` is still needed for dealing with open tabs at the end of the test, so I left it in. New try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e6a1a58e223f7e00582208bc16507bc4bc169ed (I needed to add a missing ":" to my try syntax to get the builds and tests actually running in case you are confused why you are only seeing the tests related commit)
Attachment #8954716 -
Attachment is obsolete: true
Attachment #8955060 -
Flags: review?(ckerschb)
Comment 19•6 years ago
|
||
Comment on attachment 8955057 [details] [diff] [review] 1382359-code-updated.patch Review of attachment 8955057 [details] [diff] [review]: ----------------------------------------------------------------- thanks, looks great, r=me
Attachment #8955057 -
Flags: review?(ckerschb) → review+
Comment 20•6 years ago
|
||
Comment on attachment 8955060 [details] [diff] [review] 1382359-tests-updated.patch Review of attachment 8955060 [details] [diff] [review]: ----------------------------------------------------------------- Ah I see why you still need the registerCleanup. thanks. r=me
Attachment #8955060 -
Flags: review?(ckerschb) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 21•6 years ago
|
||
Pushed by toros@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3f6b9e4bf7 Treat .onion as a secure context https://hg.mozilla.org/integration/mozilla-inbound/rev/99507aa10a4f Tests updated/added for bug 1382359 r=ckerschb CLOSED TREE
Keywords: checkin-needed
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1c3f6b9e4bf7 https://hg.mozilla.org/mozilla-central/rev/99507aa10a4f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Comment 23•6 years ago
|
||
(In reply to Georg Koppen from comment #11) > to do with the .onion URL. It seems I need to look closer at the test. I > hope I can do it in a separate bug, though, as I fear I won't get to that > before the Firefox 60 freeze and would like to see the patches I have so far > uplifted for ESR 60. Seems so. :) I've filed bug 1444062 for dealing with the test update.
You need to log in
before you can comment on or make changes to this bug.
Description
•