#23247 closed project (fixed)
Communicating security expectations for .onion: what to say about different padlock states for .onion services
Reported by: | isabela | Owned by: | pospeselr |
---|---|---|---|
Priority: | High | Milestone: | |
Component: | Applications/Tor Browser | Version: | |
Severity: | Normal | Keywords: | ux-team, tor-hs, TorBrowserTeam201806R |
Cc: | asn, arthuredelstein, tor@…, phw, pospeselr, dmr, brade, mcs, tbb-team | Actual Points: | |
Parent ID: | Points: | ||
Reviewer: | Sponsor: |
Description (last modified by )
Background
Firefox (and other browsers) have created a set of states a site can have in relationship with ssl certificates, and how to communicate that to the user.
Currently, Tor Browser doesn't communicate ideally to users that visit onion sites--i.e. http + onion looks really scary with lots of warnings! This is something that was discussed under #21321. We then realized that we should look at all the different state + .onion combinations, and carefully communicate what these mean to the user.
Objective
The work on this ticket is to map all the current states Firefox has for ssl certificates on the padlock, and from there start to build a new way to communicate these states when they are related to a .onion sites. We started mapping them here:
https://docs.google.com/document/d/1KHkj2DpmFMB0mjHEfehD5ztY2L0lQzKNtZqct1TXbmg/edit
Is still pending the most difficult part of the work, which is to define what to do for .onion sites on those states.
Child Tickets
Attachments (11)
Change History (90)
comment:1 Changed 18 months ago by
comment:2 Changed 18 months ago by
Type: | defect → project |
---|
I've switched this from a task to a project, for organizational purposes.
comment:3 Changed 18 months ago by
Description: | modified (diff) |
---|---|
Summary: | creating padlock states for .onion services on tool bar → Communicating security expectations for .onion: what to say about different padlock states for .onion services |
comment:4 Changed 18 months ago by
Relevant to this discussion, an idea I ended up not liking #21952 and conclusion.
comment:5 Changed 18 months ago by
I think there are more states not in that doc (specifically related to Mixed Content) - or at least there are nuances of the single mixed content line:
- HTTPS Site with HTTP Onion Subresources
- HTTPS Site with HTTPS Onion Subresources
- HTTPS Site with HTTPS Self-Signed Onion Subresources
- HTTP Onion with HTTP Subresources
- HTTPS Onion with HTTP Subresources
- HTTPS Self Signed Onion with HTTP Subresources
- HTTP Onion with HTTPS Subresources
- HTTPS Onion with HTTPS Subresources
- HTTPS Self Signed Onion with HTTPS Subresources
1-3 can be described as "A clearnet website embeds onion stuff"
4-6 as "An onion website embeds clearnet stuff over HTTP"
7-9 as "An onion website embeds clearnet stuff over HTTPS"
(I think that's comprehensive...)
There are five padlock styles:
- Green with EV Banner
- Green
- Strikethrough
- Red
- Missing Entirely.
My opinion about behavior:
- Onion over HTTP: Green
- Onion with Self-Signed HTTPS: Green
- Onion with CA-Issused EV Cert: Green with EV Banner [0]
- Mixed Content Scenarios:
- HTTPS Site with HTTP Onion Subresources: Green (no mixed content warning) - but we remove the EV Banner if present [1]
- HTTPS Site with HTTPS Onion Subresources: Green or Green w/ EV Banner (no mixed content warning)
- HTTPS Site with HTTPS Self-Signed Onion Subresources: Green (no mixed content warning) - but we remove the EV Banner if present [1]
- HTTP Onion with HTTP Subresources: Red
- HTTPS Onion with HTTP Subresources: Red
- HTTPS Self Signed Onion with HTTP Subresources: Red
- HTTP Onion with HTTPS Subresources: Strikethrough
- HTTPS Onion with HTTPS Subresources: Strikethrough
- HTTPS Self Signed Onion with HTTPS Subresources: Strikethrough
[0] Security concern: Make sure EV banner only displays for CA-signed EV certs and not self-signed EV certs!
[1] Removing the EV banner might be difficult, but in the ideal situation I think we should.
I reserve the right to change my mind, but this is what I am thinking right now.
comment:6 Changed 15 months ago by
Cc: | asn added |
---|
comment:7 Changed 15 months ago by
Talking about this with asn on irc the following came up. Is there is a difference between a self-signed certificate and other types of invalid ssl certificates?
E.g. A self-signed cert with the correct name vs a CA-signed cert with the incorrect name.
IF we show a green icon for a self-signed cert with the correct name, someone who is actually running a malicious onion and gets you to visit it and change all other situations (ca-signed cert with incorrect name) to one that gets you a green icon. So showing a warning page for any other situation provides no security. BUT maybe it provides the webmaster with an indicator that their server was misconfigured and is not sending the certificate they should send?
(Alternately, maybe we don't want to send that indicator because it now requires webmasters who have a working example.com cert and configuration to not only deploy a .onion but deploy a new vhost pointing at the same config and serve that vhost a separate SSL cert which is configuration they could otherwise avoid.)
comment:8 follow-up: 11 Changed 15 months ago by
More discussion: there are definitely better ways to surface a potential misconfiguration to a site admin than an intersitial. A console warning/error or (if we want to surface something to UI) a mixed content icon.
There's also the notion of showing different icons for self-signed .onion (grey onion?) vs DV-ca-signed .onion (green onion?)
Changed 15 months ago by
Attachment: | padlock states FF and TB.png added |
---|
comment:9 Changed 15 months ago by
As we signed last week, I started to work on the UI cases for .onion services states.
This is the first approach and could totally change during this process.
I re-worked the Onion glyph in order to work better on small sizes. Also, I included colors based on the current Firefox UI.
If we are on track with this ones, we can move forward with Doorhangers components. I think we are going to use doorhangers for mixed content states and for EV-style https certificates.
http://design.firefox.com/photon/components/doorhangers.html
Let me know what do you think, thanks! :)
comment:10 Changed 15 months ago by
@antonela
Looks pretty damn neat for a first try. One remark though, isn't the "Onion" near it redundant? Considering that onion addresses will become more than 50 characters long, and that the Tor Browser size is generally around 1000x600, so there are strong incentives to preserve as much space as possible for the address bar.
Also I think there should be a better idea on distinguishing between http/https onions, a "yellow" version suggests that it is less secure.
comment:11 Changed 15 months ago by
Replying to tom:
There's also the notion of showing different icons for self-signed .onion (grey onion?) vs DV-ca-signed .onion (green onion?)
Hm. Reason this idea is good:
- Will be easier for users to distinguish between real facebook onion (DV-ca-signed green onion) and phishing facebook onion (self-signed grey onion).
Reason this idea is bad:
- It basically gives no way for onion site operators to get the green onion without paying the CA mafia.
How does Let's Encrypt blend into the above idea? Would it give a green-onion or not? If yes, then phishers can just use a Let's Encrypt cert to get the green onion anyway.
comment:12 Changed 15 months ago by
Cc: | arthuredelstein added |
---|
comment:13 follow-up: 18 Changed 15 months ago by
Right now you can't get a DV onion cert. There's a recent thread on drafting a ballot to allow them in the CAB Forum, with early support, but there's no guarantee it will pass. No DV onion certs means no Let's Encrypt. And once DV is allowed, LE would need to develop the software needed to validate .onions automatically, which would take some time as well.
---
My thoughts:
Graphics wise I think all of them look good.
I don't think we should put the word 'Onion' either though. In fact, doing so overloads the location where EV data is displayed, so if I got a company called 'Onion' I could make it look like I had an onion address!
I'm not sure what the (i) button is intended to show graphics wise. "There is information for you to review here"? I presume it opens the current doorhanger thing that lets you get certificate information and review permissions.
I don't know if there was a path forward agreed upon that was not documented here, but policy-wise this is a bit different from what I at least envisioned.
1) An HTTP Onion is Orange. Orange indicates a warning state. I don't believe we should communicate that HTTP Onion is 'warning'. It's almost always better than HTTP in fact, which we give 'grey' treatment. So I think HTTP+Onion should either be Grey or Green.
2) EV HTTPS + Onion has an info bubble but does not display the company name like EV does for HTTPS. I think we should be consistent here and display the company name here.
3) I don't understand why HTTPS onion lacks a (i) but self-signed HTTPS onion has it. Both of them should let you review the information. So the (i) definetly is implying some sort of state about the website, but it's confusing what I'm supposed to be able to draw from this.
4) It seems like we need to make a decision: is a self-signed SSL cert on a .onion:
a) completely meaningless
b) an indicator something is wrong
c) an indicator of trust.
These would correspond to:
a) the same icon as a http onion
b) an orange or red icon
c) a green icon
I don't think a self-signed cert is an indicator of trust, so it wouldn't automatically mean it gets a green icon. I also don't think it's an indicator something is wrong, so automatically giving it orange or red are out too. So it should match an HTTP Onion icon *but* allow you to view the certificate in the doorhanger.
My 2 cents.
comment:14 Changed 15 months ago by
I think it would be nice to give the user a hint that an onion connection is encrypted. Maybe the icon could be a padlock superimposed on an onion? Or we could continue to show the padlock but use the word "Onion" instead of "Secure".
Of course, there's the added complication of that the padlock might be intended to convey both "encrypted" and "externally authenticated", but that model isn't quite the same for onions. I'm not sure how to deal with this complication.
comment:15 Changed 15 months ago by
Cc: | hola@… added |
---|
comment:16 Changed 15 months ago by
Cc: | hola@… removed |
---|
Changed 15 months ago by
Attachment: | onion-mockup.png added |
---|
comment:17 Changed 15 months ago by
I realize we might be a bit constrained by Mozilla's design language for Firefox, but speaking as someone that's mildly red-green colour-blind, please don't differentiate important states solely through a red/green/grey/orange colour change. Even for colour-blind users that can tell a difference if they actually look at it (such as myself) the difference isn't going to be attention grabbing and may as well not exist.
For .onion addresses, why not just add a Green onion icon beside whatever existing icon/text Firefox already uses to communicate HTTPS information?
Something like this: https://trac.torproject.org/projects/tor/attachment/ticket/23247/onion-mockup.png
comment:18 Changed 15 months ago by
Replying to tom:
My thoughts:
Graphics wise I think all of them look good.
I don't think we should put the word 'Onion' either though. In fact, doing so overloads the location where EV data is displayed, so if I got a company called 'Onion' I could make it look like I had an onion address!
I'm not sure what the (i) button is intended to show graphics wise. "There is information for you to review here"? I presume it opens the current doorhanger thing that lets you get certificate information and review permissions.
I don't know if there was a path forward agreed upon that was not documented here, but policy-wise this is a bit different from what I at least envisioned.
1) An HTTP Onion is Orange. Orange indicates a warning state. I don't believe we should communicate that HTTP Onion is 'warning'. It's almost always better than HTTP in fact, which we give 'grey' treatment. So I think HTTP+Onion should either be Grey or Green.
2) EV HTTPS + Onion has an info bubble but does not display the company name like EV does for HTTPS. I think we should be consistent here and display the company name here.
3) I don't understand why HTTPS onion lacks a (i) but self-signed HTTPS onion has it. Both of them should let you review the information. So the (i) definetly is implying some sort of state about the website, but it's confusing what I'm supposed to be able to draw from this.
4) It seems like we need to make a decision: is a self-signed SSL cert on a .onion:
a) completely meaningless
b) an indicator something is wrong
c) an indicator of trust.
These would correspond to:
a) the same icon as a http onion
b) an orange or red icon
c) a green icon
I don't think a self-signed cert is an indicator of trust, so it wouldn't automatically mean it gets a green icon. I also don't think it's an indicator something is wrong, so automatically giving it orange or red are out too. So it should match an HTTP Onion icon *but* allow you to view the certificate in the doorhanger.
My 2 cents.
+1 to all of those. I have no clear opinion yet (either) on whether we should show an HTTP .onion as grey or green.
comment:19 Changed 15 months ago by
Cc: | tor@… added |
---|
comment:20 follow-up: 21 Changed 15 months ago by
From the meeting today:
Icon Styles we can choose from. (You may need FF 58 to view these the way tjr sees them.)
Green Padlock with EV Banner
Green Onion with EV Banner
Green Padlock: https://sha512.badssl.com/
Green Onion
The four above states indicate complete trust in the website. The EV Banner is used to convey identity information, to positive indicate you are talking to this specific *company* that operates this website.
Green Padlock with warning - https://self-signed.badssl.com/ (used for excepted self-signed certs this one is WEIRD)
This state is weird. We shouldn't need it. It indicates that while the connection is secure, the browser thought it might not be secure but you went and told the browser no really this is secure.
Grey Padlock with warning - https://mixed.badssl.com/
Grey Onion with warning
These icons indicate that the website is mostly secure, but that there was a problem with the configuration. It could be better, but it's not INSECURE.
Grey Padlock with Red Strikethrough - http://http-password.badssl.com/
Grey onion with Red Strikethrough
These icons indicate something is DEFINETLY INSECURE
Grey Onion
Grey Padlock
These icons don't exist. We could make them, but we would need to define what they mean.
Missing Entirely http://http.badssl.com/
This is a legacy state. It's for HTTP. It's insecure because it's not actually secure, but we don't want to say it's insecure because we'd put it on so much of the web we'd scare users.
I believe this table represents current thinking.
Onion over HTTP: ??????? Onion with Self-Signed HTTPS: ??????? Onion with CA-Issused DV Cert: Green Onion Onion with CA-Issused EV Cert: Green Onion with EV Banner Mixed Content Scenarios: A HTTPS Site embeds onion: HTTPS Site with HTTP Onion Subresources: Keeps Original Padlock (whether that was Green or w/ Warning or whatever) HTTPS Site with HTTPS Onion Subresources: Keeps Original Padlock HTTPS Site with HTTPS Self-Signed Onion Subresources: Keeps Original Padlock An Onion embeds HTTP: HTTP Onion with HTTP Subresources: Grey onion with Red Strikethrough HTTPS Onion with HTTP Subresources: Grey onion with Red Strikethrough HTTPS Self Signed Onion with HTTP Subresources: Grey onion with Red Strikethrough An onion embeds HTTPS: HTTP Onion with HTTPS Subresources: ??????? HTTPS Onion with HTTPS Subresources: Green Onion (with EV Banner if EV certificate) HTTPS Self Signed Onion with HTTPS Subresources: Grey Onion with warning
comment:21 Changed 15 months ago by
Replying to tom:
From the meeting today:
I believe this table represents current thinking.
Hello tjr, I posted some thoughts on the various options on the pad. Check them out and please enrich them with your thoughts! :)
comment:22 Changed 14 months ago by
Based on last week meeting, I have been working on the .onion services states outlined before in this thread.
As you may see, I continued working on the .onion icon trying to make it more clear and visible even in small sizes(8px/14px).
https://trac.torproject.org/projects/tor/attachment/ticket/23247/23247-2.png
Firefox is deprecating the lockpad related icons to inform security states based on their design documentation: https://design.firefox.com/icons/viewer/
As we talked at #tor-project, it is something we need to figure out if we want to keep consistency with FF.
Let me know your thoughts!
comment:23 follow-up: 24 Changed 14 months ago by
I spoke with Mozilla's crypto engineering team - they're not aware of any padlock deprecation, so I think the design guide is a separate thing.
comment:24 follow-up: 25 Changed 14 months ago by
Replying to tom:
I spoke with Mozilla's crypto engineering team - they're not aware of any padlock deprecation, so I think the design guide is a separate thing.
ACK thanks for asking. That's good. This means we can continue considering onions in the URL bar.
BTW, you guys that are at All Hands this week, would you be able to figure out the tradeoffs about onion color on HTTP vs self-signed HTTPS? There is a debate in the end of the pad that might help you. All Hands seems like a good place to figure this debate out!
Changed 14 months ago by
Attachment: | 23247-2.png added |
---|
comment:25 Changed 14 months ago by
Replying to asn:
Replying to tom:
I spoke with Mozilla's crypto engineering team - they're not aware of any padlock deprecation, so I think the design guide is a separate thing.
ACK thanks for asking. That's good. This means we can continue considering onions in the URL bar.
BTW, you guys that are at All Hands this week, would you be able to figure out the tradeoffs about onion color on HTTP vs self-signed HTTPS? There is a debate in the end of the pad that might help you. All Hands seems like a good place to figure this debate out!
I talked to a few people there, but didn't take a big survey. Trend seems to be that positive indicators are 'blah' and we should move to only negative indicators and in a positive state show nothing.
Another vote, separate from that discussion, was a very strong 'no positive indicator for .onion'
comment:26 Changed 13 months ago by
Cc: | phw added |
---|
The following SOUPS 2016 paper seems very relevant to this ticket. It was written by people from the Chrome security team and their work resulted in the indicators we see in Chrome today:
https://www.usenix.org/system/files/conference/soups2016/soups2016-paper-porter-felt.pdf
I skimmed parts of the paper and found the following two takeaways relevant:
Section 1:
The indicator's meaning needs to be taught with words when possible. Millions of new Internet users have recently come online via smartphones without learning "standard" iconography from desktop browsers.
We may also want to change the text next to the onion icon. In the paper, in Table 4, they evaluated what string users most associate with security and "secure" won, closely followed by "https," which they deemed too technical. Another of their candidates was "secure and private" which may be suitable in our case. I worry that just replacing the lock icon with an onion may not make it clear what's different -- in particular because onions are typically not associated with security.
Section 3.1:
Making major modifications to this [lock] symbol, such as using a different object, may be disorienting: users now expect to find a lock in a browser window.
I wonder if the presence of an onion will confuse some people? Another way forward would be to use the lock icon for onion services too but change the string from "secure" to "secure and private."
comment:27 follow-up: 28 Changed 12 months ago by
So where do we think we are here? Do we think it's worth yet another meeting or do we have a plan laid out?
comment:28 Changed 12 months ago by
Replying to asn:
So where do we think we are here? Do we think it's worth yet another meeting or do we have a plan laid out?
Good question. I'll put this onto the meeting agenda for our Tor Browser meeting tomorrow.
comment:29 Changed 12 months ago by
Actually isa already answered that on the meeting pad on Feb 12:
[isa: is pending a task from me asked by Geko which is to create a new doc with the final states we are implementing and copy (cleaner version from what is currently linked on the ticket) then after that it should be planned/added for implementation at TB roadmap in Rome]
comment:30 Changed 11 months ago by
Cc: | pospeselr added |
---|---|
Keywords: | TorBrowserTeam201804 added |
Priority: | Medium → High |
comment:31 follow-up: 32 Changed 10 months ago by
Cleaned up and updated all stated with icon and copy:
https://docs.google.com/document/d/1bPrNLIl7Qy-sA7aTfElu80Xk2eXzTfH_5BGTOUDK8XU/edit#
comment:32 follow-ups: 33 34 Changed 10 months ago by
Replying to isabela:
Cleaned up and updated all stated with icon and copy:
https://docs.google.com/document/d/1bPrNLIl7Qy-sA7aTfElu80Xk2eXzTfH_5BGTOUDK8XU/edit#
In looking over the above document, the only scenario that surprised me was "HTTPS Site with HTTPS Self-Signed Onion Subresources" which has an onion icon (I expected a padlock).
comment:33 Changed 10 months ago by
Replying to brade:
Replying to isabela:
Cleaned up and updated all stated with icon and copy:
https://docs.google.com/document/d/1bPrNLIl7Qy-sA7aTfElu80Xk2eXzTfH_5BGTOUDK8XU/edit#
In looking over the above document, the only scenario that surprised me was "HTTPS Site with HTTPS Self-Signed Onion Subresources" which has an onion icon (I expected a padlock).
Padlock downgrades to onion icon to reflect "Self-Signed Onion" trick.
And for "HTTPS Site with HTTP Onion Subresources" too, please.
comment:34 follow-up: 35 Changed 10 months ago by
Replying to brade:
Replying to isabela:
Cleaned up and updated all stated with icon and copy:
https://docs.google.com/document/d/1bPrNLIl7Qy-sA7aTfElu80Xk2eXzTfH_5BGTOUDK8XU/edit#
In looking over the above document, the only scenario that surprised me was "HTTPS Site with HTTPS Self-Signed Onion Subresources" which has an onion icon (I expected a padlock).
Yes, I think it should have a padlock. I think isa may have missed this case when e updated the other ones in this section?
comment:35 Changed 10 months ago by
Replying to tom:
Replying to brade:
Replying to isabela:
Cleaned up and updated all stated with icon and copy:
https://docs.google.com/document/d/1bPrNLIl7Qy-sA7aTfElu80Xk2eXzTfH_5BGTOUDK8XU/edit#
In looking over the above document, the only scenario that surprised me was "HTTPS Site with HTTPS Self-Signed Onion Subresources" which has an onion icon (I expected a padlock).
Yes, I think it should have a padlock. I think isa may have missed this case when e updated the other ones in this section?
Seems to me this is fixed now in the doc. I think the doc Looks Good To Me right now.
comment:36 follow-up: 38 Changed 10 months ago by
FWIW it's important to keep this timeline in mind especially the coming deprecation of the lock icon (2019) for HTTPS https://blog.cloudflare.com/https-or-bust-chromes-plan-to-label-sites-as-not-secure/
- Google will announce the lock icon’s demise in 2018 and remove it in January 2019 with the release of Chrome 72
comment:37 Changed 10 months ago by
Cc: | dmr added |
---|
comment:38 follow-up: 44 Changed 10 months ago by
Replying to cypherpunks:
FWIW it's important to keep this timeline in mind especially the coming deprecation of the lock icon (2019) for HTTPS https://blog.cloudflare.com/https-or-bust-chromes-plan-to-label-sites-as-not-secure/
- Google will announce the lock icon’s demise in 2018 and remove it in January 2019 with the release of Chrome 72
Just to place the quote into context:
This is a guess/prediction by CloudFlare. That Chrome will eventually deprecate the lock icon seems to be part of their rough plan from 2014, but there is no set timeframe indicated in the article. (Still good to keep in mind, however.)
Chris Palmer's email to blink-dev in 2014 included this "strawman proposal" for introducing negative indicators and phasing out the marking of secure origins entirely:
Secure > 65%: Non-secure origins marked as Dubious
Secure > 75%: Non-secure origins marked as Non-secure
Secure > 85%: Secure origins unmarked
comment:39 Changed 10 months ago by
Keywords: | tor-hs added |
---|
comment:40 Changed 10 months ago by
Owner: | changed from tbb-team to pospeselr |
---|---|
Status: | new → assigned |
comment:41 Changed 10 months ago by
Cc: | brade mcs added |
---|
comment:42 Changed 10 months ago by
Cc: | tbb-team added |
---|
comment:43 Changed 10 months ago by
Keywords: | TorBrowserTeam201805 added; TorBrowserTeam201804 removed |
---|
Move our roadmap tickets to May.
comment:44 Changed 9 months ago by
Replying to dmr:
Replying to cypherpunks:
FWIW it's important to keep this timeline in mind especially the coming deprecation of the lock icon (2019) for HTTPS https://blog.cloudflare.com/https-or-bust-chromes-plan-to-label-sites-as-not-secure/
- Google will announce the lock icon’s demise in 2018 and remove it in January 2019 with the release of Chrome 72
Just to place the quote into context:
This is a guess/prediction by CloudFlare. That Chrome will eventually deprecate the lock icon seems to be part of their rough plan from 2014, but there is no set timeframe indicated in the article. (Still good to keep in mind, however.)
It's official: in September 2018, Google will stop marking HTTPS sites as secure (but there's a non-green doorhanger still). https://blog.chromium.org/2018/05/evolving-chromes-security-indicators.html
Full doorhanger removal will happen "eventually", and CloudFlare's prediction is it will happen "in January 2019 with the release of Chrome 72".
Changed 9 months ago by
Attachment: | Page Info SSL.png added |
---|
comment:45 follow-up: 47 Changed 9 months ago by
Ok, I've a (seemingly) full working patch with one little bit of polish remaining. The Page Info window (at the bottom under Technical Details) for HTTP Onion services indicate that the connection isn't encrypted, when in fact it is fully end-to-end encrypted by virtue of going over Tor:
Here's what an HTTPS Onion has to say:
It's easy enough to add/modify text here, but the question is what text to put here? Preferably something succinct I think (so that it can be added as an addendum to HTTPS Onion services as well).
Any thoughts on this?
comment:46 Changed 9 months ago by
Not sure about the text yet, but one thing that came to mind: there won't be an alpha based on esr52 anymore, so the patches should be against esr60 (not sure if you took this already into account).
comment:47 Changed 9 months ago by
Replying to pospeselr:
It's easy enough to add/modify text here, but the question is what text to put here? Preferably something succinct I think (so that it can be added as an addendum to HTTPS Onion services as well).
Any thoughts on this?
How about starting with something simple like Connection encrypted via onion services
or Connection encrypted using onion services
?
I know it doesn't specify the actual ciphers like the HTTPS Onion
page, but perhaps we don't need to be perfect from the beginning. If we want to specify ciphers we need to distniguish between v2 and v3 and list the right ciphers for each one. Let me know if you want to do that.
Changed 9 months ago by
Attachment: | Page Info New.png added |
---|
Changed 9 months ago by
Attachment: | Page Info SSL New.png added |
---|
comment:49 Changed 9 months ago by
Keywords: | TorBrowserTeam201805R added; TorBrowserTeam201805 removed |
---|---|
Status: | assigned → needs_review |
comment:50 Changed 9 months ago by
New text looks great to me! I didn't review the tor browser code patch.
comment:51 Changed 9 months ago by
Keywords: | TorBrowserTeam201805 added; TorBrowserTeam201805R removed |
---|---|
Status: | needs_review → needs_revision |
Overall, looks good to me! Some minor things:
1) I had trouble parsing your commit message (there are some typos and things in the "-" lines missing etc.). https://chris.beams.io/posts/git-commit/ might be a useful resource. At least it helped me a lot.
2) There is code in browser.js
regarding the PointerLock API that might require patching as well (IIRC this or similar code is used or was used to show the same info when entering into fullscreen, at least on some platforms):
get pointerlockFsWarningClassName() { // Note that the fullscreen warning does not handle _isSecureInternalUI. if (this._uriHasHost && this._isEV) { return "verifiedIdentity"; } if (this._uriHasHost && this._isSecure) { return "verifiedDomain"; } return "unknownIdentity";
3) Could you explain why you needed to add this._sslStatus != null
in
} else if (this._uriHasHost && this._isSecure && this._sslStatus != null) {
If that's not obvious I think adding a comment would be useful.
4) The const bool
does not really fit into the surrounding code (where just bool
is used). I think we should stick to the latter. Or maybe even better just get rid of this variable and do things like
if (!StringEndsWith(host, NS_LITERAL_CSTRING(".onion")))
in both places. (In the parentIsOnion
case, maybe rename host
to parentHost
then as this makes the context clearer.
5) I like the strings added to the Page Info window. However, doing it that way we have trouble translating them until we got our code upstreamed and Mozilla takes care of the translation. This has the unfortunate side-effect of mixed english $LANG text.
We had a similar issue in #20244 (see in particular comment:2:ticket:20244) and worked around it with a Torbutton overlay. I wonder if that could be a solution for this case as well.
comment:52 Changed 9 months ago by
Very nice work. I have these additional comments:
- In browser/base/content/browser.js, a space is missing after the
if
here:if(this._isEV) {
- In dom/security/nsMixedContentBlocker.cpp, a space is missing after the
if
here:if(NS_FAILED(rv)) {
- Should we remove the comments and meta data (title, desc, defs) from the SVG files?
- In browser/base/content/pageinfo/security.js, dom/security/nsMixedContentBlocker.cpp, and security/manager/ssl/nsSecureBrowserUIImpl.cpp: is it safe to assume the host name is lower case? The browser seems to switch
.ONION
to.onion
when I try to use the former but I don't know why that happens.
comment:53 follow-up: 60 Changed 9 months ago by
Ok, updated patch with both gk and mcs's feedback and fixed the word wrapping. That's what I get for assuming my text editor's word wrap functionality would just work the way I expected it to.
gk: The _sslStatus != null check is required because now, the _isSecure check will be true for .onion domains, even if they are lacking a certificate. Without that check, we'd be doing certificate related operations for HTTP onions.
mcs: The URI is already normalized (?) before it reaches any of this code. Had a similar concern with one of gk's patches awhile back :).
comment:54 Changed 9 months ago by
Keywords: | TorBrowserTeam201805R added; TorBrowserTeam201805 removed |
---|---|
Status: | needs_revision → needs_review |
comment:56 Changed 9 months ago by
comment:57 Changed 9 months ago by
http://onion = secure
https://onion + selfsign = secure (because pubkey is transmitted over onion channel)
https://onion + evsign = secure
comment:58 Changed 9 months ago by
https://onion + (SNImismatch & AltNamemismatch) = insecure warning, red padlock.
https://onion + outdatedDateCertificate = error
comment:60 Changed 9 months ago by
Keywords: | TorBrowserTeam201805 added; TorBrowserTeam201805R removed |
---|---|
Status: | needs_review → needs_revision |
Replying to pospeselr:
Ok, updated patch with both gk and mcs's feedback and fixed the word wrapping. That's what I get for assuming my text editor's word wrap functionality would just work the way I expected it to.
gk: The _sslStatus != null check is required because now, the _isSecure check will be true for .onion domains, even if they are lacking a certificate. Without that check, we'd be doing certificate related operations for HTTP onions.
Yeah, that's what I assumed. The fixups look good, thanks. Two things remain:
1) Please cut the subject line of your commit message. "Bug 23247: Communicating security expectations for .onion" is a fine subject.
2) There is still the localization issue of the hardcoded strings on the Page Info dialog.
I think there is no need to fix that for the ESR52-codebase, rather those could be addressed for ESR60.
comment:61 follow-up: 64 Changed 9 months ago by
Not sure I'm following gk regarding the strings. The new onion-related strings are not hard-coded in the Page Info dialog, but pull from pippki.properties file using the pkiBundle.getBLAHString API (as does every other string lookup). Should I be doing something else?
comment:62 Changed 9 months ago by
Keywords: | TorBrowserTeam201805R added; TorBrowserTeam201805 removed |
---|---|
Status: | needs_revision → needs_review |
Updated subject line
comment:63 Changed 9 months ago by
Uploaded new version of patch against the esr60 gecko-dev branch as well as an updated esr52 version. New version has better implemented detection of '_hasInsecureLoginForms'. Relevant logic is piped down from nsGlobalWindow::ComputeIsSecureContext which now checks for nsContentUtils::HttpsStateIsModern as well as nsContentUtils::DocumentHasOnionURI.
Will be running my ESR60 patch against the TrySerer tonight to see if this breaks anything.
comment:64 Changed 9 months ago by
Replying to pospeselr:
Not sure I'm following gk regarding the strings. The new onion-related strings are not hard-coded in the Page Info dialog, but pull from pippki.properties file using the pkiBundle.getBLAHString API (as does every other string lookup).
That's right. But where does the translation come from? Like, how would that string look like in a, say, arabic Tor Browser? Clearly, it does not get translated by Mozilla as we only ship it in our patch set. But all our translations are done on Transifex either via Torbutton or Tor Launcher. Thus, the string is essentially hard-coded assuming the browser is falling back to en-US in case it can't find a string for the required locale. It might even break, not showing the dialog at all.
Should I be doing something else?
We could try, see 5) in my comment:51.
comment:65 Changed 9 months ago by
Keywords: | TorBrowserTeam201806R added; TorBrowserTeam201805R removed |
---|
Moving review tickets to June.
Changed 9 months ago by
Attachment: | 060618.png added |
---|
comment:66 Changed 9 months ago by
Hi all! Thanks for working hard on this feature. It looks awesome.
Could be possible to replicate the icon we have at the URL bar inside the doorhanger? Is it something doable?
https://trac.torproject.org/projects/tor/attachment/ticket/23247/060618.png
I'm not sure if include it here or at #23247. Let me know if it makes sense.
Thanks!
Changed 9 months ago by
Attachment: | mixed content.png added |
---|
comment:67 Changed 9 months ago by
comment:68 Changed 9 months ago by
I think we should keep this as one possible follow-up item for the next iteration. We are close to land this (just some tiny bit is missing) and ship this to some users. And we implemented what we designed, so I think that's a good enough step for the first alpha with this feature.
comment:69 Changed 9 months ago by
Thanks, pospeselr; yes, we need to discuss all the scenarios before going to implementation.
gk, gotcha. I'll save it for the short future.
comment:70 Changed 9 months ago by
Keywords: | TorBrowserTeam201806 added; TorBrowserTeam201806R removed |
---|---|
Status: | needs_review → needs_revision |
Marking this as needs_revision
for the missing localization parts.
comment:71 follow-up: 73 Changed 9 months ago by
Turns out localization here was way simpler than what Arthur had to do for his patch. We can simply stick the strings in torbutton.properties, which is the same format as where I had them before, just in separate file that we control.
I've also attached a patch for torbutton which adds the new strings (in English) to all of our locales for future translation.
comment:72 Changed 9 months ago by
Keywords: | TorBrowserTeam201806R added; TorBrowserTeam201806 removed |
---|---|
Status: | needs_revision → needs_review |
comment:73 Changed 9 months ago by
Replying to pospeselr:
Turns out localization here was way simpler than what Arthur had to do for his patch. We can simply stick the strings in torbutton.properties, which is the same format as where I had them before, just in separate file that we control.
What happens if Torbutton is uninstalled? For #14631, we added English strings to the browser but wrote code to retrieve the strings from Torbutton if available. We probably want to implement a similar fallback scheme for this ticket. See these commits on the tor-browser-60.0.1esr-8.0-1 branch:
10ac5a7be31f6310a3a0dea6565e3ee1982c602e
b6d8bf568ba67c55a42b6b057d6e9bba12e413d0
I've also attached a patch for torbutton which adds the new strings (in English) to all of our locales for future translation.
gk can confirm, but I think it is better to only add the strings to the en-US locale. The process of updating the strings from Transifex (which gk always does before making a new release of Torbutton) should take care of filling in missing strings with the en-US ones if they are not yet translated.
Changed 8 months ago by
comment:74 Changed 8 months ago by
Ok, added (and verified) fallback code in the event that the torbutton string query fails for whatever reason to security.js, and I removed the non-english locales from the torbutton change.
comment:76 Changed 8 months ago by
Keywords: | TorBrowserTeam201806 added; TorBrowserTeam201806R removed |
---|---|
Status: | needs_review → needs_revision |
Just some nits and then we are ready to ship
s/tor-button's/Torbutton's/
s/implicity/implicitly/
Please no mixed braces usage in
+ if (!info.isOnion) { + hdr = pkiBundle.getString("pageInfo_NoEncryption"); + if (info.hostName != null) + msg1 = pkiBundle.getFormattedString("pageInfo_Privacy_None1", [info.hostName]); + else + msg1 = pkiBundle.getString("pageInfo_Privacy_None4"); + msg2 = pkiBundle.getString("pageInfo_Privacy_None2");
. Just use braces everywhere in it (even though the Mozilla code itself violates its guidelines here).
Changed 8 months ago by
comment:77 Changed 8 months ago by
Keywords: | TorBrowserTeam201806R added; TorBrowserTeam201806 removed |
---|---|
Status: | needs_revision → needs_review |
Fixed the final nits
comment:78 Changed 8 months ago by
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
Looks good. It seems we are done here, nice work Richard! I applied the patches to master
in our Torbutton repo (commit 65b5371aadb32835b99ca56fc8feb43bef81a78e) and to tor-browser-60.0.1esr-8.0-1
in our tor-browser repo (commit commit 06fd55e2e061c7348f86d7f0ced3d64ffac9baad).
Related old ticket: #8686