Closed Bug 463205 Opened 16 years ago Closed 16 years ago

It's possible to make SessionStore inject text data into the wrong document

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3.1b2

People

(Reporter: moz_bug_r_a4, Assigned: zeniko)

References

Details

(Keywords: verified1.8.1.19, verified1.9.0.5, Whiteboard: [sg:high][fixed by bug 464620])

Attachments

(2 obsolete files) 

SessionStore does not check whether a loaded document is an intended document
when restoring text data.  (On trunk, SessionStore checks a top-level
document's url, but does not check subframes.  On fx3/fx2, SessionStore does
not check at all.)  Thus, it's possible to make SessionStore inject text data
into the wrong document by loading a new document during restoration.
Attached file testcase 1 (obsolete) — 
This tries to inject text data into http://htmledit.squarefree.com/
testcase 1 does not work in https: on fx3/fx2.
Attached file testcase 1 (+https) — 
This tries to inject text data into http://htmledit.squarefree.com/
Flags: blocking1.9.0.5?
Flags: blocking1.8.1.19?
Attached patch like so? (obsolete) — — Splinter Review 

        Attachment #346550 -
        Flags: review?(dietrich)

    
    

    
  
Could be sg:moderate because of the session-restore requirement, but it's not hard to imagine a page using a known crasher to effectively force a tab reload so sg:high for now.
Assignee: nobody → zeniko
Whiteboard: [sg:high]
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.5?
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19?
Flags: blocking1.8.1.19+
Whiteboard: [sg:high] → [sg:high][needs r dietrich]

    
    

    
  
Comment on attachment 346550 [details] [diff] [review]
like so?

r=me, looks to resolve this case. minor issue: is there a case where the loaded URI might differ from the string-serialized URI in a valid way? if so, maybe nsIURI.equals() would be better?

        Attachment #346550 -
        Flags: review?(dietrich) → review+
Whiteboard: [sg:high][needs r dietrich] → [sg:high]

    
    

    
  
Comment on attachment 346550 [details] [diff] [review]
like so?

Requesting approval for Beta 2 due to [sg:high].

(In reply to comment #7)
> is there a case where the loaded URI might differ from the string-
> serialized URI in a valid way?

Not AFAICT, as it is ourselves who restored that frame in the first place.

        Attachment #346550 -
        Flags: approval1.9.1b2?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        branch patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #347747 -
        Flags: approval1.9.0.5?

    
    

    
  
Simon, is 1.8.1 branch affected? If so, can you make a patch for that as well?
Whiteboard: [sg:high] → [sg:high][has review][needs approval]
Target Milestone: --- → Firefox 3.1b2

    
    

    
  
Comment on attachment 347747 [details] [diff] [review]
branch patch

This patch applies to the 1.8.1 branch as well.

        Attachment #347747 -
        Flags: approval1.8.1.18?

    
  

        Attachment #347747 -
        Flags: approval1.8.1.18? → approval1.8.1.19?

        Attachment #347747 -
        Flags: approval1.9.0.5?

        Attachment #347747 -
        Flags: approval1.9.0.5+

        Attachment #347747 -
        Flags: approval1.8.1.19?

        Attachment #347747 -
        Flags: approval1.8.1.19+

    
    

    
  
Comment on attachment 347747 [details] [diff] [review]
branch patch

approved for 1.8.1.19 and 1.9.0.5, a=dveditz for release-drivers

        Attachment #346550 -
        Flags: approval1.9.1b2? → approval1.9.1b2+

    
    

    
  
There's a more comprehensive fix in bug 464620. We'll still want to land the tests from attachment #346550 [details] [diff] [review], though.
Whiteboard: [sg:high][has review][needs approval] → [sg:high][has review][to be fixed in bug 464620]
Whiteboard: [sg:high][has review][to be fixed in bug 464620] → [sg:high][has review][has approval]

    
    

    
  
Comment on attachment 346550 [details] [diff] [review]
like so?

We're closing up for beta2 and this hasn't landed yet, so we'll hold off until after we branch.

Also: was this fixed on trunk by bug 464620 as indicated in the previous comment?

        Attachment #346550 -
        Flags: approval1.9.1b2+ → approval1.9.1b2-

    
    

    
  
This issue was indeed FIXED by bug 464620 on Trunk and both the 1.8.1 and 1.9 branches. The tests will land in bug 464620 as well.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [sg:high][has review][has approval] → [sg:high][fixed by bug 464620]

    
  

        Attachment #346550 -
        Attachment is obsolete: true

    
  

        Attachment #347747 -
        Attachment is obsolete: true
Depends on: 464620

    
    

    
  
Verified for 1.8.1.19 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19pre) Gecko/2008112503 BonEcho/2.0.0.19pre. 

Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008112505 GranParadiso/3.0.5pre.
Keywords: fixed1.8.1.19, 
          
            fixed1.9.0.5 → verified1.8.1.19, 
          
            verified1.9.0.5

    
  
Flags: wanted1.8.0.x-
Group: core-security

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: