Closed Bug 1382359 Opened 7 years ago Closed 6 years ago

Treat .onion as a secure context

Categories

(Core :: DOM: Security, enhancement, P1)

enhancement

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)

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=
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [tor] → [tor][domsecurity-active]
Arthur, do you have a patch we can review for this?
Flags: needinfo?(arthuredelstein)
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)
Christoph what do you think?
Flags: needinfo?(ckerschb)
(In reply to Tom Ritter [:tjr] from comment #3)
> Christoph what do you think?

Yes, that looks good to me.
Flags: needinfo?(ckerschb)
(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.
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.
Priority: P3 → P2
Whiteboard: [tor][domsecurity-active] → [tor][domsecurity-active][tor 21321][tor 23439]
Tagging this for 60
Priority: P2 → P1
(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)
(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)
Attached patch 1382359-code.patch (obsolete) — Splinter Review
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)
Attached patch 1382359-tests.patch (obsolete) — Splinter Review
Attachment #8954716 - Flags: review?(ckerschb)
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 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)
(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.
Attachment #8954715 - Attachment is obsolete: true
Attachment #8955057 - Flags: review?(ckerschb)
(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 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 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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1c3f6b9e4bf7
https://hg.mozilla.org/mozilla-central/rev/99507aa10a4f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1442406
Depends on: 1444062
(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.

Attachment

General

Creator:
Created:
Updated:
Size: