Treat .onion as a secure context

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tjr, Assigned: gk)

Tracking

(Depends on 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [tor][domsecurity-active][tor 21321][tor 23439])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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]
(Reporter)

Comment 1

2 years ago
Arthur, do you have a patch we can review for this?
Flags: needinfo?(arthuredelstein)
(Assignee)

Comment 2

2 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)
(Reporter)

Comment 3

2 years ago
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.
(Assignee)

Comment 6

2 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

a year ago
Duplicate of this bug: 1432686
(Assignee)

Updated

a year ago
Priority: P3 → P2
Whiteboard: [tor][domsecurity-active] → [tor][domsecurity-active][tor 21321][tor 23439]
(Reporter)

Comment 8

a year ago
Tagging this for 60
Priority: P2 → P1
(Assignee)

Comment 9

a year 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)
(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

a year ago
Posted 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)
(Assignee)

Comment 12

a year ago
Posted 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)
(Assignee)

Comment 16

a year 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

a year ago
Attachment #8954715 - Attachment is obsolete: true
Attachment #8955057 - Flags: review?(ckerschb)
(Assignee)

Comment 18

a year 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 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+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c3f6b9e4bf7
https://hg.mozilla.org/mozilla-central/rev/99507aa10a4f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1442406
(Assignee)

Updated

a year ago
Depends on: 1444062
(Assignee)

Comment 23

a year 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.