Closed Bug 432591 Opened 16 years ago Closed 16 years ago

Fix for bug 428672 can be circumvented by using XUL element

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: jst)

References

Details

(Keywords: verified1.8.1.15, Whiteboard: [sg:high])

Attachments

(3 files, 1 obsolete file)

Fix (same as for bug 428672).
845 bytes, patch
sicking
: review+
sicking
: superreview+
damons
: approval1.9+
Details | Diff | Splinter Review
less mole-whacking
723 bytes, patch
Details | Diff | Splinter Review
1.8 branch version (tested, works).
1.81 KB, patch
dveditz
: review+
dveditz
: approval1.8.1.15+
Details | Diff | Splinter Review 
Instead of <body> element, a XUL element can be used to attach an event handler
to an outer window.
Flags: wanted1.9.0.x?
Flags: wanted1.8.1.x?
Flags: blocking1.9?
Flags: blocking1.8.1.15?
Whiteboard: [sg:high]
Attached patch like so? (obsolete) — — Splinter Review 
Assignee: nobody → crowder
Status: NEW → ASSIGNED

    
  

        Attachment #319837 -
        Flags: review?(jst)

    
    

    
  
This seems like a route to whack-mole. Can we fix nsDocument::GetInnerWindow to return null instead of an outer window? I started looking through callers (of which there are a lot fewer than nsDocment::GetScriptGlobalObject) and it seems like all of them would be happy with such a change.

    
    

    
  


  
Assignee: crowder → jst

        Attachment #319840 -
        Flags: superreview?(jonas)

        Attachment #319840 -
        Flags: review?(jonas)

    
    

    
  


  
This does fix the bug...  we can fix null-derefs whack-a-mole style (and easily), and at least it's not an XSS vulnerability.

        Attachment #319837 -
        Attachment is obsolete: true

        Attachment #319837 -
        Flags: review?(jst)

    
  

        Attachment #319842 -
        Flags: review?(jst)

        Attachment #319840 -
        Flags: superreview?(jonas)

        Attachment #319840 -
        Flags: superreview+

        Attachment #319840 -
        Flags: review?(jonas)

        Attachment #319840 -
        Flags: review+

    
    

    
  
Comment on attachment 319842 [details] [diff] [review]
less mole-whacking

Sorry, didn't see your original patch before I attached mine :(

We do want to do this, but not now IMO. The regression risk from this is unfortunately pretty high (as we've witnessed before), so we should do this as part of 431767 early in a release instead of now.

        Attachment #319842 -
        Flags: review?(jst)

    
    

    
  
Comment on attachment 319840 [details] [diff] [review]
Fix (same as for bug 428672).

a1.9+=damons

        Attachment #319840 -
        Flags: approval1.9? → approval1.9+

    
  
Depends on: 431767

    
  
Flags: blocking1.9? → blocking1.9+

    
    

    
  
jst, so I understand, you're advocating for your patch now, and crowder's patch as a 1.9.0.x thing? If so, can we get a new bug filed on that with crowder's patch attached so we can continue to track that? thx.
Whiteboard: [sg:high] → [sg:high][has patch][has review][has approval]

    
    

    
  
Beltzner, yes, bug 431767 already on file.

Fix checked in.
Flags: wanted1.9.0.x?
OS: Windows XP → All
Hardware: PC → All

    
  
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

    
  
Flags: in-testsuite?

    
    

    
  
Do we need a different patch for 1.8?
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Flags: blocking1.8.1.15+

    
    

    
  


  
This is the same patch for 1.8 (for this bug and for bug 428672 as well), though it's untested (I'm unable to build 1.8 due to cairo version dependency issues it seems). If someone is able to test this I'd appreciate it.
Whiteboard: [sg:high][has patch][has review][has approval] → [sg:high][branch patch needs testing]

    
    

    
  
Comment on attachment 320617 [details] [diff] [review]
1.8 branch version (tested, works).

Flagging dveditz for review of this so he can test. bsterne might also be able to.

        Attachment #320617 -
        Flags: review?(dveditz)

    
    

    
  
Comment on attachment 320617 [details] [diff] [review]
1.8 branch version (tested, works).

I just tested this fix, and it fixes the problem reported in this bug.

        Attachment #320617 -
        Attachment description: 1.8 branch version (untested). → 1.8 branch version (tested, works).
Whiteboard: [sg:high][branch patch needs testing] → [sg:high]
Whiteboard: [sg:high] → [sg:high][needs r dveditz then approval]

    
    

    
  
Comment on attachment 320617 [details] [diff] [review]
1.8 branch version (tested, works).

r=dveditz

Approved for 1.8.1.15, a=dveditz for release-drivers

That's two pretty similar bugs -- do we need to audit all the calls to GetScriptGlobalObject() and GetInnerWindow() and make sure they don't have this problem?

        Attachment #320617 -
        Flags: review?(dveditz)

        Attachment #320617 -
        Flags: review+

        Attachment #320617 -
        Flags: approval1.8.1.15+

    
    

    
  
Fix checked in. Dan, yes, we're spot fixing here. There's a bug on file already to make GetInnerWindow() (on trunk) always really only return an inner window, but that's not something I'd be willing to land on the branch until it's gotten a fair bit of trunk testing.
Keywords: fixed1.8.1.15

    
    

    
  
Verified for branch 2.0.0.15 release with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061005 BonEcho/2.0.0.15pre for testcase 1.
Keywords: fixed1.8.1.15 → verified1.8.1.15
Whiteboard: [sg:high][needs r dveditz then approval] → [sg:high]
Group: security

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: