722037 - Security warning (viewing_mixed) shown on secure page which has images with javascript protocol

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[ypetrov]

Attachment: 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]

Attachment: Sniffer screensot

Comment 2

[ypetrov]

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]

Related to or dupe of bug 506008?

Comment 4

[ypetrov]

No. There are no redirects from http to https.
Also this behaviour introduced in FF9. FF8 doesn't show this warning.

Comment 5

[ypetrov]

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]

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 (he/him; ⌚ UTC+1/+2)]

Maybe bug 444641?

Comment 8

[Thomas Ahlblom]

The first bad revision is:
changeset:   77221:c237a8550070
user:        Boris Zbarsky <[e-mail]>
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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Correctly ignore javascript: images in security UI.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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:]]

> 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:]]

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:]]

Because this bug is a regression in shown security state, it should be considered for the extended support branch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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 [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a7f8c60ccc

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d0a7f8c60ccc

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
tracking for ESR Firefox 13

Comment 19

[Boris Zbarsky [:bzbarsky]]

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]

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 [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/0a153b10080b

Comment 22

[u279076]

@ypetrov, can you please confirm this is fixed in latest-mozilla-esr10?
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-esr10/

Comment 23

[u279076]

Verified fixed with Firefox 10.0.5esrpre 2012-05-31.