Last Comment Bug 722037 - Security warning (viewing_mixed) shown on secure page which has images with javascript protocol
: Security warning (viewing_mixed) shown on secure page which has images with j...
Status: VERIFIED FIXED
[qa+:ashughes]
: regression
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks: 444641
  Show dependency treegraph
 
Reported: 2012-01-28 04:34 PST by ypetrov
Modified: 2012-05-31 15:40 PDT (History)
10 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
13+
verified


Attachments
Warning screenshot (33.56 KB, image/png)
2012-01-28 04:34 PST, ypetrov
no flags Details
Sniffer screensot (36.08 KB, image/png)
2012-01-28 04:36 PST, ypetrov
no flags Details
Correctly ignore javascript: images in security UI. (2.75 KB, patch)
2012-02-07 21:33 PST, Boris Zbarsky [:bz] (still a bit busy)
kaie: review+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description ypetrov 2012-01-28 04:34:53 PST
Created attachment 592390 [details]
Warning screenshot

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

Enter https://mail.yandex.com.


Actual results:

"Security Warning" window appear (screenshot attached)
Traffic sniffers (Firebug, Charles, SmartSniff and Wireshark) doesn't capture any http requests, there are https and 443 port only.
Our guess is that CORS long polling request to https://webchat2.yandex.com triggers this warning. However blank page with that request doesn't reproduce that behaviour.


Expected results:

No "Security Warning" on these pages.
Also some debugging information on these warnings would be very helpful.
Comment 1 ypetrov 2012-01-28 04:36:06 PST
Created attachment 592391 [details]
Sniffer screensot
Comment 2 ypetrov 2012-01-28 04:48:19 PST
Steps to reproduce (updated):

1. Register and login at https://mail.yandex.com.
2. Go to Settings - Chat.
3. Check "Enable chat in Yandex.Mail".
4. "Save changes".
Comment 3 Mardeg 2012-01-28 04:59:22 PST
Related to or dupe of bug 506008?
Comment 4 ypetrov 2012-01-28 05:39:24 PST
No. There are no redirects from http to https.
Also this behaviour introduced in FF9. FF8 doesn't show this warning.
Comment 5 ypetrov 2012-01-30 04:02:30 PST
We've found what have triggered this warning. There is blank image with src="javascript:null" which considered non-secure.

Test page: https://locum.ru/YgpbrN1n7g/
Comment 6 Thomas Ahlblom 2012-01-30 05:17:23 PST
Using testcase in comment 5:

WFM:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.25) Gecko/20111212 Firefox/3.6.25
Opera/9.80 (X11; Linux x86_64; U; en) Presto/2.10.229 Version/11.61
Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.77 Safari/535.7

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a2) Gecko/20120129 Firefox/11.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120130 Firefox/12.0a1


Last good nightly: 2011-09-21
First bad nightly: 2011-09-22

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8bd19f6abbb&tochange=4495e1f795c2
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-01-30 05:20:34 PST
Maybe bug 444641?
Comment 8 Thomas Ahlblom 2012-01-30 08:20:45 PST
The first bad revision is:
changeset:   77221:c237a8550070
user:        Boris Zbarsky <bzbarsky@mit.edu>
date:        Tue Sep 20 17:00:42 2011 -0400
summary:     Bug 444641 part 3.  Propagate the loading principal to the image channel as needed.  r=joe,dveditz

https://hg.mozilla.org/mozilla-central/rev/c237a8550070
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-02-07 21:18:36 PST
That's ... interesting.  What used to happen there is that we didn't propagate a principal, so the javascript: URI never ran and the image never got loaded at all.

Now we _do_ load the image.  So now we get progress notifications for it.  nsSecureBrowserUIImpl::OnStateChange checks for and ignores such notifications for javascript: URIs, but only if the incoming request is an nsIChannel.  For images it would be an imgIRequest.  The javascript: check should probably move to after the imgIRequest block.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-02-07 21:33:10 PST
Created attachment 595312 [details] [diff] [review]
Correctly ignore javascript: images in security UI.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-02-07 21:35:55 PST
Oh, two more things:

1)  I really appreciate the problem diagnosis and simple testcase!
2)  Kai, how do I add a test for this?
Comment 12 Kai Engert (:kaie) 2012-02-21 09:49:50 PST
> 2)  Kai, how do I add a test for this?

Honza wrote all the tests for the secure page state that we have.
They are in security/manager/ssl/tests/mochitest/mixedcontent.

I assume you want the test to be:
- Have a https page
- page contains <img src="javascript:null">
- test that the security state is still "secure"

I have not played with the tests myself. I looked briefly, and test_securePicture.html could be a good minimal starting point for the current need. At the top of mixedContentTest.js is a helpful comment.

From my reading of the comment, I'd guess we need
  var bypassNavigationTest = true;

and 

  function runTest()
  {
    isSecurityState("secure", "image with null javascript is secure");
    finish();
  }
Comment 13 Kai Engert (:kaie) 2012-02-21 10:10:06 PST
Comment on attachment 595312 [details] [diff] [review]
Correctly ignore javascript: images in security UI.

Looks good to me, but I haven't tested it. Thanks for your help.

r=kaie
Comment 14 Kai Engert (:kaie) 2012-02-21 10:11:38 PST
Because this bug is a regression in shown security state, it should be considered for the extended support branch.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2012-02-21 15:00:24 PST
> They are in security/manager/ssl/tests/mochitest/mixedcontent.

Perfect, that's what I needed.  I have a test for this now.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-02-21 15:04:48 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a7f8c60ccc
Comment 17 Ed Morley [:emorley] 2012-02-22 10:39:37 PST
https://hg.mozilla.org/mozilla-central/rev/d0a7f8c60ccc
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-02-22 15:33:32 PST
[Triage Comment]
tracking for ESR Firefox 13
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2012-05-14 21:18:09 PDT
Comment on attachment 595312 [details] [diff] [review]
Correctly ignore javascript: images in security UI.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Suggested by PSM module peer.
User impact if declined: Some secure pages are flagged as insecure in the UI
Fix Landed on Version: Firefox 13.
Risk to taking this patch (and alternatives if risky): Low risk, I believe.
   Should only affect image loads from javascript: URIs.
String changes made by this patch:  None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-16 11:38:08 PDT
Comment on attachment 595312 [details] [diff] [review]
Correctly ignore javascript: images in security UI.

approving for esr landing based on improving the user experience with images in security UI
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2012-05-17 20:27:10 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/0a153b10080b
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-22 15:23:09 PDT
@ypetrov, can you please confirm this is fixed in latest-mozilla-esr10?
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-esr10/
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-31 15:40:24 PDT
Verified fixed with Firefox 10.0.5esrpre 2012-05-31.

Note You need to log in before you can comment on or make changes to this bug.