312363 - document.write into iframe results in broken-lock icon (bankhapoalim.co.il incorrectly indicated as only partially encrypted)

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[Tsahi Asher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.7.12) Gecko/20050915
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.8b4) Gecko/20050913 Firefox/1.4

when entering the account management in Bank Hapoalim web site, firefox 1.5beta1
indicates that the page is only partially enchrypted. the lock at the status bar
and location bar is closed, with a red line across it. SeaMonkey 1.0alpha
(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050910
SeaMonkey/1.0a) indicates the same problem. on the other side, firefox 1.0.7 and
mozilla suite 1.7.12 do not, and show the page to be totally enchrypted.

Reproducible: Always

Steps to Reproduce:
1. open an account at Bank Hapoalim
2. sign the papers for internet access to the account
3. go to the above url and enter your account

Actual Results:  
browser reports that some elements in the page are not enchrypted

Expected Results:  
since i didn't find the string "http:" in the three main frames that make the
page, i assume i should expect to see a full enchryption indication.

here is the frameset they use:
<FRAMESET COLS="8, *, 8" border="0" onUnload="doOnUnLoad()"
onBeforeUnload="doOnUnLoad()" framespacing="0" frameborder="0"
id="FRAMESET_TIME_OUT">
<FRAME noresize="true" marginwidth="0" name="SPACE_LEFT"
SRC="javascript:top.nullPage(1);" scrolling="no"
marginheight="0"></FRAME><FRAMESET ROWS="94,*" border="0" framespacing="0"
frameborder="0">
<FRAME noresize="true" marginwidth="0" name="UPPER"
SRC="javascript:top.nullPage(1);" scrolling="no"
marginheight="0"></FRAME><FRAMESET COLS="*, 170, 3" border="0" framespacing="0"
frameborder="0" id="FRAMESET_BODY">
<FRAMESET ROWS="0,*" border="0" onLoad="top.initMenus();" framespacing="0"
frameborder="0">
<FRAME marginwidth="0" name="STAM" SRC="/cgi-bin/poalwwwc?reqName=stamFrame&u=p"
scrolling="no" marginheight="0"></FRAME><FRAME marginwidth="0" name="BODY"
SRC="javascript:top.nullPage(1);" scrolling="auto"
marginheight="0"></FRAME></FRAMESET>
        <FRAME marginwidth="0" name="LOWER" SRC="javascript:top.nullPage(1);"
scrolling="auto" marginheight="0"></FRAME><FRAME noresize="true" marginwidth="0"
name="RED_LINE" SRC="javascript:top.nullPage(1);" scrolling="no"
marginheight="0"></FRAME></FRAMESET></FRAMESET><FRAME noresize="true"
marginwidth="0" name="SPACE_RIGHT" SRC="javascript:top.nullPage(1);"
scrolling="auto" marginheight="0"></FRAME>

<NOFRAMES>
  This site requires at least Netscape 2.0, with JavaScript Enabled.
 </NOFRAMES></FRAMESET>

it comes after many lines of javascript code. i can't know how many through,
because the status bar was striped from firefox' source viewer.

Comment 1

[Daniel Veditz [:dveditz]]

Steps 1 and 2 are rather large hurdles. Could you capture an http log and attach
it to the bug? we may or may not learn something that way, but might show if the
site itself is to blame (or eliminate that possibility).

See http://www.mozilla.org/projects/netlib/http/http-debugging.html

I only want the http headers, so "set NSPR_LOG_MODULES=nsHttp:5" leaving out the
socket and host resolver bits. It would be most useful if you did this for 1.0.7
(working) and then the latest buggy Firefox or Seamonkey. Note that the log file
gets overwritten each time you launch so either move it between runs, or set it
to a different name when you are about to run the other version.

It would help keep the log file size down if you launch and go right to the bank
and then quit.

You'll probably want to edit the files to remove any Cookie lines. If the site
has decent security the cookies will only be useless tokens tied to your IP
address and that expire quickly, but why take the chance? Ditto for any part of
a URL after the first '?' -- I don't need those either and they might reveal
account information

If you want you can mail the logs directly to me, and I'll attach them to the
bug only if they show something worth following up.

Comment 2

[Tsahi Asher]

dveditz, i sent you a zip with the logs by email, from my other address.

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: zipped log files, ff1.0.7 vs ff1.5

Thanks for the logs, these look clean so I'm attaching them.

Of course I can't tell for sure how the HTML is structured, but in both Firefox
logs we load the non-ssl
http://students.bankhapoalim.co.il/cgi-bin/inetcgi/student/exitGame.jsp and
images/scripts/css referenced from that file.

Is this loaded in a frame, and therefore FF1.5 is fixing a 1.0.x bug? Or is
that just something that was loaded while you're exiting and the problem is
elsewhere. While you're on the mixed page could you look in Page Info and see
what else is loaded? The page-info dialog in Seamonkey is more useful for this
than the default one in Firefox, it has extra categories/tabs.

Do any of you folks who cc'd yourselves also see this, or did it just sound
like an interesting bug?

Have you tried another browser such as IE or Opera? Do they show mixed?

Comment 4

[Tsahi Asher]

(In reply to comment #3)
> Of course I can't tell for sure how the HTML is structured, but in both Firefox
> logs we load the non-ssl
> http://students.bankhapoalim.co.il/cgi-bin/inetcgi/student/exitGame.jsp and
> images/scripts/css referenced from that file.
> 
> Is this loaded in a frame, and therefore FF1.5 is fixing a 1.0.x bug? Or is
> that just something that was loaded while you're exiting and the problem is
> elsewhere.

this page is where i'm taken when i logout of the account, because i have a
student account.

when i logout, in any of the four browsers, i get an alert: "although this page
is encrypted, the information you entered will be sent over an unencrypted
connection and can easily read by a third party..." (i'm back translating from
hebrew to english here, because i'm using my localized version).

> While you're on the mixed page could you look in Page Info and see
> what else is loaded? The page-info dialog in Seamonkey is more useful for this
> than the default one in Firefox, it has extra categories/tabs.
> 
links: all links are javascipt: links.
images: all images are loaded from https: urls.
forms: one form, submitted to an https: url.
framse: 7 frames, all but one are javascript: frames. the one that isn't is a
https: frame.

in any of the other branches in the Privacy tab there isn't any non-https url.

> Do any of you folks who cc'd yourselves also see this, or did it just sound
> like an interesting bug?
> 

this problem was first noticed by one of our users (i head the hebrew l10n team)
on firefox 1.5b2, and others verified, so others see this as well.

> Have you tried another browser such as IE or Opera? Do they show mixed?

IE6 doesn't report any problem. i don't have opera installed.

Comment 5

[Uri Bernstein]

Confirming, based on the reports and based on the last comment, which seems to
indicate this is a real bug.

Comment 6

[Uri Bernstein]

Please note that while this might not be a security problem per se, it's certainly a big problem with the perception of security, and as such, I think efforts should be made so that it is fixed for the 1.5 release.

As it is now we have two options:
1. Tell people to ignore the "broken lock" UI, assuring them that the site is actually secure (very bad idea, because users will then ignore it also when they shouldn't).
2. Tell people not to use Firefox on this site. Once again, not a good idea. Bank Hapoalim is the largest bank in Israel, with hundreds of thousands (perhaps even millions) of customers.

Comment 7

[Tsahi Asher]

requesting blocking aviary2, 1.8rc1

Comment 8

[Reuven Gonen]

tracking back nightlies, the bug seems to appear on the trunk between 2-9-2005 and 3-9-2005. the link to bonsai is:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-02+07%3A19&maxdate=2005-09-03+06%3A54&cvsroot=%2Fcvsroot

Comment 9

[Jesse Ruderman]

Could the regression have been caused by the change in bug 298064?  I don't understand the code involved so I'm just making a wild guess.

Comment 10

[Reuven Gonen]

(In reply to comment #9)
> Could the regression have been caused by the change in bug 298064?  I don't
> understand the code involved so I'm just making a wild guess.
> 

CC peterv based on that assumption

Comment 11

[Peter Van der Beken [:peterv]]

Does the page use document.open/document.write? Bug 298064 did change which document we use to propagate security info when using document.open().
We really need a (simple) testcase to figure this out.

Comment 12

[Tsahi Asher]

Attachment: (somewhat) simplified testcase

this is the main frame of the account page, usually displaying the account info. as you can see, it has several document.write()'s.

i removed any account info, as much as i could find. tell me if i missed something.

it's not very simplified, but it's not too big either. i just didn't know what will interest you, as javascript often access the DOM.

this is just one option for this frame - the default frame you get when you access the account. as you select to get other type of information, this frame is replaced to show that info.

Comment 13

[Jesse Ruderman]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5

I don't see a broken lock icon when I load the attachment in comment 12.

Comment 14

[Jesse Ruderman]

I tried to contact the bank about this problem a few days ago, but their contact form is broken and webmaster@ bounces.

Comment 15

[Uri Bernstein]

(In reply to comment #14)
> I tried to contact the bank about this problem a few days ago, but their
> contact form is broken and webmaster@ bounces.

I just tried the contact form at http://www.bankhapoalim.com/wps/portal/!ut/p/_.cmd/cs/ce/7_0_A/s./7_0_DR/_s.7_0_A/7_0_DR?displayJsp=intView1.jsp&area=a18 (with test data) and it seems to work (I got a "thank you" page). Perhaps you should try again?

Comment 16

[Jesse Ruderman]

It worked once I rewrote my comment to not use parentheses or hyphens.

Comment 17

[Jesse Ruderman]

or apostrophes.

Comment 18

[Asa Dotzler [:asa]]

cannot reproduce with simplified testcase on latest branch build on windowsXP. If  it's the case that we're saying it's less secure than it is, that's not as horrible as the other way around.

Comment 19

[Tsahi Asher]

(In reply to comment #14)
> I tried to contact the bank about this problem a few days ago, but their
> contact form is broken and webmaster@ bounces.
> 

they also have a phone number. from abroad, it would be [phone]. they didn't say what the hours are, but israel is UTC+2

Comment 20

[Tsahi Asher]

(In reply to comment #18)
> cannot reproduce with simplified testcase on latest branch build on windowsXP.
> If  it's the case that we're saying it's less secure than it is, that's not as
> horrible as the other way around.
> 

i can upload a more complete frameset, but i don't think i can make bugzilla serve them all together in one page.

Comment 21

[Uri Bernstein]

(In reply to comment #20)
> i can upload a more complete frameset, but i don't think i can make bugzilla
> serve them all together in one page.

You can upload the individual frames, then change the frameset itself to use the bugzilla URLs of the frames you uploaded, and then upload it. Of course, if the frameset builds the URLs of the different frames dynamically, things might get more complicated. 

Comment 22

[Tsahi Asher]

(In reply to comment #21)
> (In reply to comment #20)
> > i can upload a more complete frameset, but i don't think i can make bugzilla
> > serve them all together in one page.
> 
> You can upload the individual frames, then change the frameset itself to use
> the bugzilla URLs of the frames you uploaded, and then upload it. Of course, if
> the frameset builds the URLs of the different frames dynamically, things might
> get more complicated. 
> 
i thought about it, but the frameset also calls two javascript files, and the .js extension will disappear in bugzilla.

i'll try uploading just the html files, and see if it helps.

Comment 23

[Uri Bernstein]

Hmmm, I went trough the attached log and noticed this:

0[2745e0]: http request [
0[2745e0]:   GET /newimages/Internet_banner.gif HTTP/1.1
0[2745e0]:   Host: newsletter.bankhapoalim.co.il
0[2745e0]:   User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
0[2745e0]:   Accept: image/png,*/*;q=0.5
0[2745e0]:   Accept-Language: he-il,he;q=0.8,en-us;q=0.5,en;q=0.3
0[2745e0]:   Accept-Encoding: gzip,deflate
0[2745e0]:   Accept-Charset: windows-1255,utf-8;q=0.7,*;q=0.7
0[2745e0]:   Keep-Alive: 300
0[2745e0]:   Connection: keep-alive
0[2745e0]:   Referer: https://login.bankhapoalim.co.il/cgi-bin/poalwwwc?********************************
0[2745e0]: ]

Notice that the host here is newsletter.bankhapoalim.co.il, unlike all other requests where it is login.bankhapoalim.co.il. Is it possible that the broken lock is shown because content from two different hosts is shown on the page (even if they're both HTTPS)?

Comment 24

[Tsahi Asher]

Attachment: frame

Comment 25

[Tsahi Asher]

Attachment: MainFrameSet.js

Comment 26

[Tsahi Asher]

Attachment: MainFrameSetMenu.js

Comment 27

[Tsahi Asher]

Attachment: frameset

sorry for all the spam...

Comment 28

[Uri Bernstein]

(In reply to comment #23)
Furthermore, there are references to newsletter.bankhapoalim.co.il in log-firefox1.5b2.txt and log-seamonkey1.0a.txt, but not in log-firefox1.0.7.txt.

Tsahi, can you find the frame (or script) referencing https://newsletter.bankhapoalim.co.il/newimages/Internet_banner.gif, and see if it is different in 1.0.7 than in 1.5?

Comment 29

[Jesse Ruderman]

Attachment: guess at a simplified testcase

This testcase demonstrates a bug in Firefox that is a regression from Firefox 1.0.7 and causes the lock icon to become broken when it shouldn't.  It might not be the same bug this bank's site hits, but I'm willing to bet that it is.

Comment 30

[Jesse Ruderman]

Testing with my testcase, this regressed on the branch between Sept 4 and Sept 5.  This is consistent with the theory that my testcase tests the same bug and the regression was caused by the fix for bug 298064.  (The fix went into the trunk a few days before it went into the branch, so I'm not contradicting comment 8 and comment 9.)

Comment 31

[Jesse Ruderman]

Brendan Eich and Blake Kaplan are working on fixing this bug.

Comment 32

[Scott MacGregor]

this became a blocker today.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

jst is going to back out the offending part of the patch from bug 298064 on the branch. I'm going to submit a fix in a couple of minutes to fix this on trunk and leave bug 298064 fixed (and compatible with other browsers).

Comment 34

[Johnny Stenback (:jst)]

Attachment: Minimal backout of the change that caused this.

This is a staight backout of the nsContentUtils.cpp changes (modulo a #include change that conflicts with other changes made on the branch) that caused this bug. This is the minimal change that needs to be backed out.

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Trunk patch to fix

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Johnny backed out the offending bit of bug 298064, marking fixed1.8.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201585 [details] [diff] [review]
Trunk patch to fix

>Index: content/base/public/nsContentUtils.h
>+   * Get the document from the currently executing function's context (i.e.,
>+   * the static script global object).

I'm not sure I follow this comment.

In particular, it's not clear to me how this is different from GetDocumentFromContext().  I think the difference should be documented very very explicitly so people can tell which to use.

Also, is anyone calling GetDocumentFromContext?  I don't see it in this patch...

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 201585 [details] [diff] [review]
Trunk patch to fix

I forgot to diff nsHTMLDocument.cpp (the only user currently). I'll update the patch tomorrow.

Comment 39

[Reuven Gonen]

i logged in www.bankhapoalim.co.il with a build from 2005-11-02 and everything looks fine.

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Slightly better comments

The comments here are slightly better (suggestions on better comments are welcome) and I remembered to include nsHTMLDocument.cpp in the diff this time.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201688 [details] [diff] [review]
Slightly better comments

I still don't follow the difference.  What is the difference between "the currently executing function" and "the JS context currently on the stack" in terms of origins?

It would make the most sense to describe a situation in which those origins are different and explain which would give which origin...

Comment 42

[Johnny Stenback (:jst)]

Comment on attachment 201688 [details] [diff] [review]
Slightly better comments

r=jst with the comment changes...

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Boris, I've added this comment above the two functions in nsContentUtils.h, does it help?

  /*
   * The two GetDocumentFrom* functions below allow a caller to get at a
   * document that is relevant to the currently executing script.
   *
   * GetDocumentFromCaller gets its document by looking at the last called
   * function and finding the document that the function itself relates to.
   * For example, consider two windows A and B in the same origin. B has a
   * function which does something that ends up needing the current document.
   * If a script in window A were to call B's function, GetDocumentFromCaller
   * would find that function (in B) and return B's document.
   *
   * GetDocumentFromContext gets its document by looking at the currently
   * executing context's global object and returning its document. Thus,
   * given the example above, GetDocumentFromCaller would see that the
   * currently executing script was in window A, and return A's document.
   */

Comment 44

[Brendan Eich [:brendan]]

Comment on attachment 201688 [details] [diff] [review]
Slightly better comments

Suggest using static vs. dynamic to capture static scope vs. dynamic (caller) scope.  Caller (static, eek!) vs. Context is not clear -- these are not antonyms, and both reek of "dynamic scope".

/be

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201688 [details] [diff] [review]
Slightly better comments

sr=bzbarsky with the beefed-up comment, and a comment in nsHTMLDocument explaining why it's important to get the document from context, not caller.

Also, might be worth doing a followup on the renaming brendan suggests.

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk. I'll fix the function names seperately.

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 1.8 branch patch

This is an attempt to backport this patch the 1.8 branch. I haven't yet compiled it.

Comment 48

[Johnny Stenback (:jst)]

Blake, the patch doesn't compile, it needs #include "nsPIDOMWindow" to compile (which should be done by replacing the #include of "nsIDOMWindowInternal").

Does anyone know for sure that this really is a problem on the branch? I don't see the problem with the testcases in this bug. Anyone have any input to verify this either way?

Comment 49

[Mike Schroepfer]

I believe this is required on the branch for 298064 (at least that's what BKap says over there)

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, I put the patch here since that's where the trunk patch was, though that probably was a mistake. The question is whether or not we want to fix bug 298064 on the 1.8 branch (though I'm not sure how important that really is, now).

Comment 51

[Mike Schroepfer]

Comment on attachment 238242 [details] [diff] [review]
1.8 branch patch

clearing the 1.8.1 patch flag since this is not ready to go..

Comment 52

[Johnny Stenback (:jst)]

Comment on attachment 238242 [details] [diff] [review]
1.8 branch patch

r+sr=jst

Comment 53

[Jay Patel [:jay]]

Moving blocking flags from bug 298064 to this one.  Please ask for approval for patches on the appropriate branches.  Thanks!

Comment 54

[Jay Patel [:jay]]

Clearing the blocking1.8.1.1 flag... please correct me if I'm wrong in my understanding that this is already fixed on that branch.

Comment 55

[Jay Patel [:jay]]

Comment on attachment 238242 [details] [diff] [review]
1.8 branch patch

Approved for 1.8.0 branch, a=jay for drivers.  Please land this asap!

Comment 56

[Peter Van der Beken [:peterv]]

(In reply to comment #54)
> Clearing the blocking1.8.1.1 flag... please correct me if I'm wrong in my
> understanding that this is already fixed on that branch.

Actually, it is not fixed on either branch. Basically, the regression from bug 298064 that this bug is about was fixed on trunk and both branches, but the fix for bug 298064 was partly backed out to do that. Attachment 238242 [details] [diff] restores the fix for bug 298064, and we want it on both branches.
Maybe attachment 238242 [details] [diff] [review] should have been in bug 298064 so we don't confuse the fixed status of both bugs, but things are already confused enough as it is, so I'd rather leave it here and when it's checked in we'll mark bug 298064 as fixed on the branch too.

Comment 57

[Jay Patel [:jay]]

Comment on attachment 238242 [details] [diff] [review]
1.8 branch patch

Approved for 1.8.0 and 1.8.1 branches, a=jay for drivers.

Thanks peterv for the clarification.  Please land asap.

Comment 58

[Peter Van der Beken [:peterv]]

Checked in attachment 238242 [details] [diff] [review] on branches.

Comment 59

[Peter Van der Beken [:peterv]]

QA: when verifying, please use testcases in both this bug and in bug 298064 (attachment 194172 [details]) and then also mark bug 298064 verified on the branches. Thanks.

Comment 60

[Adam Guthrie]

Looks like juanb already verified bug 298064 on the branches. Verified FIXED using Jesse's "guess at simplified testcase" with the following builds:

* Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.9pre) Gecko/20061123 Firefox/1.5.0.9pre
* Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1pre) Gecko/20061123 BonEcho/2.0.0.1pre

Comment 61

[Boris Zbarsky [:bzbarsky]]

I can't seem to reproduce this bug on trunk even if I go back to GetDocumentFromCaller(), using the testcases attached here.  Am I missing something?

Comment 62

[Tsahi Asher]

maybe because it is VERIFIED FIXED?

Comment 63

[Boris Zbarsky [:bzbarsky]]

Which part of "even if I go back to GetDocumentFromCaller()" didn't make sense?