Opened 8 years ago

Closed 8 years ago

Last modified 16 months ago

#1968 closed defect (fixed)

window.name is persistent across torbutton toggle

Reported by: katmagic Owned by: mikeperry
Priority: Immediate Milestone:
Component: Applications/Torbutton Version: Torbutton: 1.2.5
Severity: Normal Keywords: TorbuttonIteration20110305 MikePerryIteration20110305
Cc: g.koppen@…, the.magical.kat@… Actual Points: 6
Parent ID: Points: 8
Reviewer: Sponsor:

Description

If window.name is assigned by a website, it's value can be read by any JavaScript running in the same tab *at any point in the future*, regardless of what website the JavaScript is from.

Child Tickets

Attachments (1)

torbutton-1.3.2pre5-alpha.xpi (470.5 KB) - added by mikeperry 8 years ago.
Clear window.name on toggle

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by katmagic

It should be noted that window.name persists even when TorButton is toggled.

comment:2 Changed 8 years ago by mikeperry

Priority: majorcritical

comment:3 Changed 8 years ago by mikeperry

Do we have a simple repro page so I can test mitigations against this?

comment:4 Changed 8 years ago by mikeperry

Points: 8

Reproduce: 1
Test fixes: 5
Cleanup: 2

comment:5 Changed 8 years ago by mikeperry

Test this in conjuction with bug #2465.

comment:6 Changed 8 years ago by katmagic

comment:7 Changed 8 years ago by mikeperry

Keywords: TorbuttonIteration20110305 added
Priority: criticalblocker

comment:8 Changed 8 years ago by mikeperry

Keywords: MikePerryIteration20110305 added

comment:9 Changed 8 years ago by mikeperry

Just as an FYI, I did a quick hack to fix this by adding a 'window.name = null;' in jshooks.js.

The problem is that these hooks are not injected on Non-Tor pages, so window.name will still survive from Tor -> Non-Tor with this fix. Blech.

This means we have to reach in and directly manipulate the page DOM from extension-land. Fun, dangerous, and exciting.

comment:10 Changed 8 years ago by gk

Why not using the already implemented request observer? You could get the associated window (if there is any) with callbacks and check whether window.name is set and if so AND the user is requesting a new domain just reset it.

comment:11 Changed 8 years ago by gk

Cc: g.koppen@… added

comment:12 Changed 8 years ago by mikeperry

gk: Two reasons:

  1. My understanding though is that the wrappedJSObject that window lives in has changed behaviors in FF3.5 -> FF4.0. So it requires some extra testing for each one.
  1. We only want to reset this property if there has been a torbutton state change, I believe. Which means we need to find the relevant content window during the toggle phase, rather than the observer.

We want to do this on Toggle because that is the codepath that is used for resetting browser state. I think we want to allow window.name to live for a single Tor session (or until the timer from #523 goes off).

comment:13 in reply to:  12 ; Changed 8 years ago by katmagic

Cc: the.magical.kat@… added

Replying to mikeperry:

gk: Two reasons:

  1. My understanding though is that the wrappedJSObject that window lives in has changed behaviors in FF3.5 -> FF4.0. So it requires some extra testing for each one.
  1. We only want to reset this property if there has been a torbutton state change, I believe. Which means we need to find the relevant content window during the toggle phase, rather than the observer.

We want to do this on Toggle because that is the codepath that is used for resetting browser state. I think we want to allow window.name to live for a single Tor session (or until the timer from #523 goes off).

I'd definitely say that it should be reset every time a new domain is requested. I can't see any reason why someone would want it to persist between sites. It seems much cleaner to clear it.

Changed 8 years ago by mikeperry

Clear window.name on toggle

comment:14 Changed 8 years ago by mikeperry

Status: newneeds_review

Ok, that attachment should clear the window.name attribute on toggle. It ended up being easy to toss in to where we also silence js events on the tab.

The .xpi is based off of fef8cb176662120197bf10d955c1175c1dad82a2 of origin/master.

Let me know if it has any issues.

comment:15 in reply to:  13 Changed 8 years ago by mikeperry

Replying to katmagic:

Replying to mikeperry:

We want to do this on Toggle because that is the codepath that is used for resetting browser state. I think we want to allow window.name to live for a single Tor session (or until the timer from #523 goes off).

I'd definitely say that it should be reset every time a new domain is requested. I can't see any reason why someone would want it to persist between sites. It seems much cleaner to clear it.

I don't think we want to mess with how this property works by default. I can envision websites breaking in the odd corners of the web if we try to disable window.name entirely.. I could also see using the same origin policy definitions to reset window.name to make this break less.. This would be a good option for NoScript, or a hidden Torbutton option, but I still don't think it should be a default.

I can see clearing window.name and any other state data periodically by default, a-la #523, if we can figure out a way to do that without causing users to perceive breakage.

comment:16 Changed 8 years ago by mikeperry

Summary: window.name is persistent across websiteswindow.name is persistent across torbutton toggle

If we want to create an option to apply the same-origin policy to window.name, that should be another ticket. I would like to use this ticket to only apply to window.name being persistent across tor toggle, which is a more serious problem, and a violation of Torbutton's security requirements:
https://www.torproject.org/torbutton/en/design/#requirements

comment:17 Changed 8 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

I created #2669 to note the same origin policy plan. However, I don't think it is one we want to do, and that we want something explicit handling this instead, like #523.

comment:18 Changed 8 years ago by mikeperry

Actual Points: 6

comment:19 Changed 16 months ago by teor

Severity: Normal

Set all tickets without a severity to "Normal"

Note: See TracTickets for help on using tickets.