Opened 5 months ago

Closed 13 days ago

Last modified 13 days ago

#26690 closed enhancement (fixed)

TBA: Port padlock states for .onion services to mobile

Reported by: igt0 Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, TBA-a2, TorBrowserTeam201811R
Cc: sysrqb, gk, antonela Actual Points:
Parent ID: #5709 Points:
Reviewer: Sponsor: Sponsor8

Description

The same work made in the #23247 needs to be ported to mobile.

Child Tickets

TicketTypeStatusOwnerSummary
#25765defectclosedtbb-teamTBA - Communicating security expectations for .onion: what to say about different padlock states for .onion services

Attachments (5)

Change History (28)

comment:1 Changed 5 months ago by gk

Keywords: tbb-mobile added
Type: defectenhancement

comment:2 Changed 4 months ago by sysrqb

Parent ID: #5709

comment:3 Changed 3 months ago by sysrqb

Keywords: TBA-a2 added

I'd like this, but not required.

comment:4 Changed 3 months ago by igt0

Status: newneeds_review

https://trac.torproject.org/projects/tor/attachment/ticket/26690/0001-Bug-26690-Port-padlock-states-for-.onion-services-to.patch

Bug 26690 - Port padlock states for .onion services to mobile


Prior to this patch, TBA was showing all onion services as SSL/TLS
encrypted connections(lock icon).
This patch fixes the issue adding several new Onion icons to indicate
all the various permutations of the Onions services hosted HTTP or HTTPS
pages.

comment:5 Changed 3 months ago by gk

Keywords: TorBrowserTeam201809R added

comment:6 Changed 2 months ago by gk

Keywords: TorBrowserTeam201810R added; TorBrowserTeam201809R removed

Moving review tickets to October

comment:7 Changed 8 weeks ago by sysrqb

Status: needs_reviewneeds_revision

Nice, looks good! Only some minor comments.

From the commit message:

 Prior to this patch, TBA was showing all onion services as SSL/TLS
    encrypted connections(lock icon).

Nit: That isn't true, is it? Maybe I'm misreading it. Currently, onion sites served without TLS do not show a lock icon, right?


+    <item
+        android:drawable="@drawable/ic_lock_onion_active"
+        android:maxLevel="8"/>

I feel like "onion_active" doesn't describe the state well. Can we call it onion_lock (or something similar)? When I see IconType.ONION_ACTIVATE in the code, I don't immediately remember what that means. I think IconType.ONION_LOCK be a little better. And maybe the icons that don't contain locks shouldn't have "lock" in their name? Thoughts?


@@ -100,6 +103,8 @@ public class SecurityModeUtil {
         final MixedMode displayMixedMode = identity.getMixedModeDisplay();
         final TrackingMode trackingMode = identity.getTrackingMode();
         final boolean securityException = identity.isSecurityException();
+       final boolean isOnionHost = identity.isOnionHost();
+       final boolean hasCert = identity.hasCert();

Nit: Please correct the indentation


@@ -119,9 +124,15 @@ public class SecurityModeUtil {
             return IconType.DEFAULT;
         }
 
-        return securityModeMap.containsKey(securityMode)
-                ? securityModeMap.get(securityMode)
-                : IconType.UNKNOWN;
+        if (securityMode == SecurityMode.UNKNOWN) {
+            return isOnionHost ? IconType.ONION : IconType.UNKNOWN;
+       } else if (securityMode == SecurityMode.IDENTIFIED) {
+            return isOnionHost ? (hasCert ? IconType.ONION_ACTIVATE : IconType.ONION) : IconType.LOCK_SECURE;
+       } else if (securityMode == SecurityMode.VERIFIED) {
+            return isOnionHost ? IconType.ONION_ACTIVATE : IconType.LOCK_SECURE;
+       } else {
+            return IconType.UNKNOWN;
+        }
     }

Nit. Here, too.


-                mIcon.setImageResource(R.drawable.ic_lock_disabled);
+                int resId = siteIdentity.isOnionHost() ? R.drawable.ic_lock_onion_disabled : R.drawable.ic_lock_disabled;
+                mIcon.setImageResource(resId);

These changes in mobile/android/base/java/org/mozilla/gecko/toolbar/SiteIdentityPopup.java are #27657, so I'm not sure we should implement this yet (although I like it).


-        final boolean isIdentityKnown = (siteIdentity.getSecurityMode() == SecurityMode.IDENTIFIED ||
-                                         siteIdentity.getSecurityMode() == SecurityMode.VERIFIED);
+        final boolean isIdentityKnown = ((siteIdentity.getSecurityMode() == SecurityMode.IDENTIFIED ||
+                                         siteIdentity.getSecurityMode() == SecurityMode.VERIFIED) &&
+                                         siteIdentity.hasCert());

Is this needed? Can the SecurityMode be Verified or Identified without a cert? (I don't see a code path that allows this, but it's possible I missed it)


I created some (simple) test pages for this:
https://people.torproject.org/~sysrqb/mixed_content/
http://sbe5fi5cka5l3fqe.onion/~sysrqb/mixed_content/

The fact Mozilla re-implemented all of this logic for mobile instead of reusing some of the existing browser functionality is really annoying.

comment:8 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201810R removed

comment:9 Changed 7 weeks ago by pili

Sponsor: Sponsor8

comment:10 in reply to:  7 Changed 6 weeks ago by igt0

Replying to sysrqb:

Nice, looks good! Only some minor comments.

From the commit message:

 Prior to this patch, TBA was showing all onion services as SSL/TLS
    encrypted connections(lock icon).

Nit: That isn't true, is it? Maybe I'm misreading it. Currently, onion sites served without TLS do not show a lock icon, right?

Yeah, you are right. I updated the commit msg.


+    <item
+        android:drawable="@drawable/ic_lock_onion_active"
+        android:maxLevel="8"/>

I feel like "onion_active" doesn't describe the state well. Can we call it onion_lock (or something similar)? When I see IconType.ONION_ACTIVATE in the code, I don't immediately remember what that means. I think IconType.ONION_LOCK be a little better. And maybe the icons that don't contain locks shouldn't have "lock" in their name? Thoughts?


I renamed them.

@@ -100,6 +103,8 @@ public class SecurityModeUtil {
         final MixedMode displayMixedMode = identity.getMixedModeDisplay();
         final TrackingMode trackingMode = identity.getTrackingMode();
         final boolean securityException = identity.isSecurityException();
+       final boolean isOnionHost = identity.isOnionHost();
+       final boolean hasCert = identity.hasCert();

Nit: Please correct the indentation


Duh! Updated it.

@@ -119,9 +124,15 @@ public class SecurityModeUtil {
             return IconType.DEFAULT;
         }
 
-        return securityModeMap.containsKey(securityMode)
-                ? securityModeMap.get(securityMode)
-                : IconType.UNKNOWN;
+        if (securityMode == SecurityMode.UNKNOWN) {
+            return isOnionHost ? IconType.ONION : IconType.UNKNOWN;
+       } else if (securityMode == SecurityMode.IDENTIFIED) {
+            return isOnionHost ? (hasCert ? IconType.ONION_ACTIVATE : IconType.ONION) : IconType.LOCK_SECURE;
+       } else if (securityMode == SecurityMode.VERIFIED) {
+            return isOnionHost ? IconType.ONION_ACTIVATE : IconType.LOCK_SECURE;
+       } else {
+            return IconType.UNKNOWN;
+        }
     }

Nit. Here, too.


Okey dockey.

-                mIcon.setImageResource(R.drawable.ic_lock_disabled);
+                int resId = siteIdentity.isOnionHost() ? R.drawable.ic_lock_onion_disabled : R.drawable.ic_lock_disabled;
+                mIcon.setImageResource(resId);

These changes in mobile/android/base/java/org/mozilla/gecko/toolbar/SiteIdentityPopup.java are #27657, so I'm not sure we should implement this yet (although I like it).


I think we should because without this patch for some cases it is showing the string "Null" and it is ugly and misleading.

-        final boolean isIdentityKnown = (siteIdentity.getSecurityMode() == SecurityMode.IDENTIFIED ||
-                                         siteIdentity.getSecurityMode() == SecurityMode.VERIFIED);
+        final boolean isIdentityKnown = ((siteIdentity.getSecurityMode() == SecurityMode.IDENTIFIED ||
+                                         siteIdentity.getSecurityMode() == SecurityMode.VERIFIED) &&
+                                         siteIdentity.hasCert());

Is this needed? Can the SecurityMode be Verified or Identified without a cert? (I don't see a code path that allows this, but it's possible I missed it)

Yeah, it should't. However I reversed engineered the https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-60.3.0esr-8.5-1&id=c3775a668d0c9fae044aa26ed85994139590c21d

And it always checked for the cert.


I created some (simple) test pages for this:
https://people.torproject.org/~sysrqb/mixed_content/
http://sbe5fi5cka5l3fqe.onion/~sysrqb/mixed_content/

The fact Mozilla re-implemented all of this logic for mobile instead of reusing some of the existing browser functionality is really annoying.

comment:12 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201810 removed

comment:13 Changed 3 weeks ago by gk

Cc: antonela added
Keywords: TorBrowserTeam201811 added; TorBrowserTeam201811R removed
Status: needs_reviewneeds_revision

Sorry for the delay here. Just some nits I found:

1) indentation (in security_mode_icon_nm.xml)

+        android:drawable="@drawable/ic_onion_disabled"
+	android:maxLevel="9"/>

2) In the commit message

s/Onion icons/onion icons
s/of the Onion services hosted/of onion services hosted/

antonela: I felt the onion icon (not the one for extended validation nor the one showing insecure content) was too big. Compared to the desktop, where the icon stays the same size-wise compared to the lock icon, the onion icon seems to get bigger on mobile. Could we fix that or do you want to leave it as-is?

Oh, and, yes, having the things scheduled for #27657 already done here for mobile is pretty cool!

comment:14 Changed 2 weeks ago by igt0

Status: needs_revisionneeds_review

comment:15 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed

comment:16 in reply to:  13 ; Changed 2 weeks ago by antonela

Replying to gk:

antonela: I felt the onion icon (not the one for extended validation nor the one showing insecure content) was too big. Compared to the desktop, where the icon stays the same size-wise compared to the lock icon, the onion icon seems to get bigger on mobile. Could we fix that or do you want to leave it as-is?

I just tested the .apk and yes is big. The size should be the same size we have at the lock icon (24px). If we can fix it, would be awesome.

Igt0 let me know if you need any asset.

Last edited 2 weeks ago by antonela (previous) (diff)

comment:17 in reply to:  16 Changed 2 weeks ago by sysrqb

Replying to antonela:

Replying to gk:

antonela: I felt the onion icon (not the one for extended validation nor the one showing insecure content) was too big. Compared to the desktop, where the icon stays the same size-wise compared to the lock icon, the onion icon seems to get bigger on mobile. Could we fix that or do you want to leave it as-is?

I just tested the .apk and yes is big. The size should be the same size we have at the lock icon (24px). If we can fix it, would be awesome.

Igt0 let me know if you need any asset.

I see the SVG used in #23247 is 16x16 (by default?), but the PNG used here is showing as 36x36.

Overall, I think the patch looks good.

comment:18 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201811R removed
Status: needs_reviewneeds_revision

Setting this as needs_revision for the icon issue.

comment:19 Changed 2 weeks ago by antonela

The original assets have a white space around the icon, this is why it looks smaller.
Already provided assets with that white space to igt0 for implementation.

Changed 2 weeks ago by antonela

Attachment: assets.zip added

TBA Alpha - Onion padlock assets

comment:20 Changed 2 weeks ago by gk

igt0: How is it going with the updated icon? I'd like to get that patch into the upcoming alpha.

comment:22 Changed 13 days ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good now, thank! Applied to tor-browser-60.3.0esr-8.5-1 (commit e5659b39d8bc51faecac5daa7f2cb9b63d4dccf8).

FWIW: I found a fun bug while testing (thinking first it is related to this patch) where the security state is vanishing while a download is happening if one clicks on the lock icon. But that's actually a Fennec issue which is tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=1511003.

comment:23 Changed 13 days ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed
Note: See TracTickets for help on using tickets.