Opened 3 years ago

Closed 3 years ago

#19186 closed defect (fixed)

KeyboardEvents are only rounding to 100ms

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

Description

In #1517, the idea was to round most event time stamps to 100ms, but to round KeyboardEvent.timeStamp to 250 ms (see https://trac.torproject.org/projects/tor/ticket/1517#comment:25).

Our current patch for #1517 is here.

However, when I test KeyboardEvents in the latest Tor Browser, I only see rounding to 100 ms. With the debugger I observed that calling myKeyboardEvent.timeStamp in the browser console is calling Event::TimeStamp() instead of KeyboardEvent::TimeStamp. So we're apparently failing to properly override the Event::TimeStamp call.

Child Tickets

Change History (12)

comment:1 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201505R added; TorBrowserTeam201505 removed
Status: newneeds_review

If we want to make sure that KeyboardEvent::timeStamp rounding to 250 ms is exposed to content JS, we can make Event::timeStamp virtual (and fix the return type of KeyboardEvent::timeStamp). Here's an example fixup, probably not for use.

But I'm not sure this is really useful, as it's still very easy to get 100-ms precision on keystroke times. Just ignore KeyboardEvent.timeStamp and use Date.now() on the KeyboardEvent handler:

document.addEventListener("keydown", e => console.log(e.key + ": " + Date.now()));

So I'm proposing the following patch instead, which simply removes our original change to KeyboardEvent.h. Then we would rely on the 100-ms rounding inside Event.cpp, which is already applied to KeyboardEvents. (If it looks like 100-ms is unsufficient jitter for KeyboardEvents, then we may want to increase the rounding for all JS timing sources.)

https://github.com/arthuredelstein/tor-browser/commit/19186
Hash cb8eace4cd33cc9905bb7edbb8d9c066cc4217df

comment:2 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201505R removed

Dragging this into 2016 :)

comment:3 Changed 3 years ago by gk

Is that a regression due to changes in ESR 45 or did we already have this problem with ESR38-bases Tor Browsers?

comment:4 in reply to:  3 Changed 3 years ago by arthuredelstein

Replying to gk:

Is that a regression due to changes in ESR 45 or did we already have this problem with ESR38-bases Tor Browsers?

Turns out it was already there -- I see this problem in Tor Browser 5.5.

comment:5 Changed 3 years ago by gk

Looks good to me. And if that has always been as your patch proposes then we probably won't have a ton of users coming to us complaining about broken websites/games/whatever.

However, it bothers me a bit that we did not catch that earlier. Could we maybe have a test for that (a new ticket for it is fine)?

comment:6 in reply to:  5 Changed 3 years ago by arthuredelstein

Replying to gk:

Looks good to me. And if that has always been as your patch proposes then we probably won't have a ton of users coming to us complaining about broken websites/games/whatever.

However, it bothers me a bit that we did not catch that earlier. Could we maybe have a test for that (a new ticket for it is fine)?

Agreed. I posted regression tests at ticket:19193#comment:1.

comment:7 Changed 3 years ago by gk

Cc: mcs brade added

Mark/Kathy: Could you have a look at this one, too?

comment:8 Changed 3 years ago by gk

Keywords: TorBrowserTeam201606 added

Moving reviews to June

comment:9 Changed 3 years ago by gk

Keywords: TorBrowserTeam201606R added; TorBrowserTeam201605R removed

comment:10 Changed 3 years ago by gk

Keywords: TorBrowserTeam201606 removed

comment:11 in reply to:  7 Changed 3 years ago by mcs

Replying to gk:

Mark/Kathy: Could you have a look at this one, too?

r=brade, r=mcs
This looks fine to us as well.

comment:12 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Commit 7cfce1d2cd71ed69ae3ffb4e59007eeae5ac5815 on tor-browser-45.2.0esr-6.5-1 has it, thanks.

Note: See TracTickets for help on using tickets.