1018994 - Favicons in Awesomebar allow HTTPS spoofing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P5)

Description

[Gian-Carlo Pascutto [:gcp]]

Desktop Firefox removed Favicons in the Awesombar in Firefox 14. Bug 588270 and Bug 742419, among other. Some Googling finds that this was due the security concerns that exist when a site can spoof that a HTTPS connection is established.

For example: http://www.slideshare.net/SecurityTube.Net/black-hat-dc-09-marlinspike-defeating-ssl

One scenario is a login page on HTTP where the actual signin is supposed to happen over SSL. If you can MITM it, then feed a false "safe" favicon it appears that the signin in happening on the secure page while in fact it's still being MITM'ed. You can then continue feeding the favicon to give the impression the user is secured when he's in fact not.

POC:
http://sjeng.org/mozilla/phish/

Gives a false "EV lock" on Firefox for Android. Testing on a Nexus 7, Chrome is not vulnerable. I didn't check what they do on phones.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Only place Chrome shows the Favicon

Screenshot of the only time Chrome shows the favicon.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Ian, what do you want to do here?

Comment 3

[Aaron Train [:aaronmt]]

Here's a nice POC with our matching asset http://people.w3.org/mike/phish/

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Just want to note that this is fixed in the new tablet design (after bug 1066253 lands) which has a similar favicon/security layout to desktop.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 7

[:Margaret Leibovic]

Anthony, let's revisit this bug.

Comment 8

[April King [:April]]

Attachment: Fennec Screenshot

Comment 9

[April King [:April]]

Added a screenshot showing what it looks like when using the actual Fennec lock icon.  It's _really_ hard to tell that it's actually not a secure resource.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Thanks for the screen shots April!

If I'm understanding correctly, displaying an empty favicon in the comment 8 case would solve this issue right?

That is, just like our Tablet UI (mentioned in comment 4), we can always save that space to display something. 

Or am I missing something?

This reminds me of bug 1144430 where we talked about using the text to reinforce the actual nature of the site in question. Does that touch this issue mentioned here at all?

Comment 11

[Gian-Carlo Pascutto [:gcp]]

(In reply to Anthony Lam (:antlam) from comment #10)
> Thanks for the screen shots April!
> 
> If I'm understanding correctly, displaying an empty favicon in the comment 8
> case would solve this issue right?

I don't understand what "empty favicon" or "that case" means here. The site is supplying the false lock as its favicon. We cannot reliably detect that.

Aside from the obvious fix of never displaying favicons in that space to begin with, if we always display crossed through locks for HTTP sites (like Desktop does now) then the spoofability reduces, because the user will see both locks and hopefully be alarmed enough by the crossed out one to disregard the green one.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #11)
> (In reply to Anthony Lam (:antlam) from comment #10)
> > Thanks for the screen shots April!
> > 
> > If I'm understanding correctly, displaying an empty favicon in the comment 8
> > case would solve this issue right?
> 
> I don't understand what "empty favicon" or "that case" means here. The site
> is supplying the false lock as its favicon. We cannot reliably detect that.
> Aside from the obvious fix of never displaying favicons in that space to
> begin with, if we always display crossed through locks for HTTP sites (like
> Desktop does now) then the spoofability reduces, because the user will see
> both locks and hopefully be alarmed enough by the crossed out one to
> disregard the green one.

Sorry, I'm not fully understanding the context here. Can you help me understand a bit better?

That is, if a site sets their favicon to be a green lock, shouldn't we also have an icon (globe/caution/lock) next to it? But I only see 1 icon on this site. Why is that?

As for displaying a striked through lock for HTTP sites, I think that makes sense. I believe :liuche and myself followed :tanvi's lead here for the situations in order to match Desktops representation.

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

The site does not use any sort of encryption. Thus since it is not an encrypted site the site is free to show their own green icon.

It is not just green lock icons that are a concern. Anything that looks like it could be a sign of trust ex a gold lock, a green key, a realistic lock, and etc. Desktop has found that the only place that is safe to display the favicon is on the tab. Any location in the address bar is a risk for spoofing the user into thinking that they are on a secure site when they are not.

bug 729495 made this more difficult to detect. The demo page successfully made you believe that you were on an encrypted page.

Comment 14

[April King [:April]]

On a mobile browser, I think the only icon that should ever be displayed in the address bar is the lock icon indicating that it's a secure session. I don't think there's any need for a non-secure icon, crossed out page icons, or the like. If it's HTTPS, give them a lock, if not, no favicon. The spoofing risk is just too high.

There's still plenty of places to use the favicon (tabs, bookmarks, pages saved to the home screen, the recent app list), but it absolutely should not appear in the address bar, as the risk of phishing is too high. Desktop has taken things a step further and separated its iconography with a divider, but even there it doesn't use the favicon in the address bar. I think Fennec is the only major UA that still shows favicons next to the URL.

At some point, we'll start using slashed lock icons for http sites, but I suspect that will be years down the road given the current telemetry.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

I believe my needinfo? has been answered by kbrosnan. Basically, for an unencrypted site, if they supply a favicon we just display that and don't display our own lock or anything alike. If the favicon is a fake lock, that's the only thing that gets displayed.

>I think the only icon that should ever be displayed in the address bar is the lock icon indicating that it's a secure session.

Our of curiosity, what about mixed content?

Comment 16

[April King [:April]]

> Our of curiosity, what about mixed content?

Sorry, I should have been more clear.  :)

Status indicators (whether they be a lock icon, slashed lock, yellow warning triangle, etc.) are the only things that should appear there. Just no favicon. If we do decide we want to disable a downgrade icon, or a globe icon, then that's fine, just as long as it's not alongside a favicon that could indicate otherwise.

That said, I *personally* think we should have only two icon status: a lock icon (indicating that everything is okay) or a nothing (indicating that there's mixed content, no https, or whatever). Mixed content indicators or not, definitely no favicon.

Comment 17

[:Margaret Leibovic]

I think we should make an Aha! card for fixing spoofing problems in the urlbar, which would including fixing this bug and bug 1236431.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1018994 - Remove favicon from the url bar. r?mcomella

Review commit: https://reviewboard.mozilla.org/r/31305/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31305/

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: no-favicon.png

This patch does what we already do on tablets: Just show the site security icon but not the favicon.

To Do:
* Increase the touch area. It's much smaller now and hard to hit.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Nice patch. Nice screenshot.

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Thanks for the visuals Sebastian! :)

For some context, the favicon has always been used in other places for experiences around initiatives such as task continuity. It's a nice continuous visual that allows our page authors to express themselves and their content to come through.

But I think there's other ways to do this now. We've seen this in Google's <theme> tag and how that affects the Toolbar (for example). Now seems to me like the right time to find new ways to further that ideal and not depend on the favicon for that - while dealing with these security concerns.

tl;dr
 - I'm fine with removing it but I'd like to fix this "touch area" issue. Sebastian - if you have some time perhaps I could iterate with you on this via Vidyo in the scope of this bug?
 - the icon and URL looks misaligned now somehow, seems like a simple enough fix that we can do in here too?

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Anthony Lam (:antlam) from comment #21)
> tl;dr
>  - I'm fine with removing it but I'd like to fix this "touch area" issue.
> Sebastian - if you have some time perhaps I could iterate with you on this
> via Vidyo in the scope of this bug?
>  - the icon and URL looks misaligned now somehow, seems like a simple enough
> fix that we can do in here too?

Yeah, let's tweak that before landing. This is targeting 47 so we have a lot of time anyways. My patch did just remove the favicon view but maybe instead I should just use the view properties (size etc.) of the favicon view for the security icon.

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

I tried to optimize this a bit by using the measurements from the favicon view. It looks better but still not perfect (I'll attach a screenshot later). What makes this a bit challenging is that all the icons have different dimensions (values for HDPI):

* favicon_globe: 48x48
* lock_secure: 45x60
* warning_minor: 30x30
* lock_disabled: 58x60
* shield_enabled: 52x60
* shield_disabled: 58x60

In addition to that favicon_globe.png doesn't have a xxhdpi version. I filed bug 1242500 for that.

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: no-favicon-updated.png

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: urlbar_without_favicon.png

This is the URL bar with updated dimensions.

Comment 26

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8713312 [details]
urlbar_without_favicon.png

A+ :D

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1018994-Remove-Favicon.patch

Updated patch with the dimensions from antlam's spec. I tested this patch on tablets too to avoid regressions here.

Comment 28

[April King [:April]]

I agree with :antlam that :sebastian's updated urlbar looks great.

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8709149 [details]
MozReview Request: Bug 1018994 - Remove favicon from the url bar. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31305/diff/1-2/

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

The new patch optimizes the touch area: Using padding instead of margin.

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dd2635a103a

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sebastian, I'll get to this tomorrow for sure.

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8709149 [details]
MozReview Request: Bug 1018994 - Remove favicon from the url bar. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31305/diff/2-3/

Comment 34

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8709149 [details]
MozReview Request: Bug 1018994 - Remove favicon from the url bar. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31305/diff/3-4/

Comment 35

[Sebastian Kaspari (:sebastian; :pocmo)]

I updated the patch to include the "flag" changes from bug 1236431.

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8709149 [details]
MozReview Request: Bug 1018994 - Remove favicon from the url bar. r?mcomella

https://reviewboard.mozilla.org/r/31305/#review31307

I'm excited to clean up the visuals here – sliding the site security icon in and out caused a lot of polish issues.

Sorry for the delay – thanks Sebastian!

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:113
(Diff revision 4)
>      private AlphaAnimation mLockFadeIn;
>      private TranslateAnimation mTitleSlideLeft;
>      private TranslateAnimation mTitleSlideRight;

We should be able to remove these animations now.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:131
(Diff revision 4)
>      private PropertyAnimator mForwardAnim;

This is only assigned to now so we should be able to remove it.

Comment 38

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/7458513998f5c57e3227f0721e5bb18697823ce3
Bug 1018994 - Remove favicon from the url bar. r=mcomella

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7458513998f5

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Remove Favicons in Awesomebar to prevent HTTPS spoofing
[Links (documentation, blog post, etc)]:

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx 47 (Aurora) release notes

Comment 42

[Sorina Florean [:sflorean]]

Hi all, I will take this feature as a QA. Here is the Test plan based on which the testing is made: 
https://wiki.mozilla.org/QA/Fennec/Remove_Favicons_in_Awesomebar_to_prevent_HTTPS_spoofing