[tbb-bugs] #26690 [Applications/Tor Browser]: TBA: Port padlock states for .onion services to mobile

[Tor Bug Tracker & Wiki]

#26690: TBA: Port padlock states for .onion services to mobile
-------------------------------------------------+-------------------------
 Reporter:  igt0                                 |          Owner:  tbb-
                                                 |  team
     Type:  enhancement                          |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-mobile, TBA-a2,                  |  Actual Points:
  TorBrowserTeam201810                           |
Parent ID:  #5709                                |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor8
-------------------------------------------------+-------------------------

Comment (by igt0):

 Replying to [comment:7 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26690#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online