Opened 4 years ago

Closed 4 years ago

#19478 closed defect (fixed)

File API leaks ms-resolution time

Reported by: arthuredelstein Owned by: arthuredelstein
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-fingerprinting, TorBrowserTeam201606R
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The following calls leak time to content scripts:

new File([], "").lastModifiedDate
new File([], "").lastModified

Child Tickets

Change History (10)

comment:1 Changed 4 years ago by arthuredelstein

Owner: changed from tbb-team to arthuredelstein
Status: newaccepted

comment:2 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201506R added; TorBrowserTeam201506 removed
Status: acceptedneeds_review

Here are two patches for review:

https://github.com/arthuredelstein/tor-browser/commits/19478

979baf5c75605ad0dc15b8638baa02aaac835214 (fix)
b72f23eb82153d95ecf553d65aea0b98b604d7fd (regression test)

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:3 Changed 4 years ago by gk

Keywords: TorBrowserTeam201606R added; TorBrowserTeam201506R removed

We already have 2016. :)

comment:4 Changed 4 years ago by gk

Hm. new Date.getTime() gives me something like 1467036079100 and Math.floor(new File([], "").lastModified / 100000) * 100000) 1467036100000 or 1467036400000 or something similar. It seems your code is not rounding to 100ms?

comment:5 Changed 4 years ago by gk

Cc: mcs brade added

comment:6 in reply to:  4 ; Changed 4 years ago by mcs

Replying to gk:

Hm. new Date.getTime() gives me something like 1467036079100 and Math.floor(new File([], "").lastModified / 100000) * 100000) 1467036100000 or 1467036400000 or something similar. It seems your code is not rounding to 100ms?

The units for mLastModificationDate are microseconds, so part of the patch will need to be changed.

I am not sure how to fix the tests to detect when values are rounded too much. Maybe by using a real file with a known modified date?

comment:7 in reply to:  6 ; Changed 4 years ago by arthuredelstein

Replying to mcs:

Replying to gk:

Hm. new Date.getTime() gives me something like 1467036079100 and Math.floor(new File([], "").lastModified / 100000) * 100000) 1467036100000 or 1467036400000 or something similar. It seems your code is not rounding to 100ms?

The units for mLastModificationDate are microseconds, so part of the patch will need to be changed.

Thanks everyone for having a look at this patch. I used 100000 exactly because that mLastModificationDate variable is in microseconds, so my feeling is the patch is correct. What part do you have in mind that needs to be changed?

Here are relevant lines from the mochitest on OS X. I think it is correctly rounding to the most recent 100 ms.

7 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new Date().getTime()' should be rounded to nearest 100 ms; saw 1467057044400 
[snip]
13 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new File([], "").lastModified' should be rounded to nearest 100 ms; saw 1467057044500 
14 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new File([], "").lastModifiedDate.getTime()' should be rounded to nearest 100 ms; saw 1467057044500

I am not sure how to fix the tests to detect when values are rounded too much. Maybe by using a real file with a known modified date?

If we bind this behavior to a pref (perhaps in the upstreamed version) and then measure the same value with pref on and off, it should be possible to test that the two values are within 100 ms. But since we don't have that pref right now, I would be inclined to just do manual tests for now.

comment:8 in reply to:  7 Changed 4 years ago by mcs

Replying to arthuredelstein:

Thanks everyone for having a look at this patch. I used 100000 exactly because that mLastModificationDate variable is in microseconds, so my feeling is the patch is correct. What part do you have in mind that needs to be changed?

Hmm. On second thought, I think your patch is correct. I will try to compile and run it later, unless GeKo has second thoughts about the problem he saw.

comment:9 in reply to:  7 Changed 4 years ago by gk

Replying to arthuredelstein:

Replying to mcs:

Replying to gk:

Hm. new Date.getTime() gives me something like 1467036079100 and Math.floor(new File([], "").lastModified / 100000) * 100000) 1467036100000 or 1467036400000 or something similar. It seems your code is not rounding to 100ms?

The units for mLastModificationDate are microseconds, so part of the patch will need to be changed.

Thanks everyone for having a look at this patch. I used 100000 exactly because that mLastModificationDate variable is in microseconds, so my feeling is the patch is correct. What part do you have in mind that needs to be changed?

Here are relevant lines from the mochitest on OS X. I think it is correctly rounding to the most recent 100 ms.

7 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new Date().getTime()' should be rounded to nearest 100 ms; saw 1467057044400 
[snip]
13 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new File([], "").lastModified' should be rounded to nearest 100 ms; saw 1467057044500 
14 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new File([], "").lastModifiedDate.getTime()' should be rounded to nearest 100 ms; saw 1467057044500

Interesting. I assumed testing in the scratchpad of an unpatched Tor Browser would be enough in a first pass. But it seems I was wrong for some reason. I've built the patch and verified that it is working. Thus, looks fine to me.

I am not sure how to fix the tests to detect when values are rounded too much. Maybe by using a real file with a known modified date?

If we bind this behavior to a pref (perhaps in the upstreamed version) and then measure the same value with pref on and off, it should be possible to test that the two values are within 100 ms. But since we don't have that pref right now, I would be inclined to just do manual tests for now.

Sounds good to me.

comment:10 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Seems we are done here as well. tor-browser-45.2.0esr-6.5-1 has the changes (commits f2291c41ff45ff3108ef05539fadf7fafac2e7cd and 0f601022eee81c9a0d2fd94a59e4719163d155e4).

Note: See TracTickets for help on using tickets.