Closed Bug 84749 Opened 23 years ago Closed 23 years ago

BODY onUnload JavaScript gets wrong value for location.href

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.3

People

(Reporter: sdh4, Assigned: rpotts)

References

(
URL
)

Details

(Keywords: privacy, testcase, Whiteboard: PDT+)

Attachments

(2 files)

testcase
137 bytes, text/html
Details
Patch to fire UnLoad() event before changing any state in the DocShell...
11.09 KB, patch
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.16-3 i686; en-US; rv:0.9.1)
Gecko/20010607
BuildID:    2001060713

When the onUnload JavaScript is executed, location.href is the address of the
new web page, not the old web page. This allows the old web page to spy on where
you go to after you leave it (even if you leave by typing in a URL). 

This bug should be severity: security, except that classification does not exist
(bug #84746)

Reproducible: Always
Steps to Reproduce:
1.go to www.weimer.com (one of many sites run by netidentity.com)
2. Wait for an auto-redirect
3. Save the HTML for this webpage in a temporary place
4. go to your home page
5. A tiny hidden window has popped up (it may be difficult/impossible
to find. I can find it easily (~75x75) on Linux with IceWM window manager and
the GNOME pager. It usually hides on the left edge or lower right corner of the
screen. 
6. Right click on the hidden window and view page info. You will see your
homepage location encoded into its URL.
7. Look at the source you saved in step 3. In particular the BODY onUnload and
the function MakeRemoteWindow().
8. Quit your browser to get rid of the obnoxious JavaScript web bug. 

Actual Results:  I got my homepage location URL encoded into the web bug window
that popped up.

Expected Results:  Encoded the referring page (weimer.com)


<BODY leftMargin="20" topMargin="0" marginwidth="20" marginheight="0"
onUnload="MakeRemoteWindow();">

function MakeRemoteWindow() {

 // allow disabling of console
 if (remote == 1){
 return;
 }

 var baseurl = 'http://nitrous.exitfuel.com/';
 var console = baseurl +
 '?BPROGRAM=' + program + 
 '&' + 
 'LINKIN=' + linkin +
 '&' +
 'REF=' + location.href;

 H = window.open(console,"ef","height=1,width=1,left=10000,top=10000,location=0,
menubar=0,statusbar=0,locationbar=0,scrollbars=0,resizable=0");

}
Yikes!  over to DOM 0, ccing mstoltz and hyatt (since I recall hyatt doing
things with keeping the old doc in place till the new one comes in).

Steve, thanks for the excellent bug report.  Our apologies for taking so long to
get to it.
Assignee: joki → jst
URL: http://www.weimer.com
Status: UNCONFIRMED → NEW
Component: DOM Events → DOM Level 0
Ever confirmed: true
Keywords: privacy
QA Contact: vladimire → desale
Attached file testcase — 

    
  
Keywords: testcase
OS: Linux → All
Hardware: PC → All

    
    

    
  
Think bug 88684 is related to this.


    
    

    
  
This should be fixed for mozilla0.9.3.
Status: NEW → ASSIGNED
Keywords: mozilla0.9.3
Target Milestone: --- → mozilla0.9.3

    
    

    
  
Ok, so I talked to Rick about this and he did some investigation and thinks we
can simply move the fireing of the onunload handler earlier in the code so that
it's fired while window.location still has the value of the old page. Reassigning.
Assignee: jst → rpotts
Status: ASSIGNED → NEW

    
    

    
  
I forgot to mention that the patch is for the trunk :-)


    
    

    
  
nsDocShell::FireUnloadNotification() never uses 'rv', maybe remove it?

Do we really care about 'rv' at line ~3815?

In stead of making mFiredUnloadEvent a PRBool cound't you make it a PRPackedBool
and squeese it in with other smaller than 32-bit members?

Looks like we could loose 'rv' in DocumentViewerImpl::Unload() too.

Nit of the day, in nsDocShell::FireUnloadNotification():

+    if (mContentViewer && !mFiredUnloadEvent) {
+      mFiredUnloadEvent = PR_TRUE;
+
+        rv = mContentViewer->Unload();

Fix the indentation just below the if...
Other than that, sr=jst

    
    

    
  
I guess that in nsDocShell::FireUnloadNotifications(...) i'm not really sure
what the 'failure' policy should be :-)

One option is to fail if any of the nsIContentViewer::Unload() calls fail...
Another option is to always ignore the result code and continue notifying...

I have no idea which is the best option :-)  So, for now, I was capturing the
return code but ignoring it - best of both worlds :-)

I've made changes to ignore the return code from FireUnloadNotifications...

I was afraid to lump in the PRBool -> PRPackedBool changes into this patch
because it needs to land on the branch too :-)

I'll open up another bug to address the PRPackedBool issue specifically.

-- rick


    
    

    
  
This is Nisheeth typing at jst's computer.  r=nisheeth.

    
    

    
  
I've checked the changes into the trunk...  I'm just waiting for approval to
check them into the branch...


    
    

    
  
Adding nsBranch keyword to get more eyeballs on this security bug.
Keywords: nsBranch

    
    

    
  
The best (only, probably) way to do it is to e-mamil pdt2@netscape.com.

    
    

    
  
There's a whole lot going on in that patch.  Is there a simple version that
could be made for the branch?  Something like "deleted call from old place and
inserted it in new place"?  I'm hesitant to plus the existing patch.

    
    

    
  
hey steve,

believe it or not, that's basically what this patch is :-)

In the current implementation, nsWebShell.cpp has the code to iterate across
DocShells and fire the OnUnload(...) event- FireUnloadForChildrten(...).  This
code as been moved into nsDocShell.cpp as the FireUnloadNotification(...) method.

I needed to move this code into the DocShell, because this method needs to be
called from the middle of a method within nsDocShell.cpp

The other big part of the patch is moving the code that used to be in
nsWebShell::FireUnloadEvent(...) into nsDocumentViewer::Unload(...).  I decided
to do this (Rather than move it into the nsDocShell) because the OnLoad() event
is fired from the nsDocumentViewer too... and it is easier to get to the
information required to fire the event at the nsDocumentViewer level (than at
the nsDocShell/nsWebShell level).

Finally, the call to FireUnloadNotification() got moved from
nsWebShell::SetupNewViewer(...) into he middle of
nsDocShell::CreateContentViewer(...).  This is the critical part of the patch
(ie. the part that actually fixes the bug).  The rest of the patch is just the
housekeeping required to make the fix...

    
    

    
  
Oops, I forgot to add myself to the CC list and am only just seeing your
response :-(

Adding PDT+, please check in on the branch.  What can we do to ensure complete
confidence in this fix?  Do we have a targetted set of tests?  Can we invent a
set of tests?  Have people been stressing this area with trunk builds since the
patch went in?
Whiteboard: PDT+

    
    

    
  
The upshot of the fix is the thew OnUnload(...) script handler will be fired
earlier than before - assuming that all of the code is correct :-)

jst and i lxr'ed through the codebase to see what code relies on the
OnUnload(...) event... it looked like some of the wizards were the only consumers...

So, i think that if the Profile wizard and the mail account wizard do not show
regressions, then we should be in good shape...

I haven't heard of any regressions in the wizards since i landed this patch on
the trunk...

    
    

    
  
I've just checked the patch into the branch.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
Verified with 2001-07-24-06 branch.
Status: RESOLVED → VERIFIED

    
    

    
  
Is there a connection to bug 88684 ?

    
    

    
  
See also bug 145579, another privacy bug with the same impact.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: