Closed Bug 1018994 Opened 6 years ago Closed 4 years ago

Favicons in Awesomebar allow HTTPS spoofing

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed
relnote-firefox --- 47+
fennec 47+ ---

People

(Reporter: gcp, Assigned: sebastian)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

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.
Screenshot of the only time Chrome shows the favicon.
Ian, what do you want to do here?
tracking-fennec: --- → ?
Flags: needinfo?(ibarlow)
tracking-fennec: ? → +
Flags: needinfo?(ibarlow) → needinfo?(ywang)
Here's a nice POC with our matching asset http://people.w3.org/mike/phish/
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.
filter on [mass-p5]
Priority: -- → P5
Depends on: 1111818
Duplicate of this bug: 1225229
Anthony, let's revisit this bug.
Flags: needinfo?(ywang) → needinfo?(alam)
Attached image Fennec Screenshot
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.
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?
Flags: needinfo?(alam)
(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.
(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.
Flags: needinfo?(gpascutto)
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.
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.
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?
Flags: needinfo?(gpascutto)
> 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.
tracking-fennec: + → ?
I think we should make an Aha! card for fixing spoofing problems in the urlbar, which would including fixing this bug and bug 1236431.
tracking-fennec: ? → 47+
Flags: needinfo?(bbermes)
Flags: needinfo?(alam)
Assignee: nobody → s.kaspari
Attached image no-favicon.png (obsolete) —
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.
Attachment #8709150 - Flags: feedback?(alam)
Nice patch. Nice screenshot.
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?
Flags: needinfo?(alam) → needinfo?(s.kaspari)
(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.
Flags: needinfo?(s.kaspari)
Depends on: 1242500
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.
Attached image no-favicon-updated.png (obsolete) —
Attachment #8709150 - Attachment is obsolete: true
Attachment #8709150 - Flags: feedback?(alam)
Blocks: 1242303
This is the URL bar with updated dimensions.
Attachment #8711683 - Attachment is obsolete: true
Attachment #8713312 - Flags: feedback?(alam)
No longer blocks: 1242303
Comment on attachment 8713312 [details]
urlbar_without_favicon.png

A+ :D
Attachment #8713312 - Flags: feedback?(alam) → feedback+
Attached patch 1018994-Remove-Favicon.patch (obsolete) — Splinter Review
Updated patch with the dimensions from antlam's spec. I tested this patch on tablets too to avoid regressions here.
Attachment #8709149 - Attachment is obsolete: true
Attachment #8713553 - Flags: review?(michael.l.comella)
I agree with :antlam that :sebastian's updated urlbar looks great.
Attachment #8713553 - Attachment is obsolete: true
Attachment #8713553 - Flags: review?(michael.l.comella)
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/
Attachment #8709149 - Attachment description: MozReview Request: Bug 1018994 - Do not show favicon in location bar. r? → MozReview Request: Bug 1018994 - Remove favicon from the url bar. r?mcomella
Attachment #8709149 - Attachment is obsolete: false
Attachment #8709149 - Flags: review?(michael.l.comella)
The new patch optimizes the touch area: Using padding instead of margin.
Sebastian, I'll get to this tomorrow for sure.
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 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/
I updated the patch to include the "flag" changes from bug 1236431.
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.
Attachment #8709149 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/mozilla-central/rev/7458513998f5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1248437
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)]:
relnote-firefox: --- → ?
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
QA Contact: sorina.florean
You need to log in before you can comment on or make changes to this bug.