Keyboard layout is leaked by KeyboardEvent

NEW
Assigned to

Status

()

Core
Event Handling
P1
normal
2 years ago
5 days ago

People

(Reporter: arthuredelstein, Assigned: timhuang)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor 15646][tor 17009][tor-standalone][fingerprinting][fp:m3])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

Different keyboard layouts (such as French, Dvorak, etc.) can be detected by content scripts by examining the properties of KeyboardEvents as the user types. This information can be used with other data to track users.

We introduced a patch in Tor Browser to avoid leaking the user's preferred keyboard layout, by introducing an option to spoof the KeyboardEvent properties to mimic a US English QWERTY keyboard regardless of the actual keyboard layout.

We would like to propose upstreaming this patch to Firefox. The spoofing is active only when the pref "privacy.resistFingerprinting" is enabled.
Created attachment 8684045 [details] [diff] [review]
0001-Bug-1222285-Prevent-keyboard-layout-fingerprinting-v.patch
Attachment #8684045 - Flags: review?(bugs)
Comment on attachment 8684045 [details] [diff] [review]
0001-Bug-1222285-Prevent-keyboard-layout-fingerprinting-v.patch

>+  Preferences::AddBoolVarCache(&sPrivacyResistFingerprinting,
>+                               "privacy.resistFingerprinting", false);
You want to use this for other stuff too?
Since I was thinking this stuff could be useful for web devs too, in which case it has nothing to do with
fingerprinting but about testing different key layouts.

>+// Three global constant static maps.
>+// gCodes provides a codeName for each keyName.
>+static nsDataHashtable<nsStringHashKey, nsString>* gCodes;
>+// gKeyCodes provides a keyCode for each keyName.
>+static nsDataHashtable<nsStringHashKey, uint32_t>* gKeyCodes;
>+// gShiftStates provides a shift value for each keyName.
>+static nsDataHashtable<nsStringHashKey, bool>* gShiftStates;
>+
>+static StaticMutex createKeyCodesMutex;
>+
Hmm, so you declare static variables in an .h file.
At least have -inl or Inlines in the file name indicating it isn't really a normal .h

>+// Populate the global static maps gCodes, gKeyCodes, gShiftStates
>+// with their constant values.
>+static void createKeyCodes()
methods should start with capital letter.

>+{
>+  if (gCodes) return;
Always {} with if

>+
>+  StaticMutexAutoLock lock(createKeyCodesMutex);
What is this mutex for?


> KeyboardEvent::AltKey()
> {
>-  return mEvent->AsKeyboardEvent()->IsAlt();
>+  bool altState = mEvent->AsKeyboardEvent()->IsAlt();
>+  if (ResistFingerprinting()) {
>+    nsString keyName;
nsAutoString



>@@ -68,7 +87,16 @@ KeyboardEvent::GetCtrlKey(bool* aIsDown)
> bool
> KeyboardEvent::ShiftKey()
> {
>-  return mEvent->AsKeyboardEvent()->IsShift();
>+  bool shiftState = mEvent->AsKeyboardEvent()->IsShift();
>+  if (!ResistFingerprinting()) {
>+    return shiftState;
>+  }
>+  // Find a consensus fake shift state for the given key name.
>+  bool fakeShiftState;
>+  nsString keyName;
nsAutoString for stack strings.





> KeyboardEvent::KeyCode()
> {
>   // If this event is initialized with ctor, we shouldn't check event type.
>-  if (mInitializedByCtor) {
>-    return mEvent->AsKeyboardEvent()->keyCode;
>-  }
>-
>-  if (mEvent->HasKeyEventMessage()) {
>-    return mEvent->AsKeyboardEvent()->keyCode;
>+  if (mInitializedByCtor || mEvent->HasKeyEventMessage()) {
>+    if (!ResistFingerprinting()) {
>+      return mEvent->AsKeyboardEvent()->keyCode;
>+    } else {
>+      if (CharCode() != 0) {
>+        return 0;
>+      }
>+      // Find a consensus key code for the given key name.
>+      nsString keyName;
>+      GetKey(keyName);
>+      uint32_t keyCode;
>+      return gKeyCodes->Get(keyName, &keyCode) ? keyCode : 0;
>+    }
This looks wrong. If the event was initialized by ctor, you shouldn't change the keyCode.
The data isn't coming from UA or OS, but from a script.


>+  switch (location) {
>+    case 0 : return 0;
>+    case 1 : return 1;
>+    case 2 : return 1;
>+    case 3 : return 0;
>+    default: return 0;
>+  }
No magical numbers. Please use constants.
nsIDOMKeyEvent::DOM_xxx

>--- a/dom/events/KeyboardEvent.h
>+++ b/dom/events/KeyboardEvent.h
>@@ -49,6 +49,15 @@ public:
> 
>   bool GetModifierState(const nsAString& aKey)
>   {

>+      if (aKey.Equals(NS_LITERAL_STRING("Shift"))) {
>+        return ShiftKey();
>+      }
It is not clear to me why this is needed.


r-, but I'm also still thinking if we actually want more generic solution here where one could easily provide these kinds of key mappings, use case would be devtools.
Attachment #8684045 - Flags: review?(bugs) → review-
Looks like the patch hides the attribute values of untrusted events too. So, web apps must be able to distinguish if the user hides its actual keyboard event.

Anyways, I don't like this feature because this also useful only for ANSI keyboard layout users. If the user's standard keyboard layout isn't same as US keyboard layout like JIS keyboard layout in Japan, web apps which handles KeyboardEvent won't work fine. I guess that some users don't understand the fact and must believe that it's a bug of Firefox.

So, I think that it must be worthwhile only for we create the default keyboard layout mapping from the locale's standard keyboard layout. I guess that it should depend on the language of the web page or language of the first accept language.

How do you think about the i18n issue, smaug?

So, I strongly disagree with current patch's approach.
Flags: needinfo?(bugs)
According to the purpose, I believe that we should create a table which maps accept-language to preferred keyboard layout. And KeyCodeConsensus should create dummy keyboard layout as the preferred accept-language's keyboard layout. E.g., in Japan, users who use ANSI keyboard layout are minority. So, it doesn't make sense to emulate ANSI keyboard layout in Japan (i.e., easier to track the specific user with this feature in Japan).
I'd like to know how the tracking sites listen key events in different website? If it were possible, we should lie only to such listeners.
masayuki's concerns are very valid, so Arthur could you perhaps explain some more background to this.
Does Tor always change accept-language to en-US perhaps?
Flags: needinfo?(bugs)
> Does Tor always change accept-language to en-US perhaps?

Even if so, such change is a big hint for trackers unless their target users are in en-US region...
(In reply to Olli Pettay [:smaug] from comment #6)
> masayuki's concerns are very valid, so Arthur could you perhaps explain some
> more background to this.
> Does Tor always change accept-language to en-US perhaps?

Yes, Tor Browser sets "intl.accept_languages" to "en-US, en". That's intended to make all Tor Browser users look the same. So similarly, we are trying to give the appearance that every user has the same keyboard layout.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> > Does Tor always change accept-language to en-US perhaps?
> 
> Even if so, such change is a big hint for trackers unless their target users
> are in en-US region...

The approach in Tor Browser is not to blend in with all web users (which is impossible), but rather to make all Tor Browser users indistinguishable from one another. So, for example, we are trying to make it difficult to determine if a user prefers English or Japanese. We are not trying to hide that the user has this pref enabled.

This proposal is part of a larger effort to provide settings for fingerprinting resistance. See for example https://dxr.mozilla.org/mozilla-central/search?q=resistFingerprinting&redirect=false&case=false
Whiteboard: [tor]
Priority: -- → P3
Summary: Keyboard layout is leaked by KeyboardEvent → Keyboard layout is leaked by KeyboardEvent (Tor 15646)
Blocks: 1260929
Summary: Keyboard layout is leaked by KeyboardEvent (Tor 15646) → Keyboard layout is leaked by KeyboardEvent (Tor 15646, 17009)
Whiteboard: [tor] → [tor][tor-standalone]
Whiteboard: [tor][tor-standalone] → [tor][tor-standalone][fingerprinting]
Blocks: 1329996

Updated

9 months ago
Priority: P3 → P2
Summary: Keyboard layout is leaked by KeyboardEvent (Tor 15646, 17009) → Keyboard layout is leaked by KeyboardEvent
Whiteboard: [tor][tor-standalone][fingerprinting] → [tor 15646][tor 17009][tor-standalone][fingerprinting]
Priority: P2 → P1
Whiteboard: [tor 15646][tor 17009][tor-standalone][fingerprinting] → [tor 15646][tor 17009][tor-standalone][fingerprinting][fp:m1]
Whiteboard: [tor 15646][tor 17009][tor-standalone][fingerprinting][fp:m1] → [tor 15646][tor 17009][tor-standalone][fingerprinting][fp:m3]
(Assignee)

Updated

2 months ago
Assignee: nobody → tihuang
(Assignee)

Comment 10

2 months ago
I've noticed that Tor browser will still fire the keyboard event even though the key is not in the US English QWERTY keyboard. For example, the key 'ü' for German keyboard layout. For those keys, it will strip keyboardEvent.code and make the keyboardEvent.keycode to 0. However, the content can still inspect the keyboardEvent.key and keyboardEvent.charCode to figure out which key has been pressed. This means a website can still tell which keyboard layout that someone is using if those keys that are not in the US English QWERTY keyboard have been pressed.

So, I am wondering that should we block these non-US English QWERTY keyboard events?

What do you think about this, Arthur?
Flags: needinfo?(arthuredelstein)
Hi Tim -- my thinking is we don't want to completely block characters like 'ü'. Ideally we would always give the same KeyboardEvent regardless of the physical keyboard. That means we could add additional hardcoded .keyCode  and .code values for those other characters. Because they're not found on a US Keyboard, we could use values for a given character commonly found on other keyboards. For example, 'ü' would have the same .keyCode and .code as '[' if we use its position on the standard German keyboard layout.

By the way, I should mention we made some revisions in Tor Browser since I posted the attached patch in comment 1. The current keyboard patches (with regression tests) are at https://torpat.ch/15646 and https://torpat.ch/17009
Flags: needinfo?(arthuredelstein)

Comment 12

a month ago
FYI: Here is a useful test page for key events: https://w3c.github.io/uievents/tools/key-event-viewer.html
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8684045 - Attachment is obsolete: true
Comment on attachment 8909812 [details]
Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a US English QWERTY keyboard when 'privacy.resistFingerprinting' is true.

Masayuki, could you take a look at these? You might have stronger opinions than I do about this stuff.
Attachment #8909812 - Flags: review?(bugs) → review?(masayuki)
Attachment #8909813 - Flags: review?(bugs) → review?(masayuki)
Attachment #8909814 - Flags: review?(bugs) → review?(masayuki)
Comment on attachment 8909812 [details]
Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a US English QWERTY keyboard when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/181314/#review186926

And I also suspect if this can actually protects users. If web apps make users type something in text editor like <input type="text">, <textarea> or <div contenteditable>, TextEditor will handle the key value or charCode value as is, but the keyboard event lies to event listeners. So, web apps can detect if the user agent is a liar.

Additionally, perhaps, this patch makes KeyboardEvent lie to event listeners of addons too. It must be unexpected behavior for you too. Although, I'm not sure how to distinguish if the event attributes are accessed from addons or not.

::: dom/events/KeyboardEvent.h:52
(Diff revision 1)
>    bool ShiftKey();
>    bool MetaKey();
>  
>    bool GetModifierState(const nsAString& aKey)
>    {
> +    if (ShouldResistFingerprinting()) {

Please use this style:


{
  if (!ShouldResistFingerprinting()) {
    return GetModifierStateInternal(aKey);
  }
  ...
}

And you need to add automated tests with synthesizeKey() of EventUtils.js.

And if this is created by script, you shouldn't apply any hack since it'll be available as fingerprint.

::: dom/events/KeyboardEvent.h:53
(Diff revision 1)
> +      if (aKey.Equals(NS_LITERAL_STRING("Alt")) ||
> +          aKey.Equals(NS_LITERAL_STRING("AltGraph"))) {
> +        return AltKey();
> +      }
> +      if (aKey.Equals(NS_LITERAL_STRING("Shift"))) {
> +        return ShiftKey();
> +      }

How about other modifier keys especially for "Control"? Some Asian keyboard layouts inserts characters with Ctrl (+ Shift) + foo.

And please explain the reason why we need to do this with comment.

::: dom/events/KeyboardEvent.h:108
(Diff revision 1)
>    // If the instance is created with Constructor(), which may have independent
>    // value.  mInitializedWhichValue stores it.  I.e., this is invalid when
>    // mInitializedByCtor is false.
>    uint32_t mInitializedWhichValue;
> +
> +  bool ShouldResistFingerprinting();

Please explain what should do and should not do when this return true with comment.

::: dom/events/KeyboardEvent.cpp:44
(Diff revision 1)
> -  return mEvent->AsKeyboardEvent()->IsAlt();
> +  bool altState = mEvent->AsKeyboardEvent()->IsAlt();
> +
> +  if (ShouldResistFingerprinting()) {

Please use early return style as I mentioned above.  And if this is created by script, you shouldn't apply any hack since it'll be available as fingerprint.

::: dom/events/KeyboardEvent.cpp:52
(Diff revision 1)
> +    // This is because the Alt key will be always false for regular US English
> +    // QWERTY keyboard when shift key is present. But for other keyboard layout,

I still don't believe that we always return US QWERTY layout information to protect users. If the document is not for US users, this may cause stronger hint than user's keyboard layout information.

I think that at first step of this kind of hack, only supports US keyboard layout is okay. However, you should make strategy in the near future clearer. E.g., nsRFPService::GetSpoofedShiftKeyState() will take the target as an argument and only changing nsRFPService::GetSoofedShiftKeyState() can improve the supports.

::: dom/events/KeyboardEvent.cpp:57
(Diff revision 1)
> +    bool exists = nsRFPService::GetSpoofedShiftKeyState(keyName, &unused);
> +
> +    altState = exists ? false : altState;

I don't understand why you need temporary variable for the result of nsRFPService::GetSpoofedShiftKeyState(). Why don't you write this as:

if (nsRFPService::GetSpoofedShiftKeyState(...)) {
  return false;
}

?

And using GetKey() may make damage to the performance. Why don't you refer mEvent->AsKeyboardEvent()->mKeyNameIndex if it's available (i.e., not KEY_NAME_INDEX_USE_STRING)?

::: dom/events/KeyboardEvent.cpp:90
(Diff revision 1)
> -  return mEvent->AsKeyboardEvent()->IsShift();
> +  bool shiftState = mEvent->AsKeyboardEvent()->IsShift();
> +
> +  if (ShouldResistFingerprinting()) {
> +    bool fakeShiftState = false;
> +    nsAutoString keyName;
> +
> +    GetKey(keyName);
> +    bool exists = nsRFPService::GetSpoofedShiftKeyState(keyName, &fakeShiftState);
> +
> +    shiftState = exists ? fakeShiftState : shiftState;
> +  }

Same as AltKey().

::: dom/events/KeyboardEvent.cpp:92
(Diff revision 1)
>  bool
>  KeyboardEvent::ShiftKey()
>  {
> -  return mEvent->AsKeyboardEvent()->IsShift();
> +  bool shiftState = mEvent->AsKeyboardEvent()->IsShift();
> +
> +  if (ShouldResistFingerprinting()) {

Use early return style here. And if this is created by script, you shouldn't apply any hack since it'll be available as fingerprint.

::: dom/events/KeyboardEvent.cpp:158
(Diff revision 1)
>  KeyboardEvent::GetKey(nsAString& aKeyName)
>  {
>    mEvent->AsKeyboardEvent()->GetDOMKeyName(aKeyName);

Why don't you touch KeyboardEvent.key? I really don't understand this reason...

::: dom/events/KeyboardEvent.cpp:167
(Diff revision 1)
>  }
>  
>  void
>  KeyboardEvent::GetCode(nsAString& aCodeName)
>  {
> +  if (ShouldResistFingerprinting()) {

Use early return style. And if this is created by script, you shouldn't apply any hack since it'll be available as fingerprint.

::: dom/events/KeyboardEvent.cpp:172
(Diff revision 1)
> +  if (ShouldResistFingerprinting()) {
> +    // Use a consensus code name corresponding to the
> +    // key name.
> +    nsAutoString keyName;
> +    nsAutoString codeName;
> +    GetKey(keyName);

Same. You should use mKeyNameIndex as far as possible. Local string copy should be avoided as far as possible.

::: dom/events/KeyboardEvent.cpp:174
(Diff revision 1)
> +    bool exists = nsRFPService::GetSpoofedCode(keyName, codeName);
> +
> +    if (exists) {
> +      aCodeName.Assign(codeName);
> +    }

So, this means that when user types keys which do no exist in US keyboard layout, e.g., IntlRo, IntlBackslash, contents can listen to odd keyboard events whose .keyCode is 0, .charCode is 0, .code is "". So, I think that when KeyboardLayout is constructed, the event's mOnlySystemGroupDispatchInContent should be set to true and can skip these hacks.
https://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/widget/BasicEvents.h#115-120

::: dom/events/KeyboardEvent.cpp:268
(Diff revision 1)
> +    if (ShouldResistFingerprinting()) {
> +      if (CharCode() != 0) {
> +        return 0;
> +      }
> +
> +      nsAutoString keyName;
> +      GetKey(keyName);
> +
> +      // Find a consensus key code for the given key name.
> +      bool exists = nsRFPService::GetSpoofedKeyCode(keyName, &keyCode);
> +      if (!exists) {
> +        keyCode = 0;
> +      }
> +    }
> +    return keyCode;

Same as above methods.

::: dom/events/KeyboardEvent.cpp:332
(Diff revision 1)
>  uint32_t
>  KeyboardEvent::Location()
>  {
> -  return mEvent->AsKeyboardEvent()->mLocation;
> +  uint32_t location = mEvent->AsKeyboardEvent()->mLocation;
> +
> +  if (ShouldResistFingerprinting()) {

Use early return style and do nothing if this event is created by script.

::: dom/events/KeyboardEvent.cpp:333
(Diff revision 1)
> +    // To resist fingerprinting, hide right modifier keys, as
> +    // well as the numpad.

I don't understand the reason why we need to hide the location. Right modifiers and NumPad keys may not be available in some keyboard. However, some users of not such keyboard may use only Left modifiers and Standard position keys. So, hiding the location causes only breaking compatibility with some web sites like games.

::: dom/events/KeyboardEvent.cpp:452
(Diff revision 1)
> +KeyboardEvent::ShouldResistFingerprinting()
> +{
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(mOwner);
> +
> +  if (!doc) {
> +    nsCOMPtr<nsINode> node = do_QueryInterface(mOwner);
> +
> +    if (node) {
> +      doc = node->OwnerDoc();
> +    } else {
> +      nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(mOwner);
> +      if (window) {
> +        doc = window->GetExtantDoc();
> +      }
> +    }
> +  }
> +
> +  return !!doc && nsContentUtils::ShouldResistFingerprinting(doc);

Looks like that this may be called even if it's in normal mode. However, this running cost is not cheap because using a lot of QI. That means that accessing some attributes of KeyboardEvent may make damage to the performance of Firefox. So, you should think better way to access the pref...

::: dom/events/KeyboardEvent.cpp:452
(Diff revision 1)
>    keyEvent->mKeyNameIndex = KEY_NAME_INDEX_USE_STRING;
>    keyEvent->mKeyValue = aKey;
>  }
>  
> +bool
> +KeyboardEvent::ShouldResistFingerprinting()

So, I guess that this should check if this event is created by script.

::: toolkit/components/resistfingerprinting/nsRFPService.h:26
(Diff revision 1)
>  
>  #define LEGACY_BUILD_ID    "20100101"
>  
>  namespace mozilla {
>  
> +struct FakeKeyboardCode {

Put the |{| to the next line.

::: toolkit/components/resistfingerprinting/nsRFPService.h:27
(Diff revision 1)
> +  nsString mKey;
> +  nsString mCode;

Please use KEY_NAME_INDEX as far as possible and CODE_NAME_INDEX.

::: toolkit/components/resistfingerprinting/nsRFPService.h:30
(Diff revision 1)
>  
> +struct FakeKeyboardCode {
> +  nsString mKey;
> +  nsString mCode;
> +  uint32_t mKeyCode;
> +  bool     mShiftStates;

I think that using mozilla::Modifiers is better for supporting other keyboard layouts in the future.

::: toolkit/components/resistfingerprinting/nsRFPService.h:63
(Diff revision 1)
> +  // Methods for getting spoofed key codes.
> +  static bool GetSpoofedShiftKeyState(const nsAString& aKeyName, bool* aOut);
> +  static bool GetSpoofedCode(const nsAString& aKeyName, nsAString& aOut);
> +  static bool GetSpoofedKeyCode(const nsAString& aKeyName, uint32_t* aOut);

Please explain what each argument and each result mean with comment.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:263
(Diff revision 1)
> +  // Creating the key code table for spoofing keyboard events.
> +  CreateSpoofingKeyCodes();

Hmm, this is called by nsContentUtils::Init() without any conditions but this will waste a lot of memory for *all* users. I think that it should be created when they are really necessary. And also, this must make damage to the start up performance.
Attachment #8909812 - Flags: review?(masayuki) → review-
Comment on attachment 8909813 [details]
Bug 1222285 - Part 2: Making the keyboard events of modifier keys been suppressed when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/181316/#review186928

::: layout/base/PresShell.cpp:8415
(Diff revision 1)
> +      aEvent->AsKeyboardEvent()->GetDOMKeyName(keyName);
> +
> +      if (keyName.Equals(NS_LITERAL_STRING("Shift")) ||
> +          keyName.Equals(NS_LITERAL_STRING("Alt")) ||
> +          keyName.Equals(NS_LITERAL_STRING("AltGraph"))) {

You should check mKeyNameIndex with switch statement. And perhaps, KeyboardEvent should have a method to check this in WidgetEventImpl.cpp.

::: layout/base/PresShell.cpp:8420
(Diff revision 1)
> +      aEvent->AsKeyboardEvent()->GetDOMKeyName(keyName);
> +
> +      if (keyName.Equals(NS_LITERAL_STRING("Shift")) ||
> +          keyName.Equals(NS_LITERAL_STRING("Alt")) ||
> +          keyName.Equals(NS_LITERAL_STRING("AltGraph"))) {
> +        aEvent->mFlags.mOnlyChromeDispatch = true;

I think that this should be mOnlySystemGroupDispatchInContent since keyboard event listeners in the system group may need to handle modifier key events. And perhaps, you need to do that for other modifier keys too. E.g., pressing "Meta" shouldn't cause pasting text on Windows but not so on macOS.
Attachment #8909813 - Flags: review?(masayuki) → review-
Comment on attachment 8909814 [details]
Bug 1222285 - Part 3: Add a test case to check whether keyEvents been correctly spoofed and modifier keys been correctly suppressed.

https://reviewboard.mozilla.org/r/181318/#review186932

I think that you should write this test with synthesizeKey of EventUtils because:

* synthesizeKey is available on any platforms.
* synthesizeKey can test in PresShell::HandleEvent().
* synthesizeKey does NOT depend on native keyboard event handlers, so, can emulate any keyboard layout's KeyboardEvent.

Additionally, you should test event listeners in the system event group. In them, KeyboardEvent needs to return actual value for Firefox itself.

Anyway, temporarily r-'ing for now since you'll need to rewrite this test for changing some behavior which I pointed above.
Attachment #8909814 - Flags: review?(masayuki) → review-
(Reporter)

Comment 20

a month ago
mozreview-review-reply
Comment on attachment 8909812 [details]
Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a US English QWERTY keyboard when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/181314/#review186926

The goal of "privacy.resistFingerprinting" is not to hide the fact that the user agent is lying. That fact is already trivial for any web app to detect. Instead the goal is to make all members of the category of users with "privacy.resistFingerprinting" enabled to appear as in distinguishable as possible. So, for example, if a <textarea> detects a letter "Y", we propose the KeyboardEvent.code should report "KeyY" (as on a US keyboard) and not "KeyZ" (as on a German keyboard), regardless of whether the user is using a US or German keyboard.

I agree that we should only lie to content scripts, and not addons. I think it should be possible to detect whether the Event is targeted to chrome or content. Mozilla already does this in several places in the codebase for "privacy.resistFingerprinting".

> Why don't you touch KeyboardEvent.key? I really don't understand this reason...

KeyboardEvent.key returns the character value of the pressed key, not its physical location on the keyboard. It should match what appears in a <textarea> when the user presses the key. So we don't need to spoof it. We only want to spoof anything that leaks the physical location of the key, namely KeyboardEvent.code.
(Reporter)

Comment 21

26 days ago
mozreview-review
Comment on attachment 8909812 [details]
Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a US English QWERTY keyboard when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/181314/#review189484

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:335
(Diff revision 1)
>        prefs->RemoveObserver(RESIST_FINGERPRINTING_PREF, this);
>      }
>    }
>  }
>  
> +void

I wonder if it might be better to keep the spoofing code near the rest of the KeyEvent code -- is there any advantage in making it part of nsRFPService?
Attachment #8909812 - Flags: review?(arthuredelstein) → review+
(Reporter)

Comment 22

26 days ago
mozreview-review
Comment on attachment 8909813 [details]
Bug 1222285 - Part 2: Making the keyboard events of modifier keys been suppressed when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/181316/#review189486
Attachment #8909813 - Flags: review?(arthuredelstein) → review+
(Reporter)

Comment 23

26 days ago
mozreview-review
Comment on attachment 8909814 [details]
Bug 1222285 - Part 3: Add a test case to check whether keyEvents been correctly spoofed and modifier keys been correctly suppressed.

https://reviewboard.mozilla.org/r/181318/#review189488

I agree with Masayuki that it would be good to use look into EventUtils.synthesizeKey, although I'm not sure about the advantages. Otherwise I think this looks good.
Attachment #8909814 - Flags: review?(arthuredelstein) → review+
(Assignee)

Comment 24

22 days ago
mozreview-review-reply
Comment on attachment 8909812 [details]
Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a US English QWERTY keyboard when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/181314/#review189484

> I wonder if it might be better to keep the spoofing code near the rest of the KeyEvent code -- is there any advantage in making it part of nsRFPService?

The reason that I put it in nsRFPService is twofold.
1. I think it would be simpler if we put all core spoofing code into one place in case that we want to modify them in the future.
2. The nsRFPService is the best place to make the decision regarding when to generate the spoofing keyboard code table. Right now, I think the best timing for generating the table is when 'privacy.resistFingerprinting' been flipping to true. We can easily achieve this within nsRFPService. However, it would be a bit difficult for keyEvent code unless we add an observer in it. It could complicate the KeyEvent code.

For these reasons, I choose to put code in nsRFPService.
Comment on attachment 8909812 [details]
Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a US English QWERTY keyboard when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/181314/#review189484

> The reason that I put it in nsRFPService is twofold.
> 1. I think it would be simpler if we put all core spoofing code into one place in case that we want to modify them in the future.
> 2. The nsRFPService is the best place to make the decision regarding when to generate the spoofing keyboard code table. Right now, I think the best timing for generating the table is when 'privacy.resistFingerprinting' been flipping to true. We can easily achieve this within nsRFPService. However, it would be a bit difficult for keyEvent code unless we add an observer in it. It could complicate the KeyEvent code.
> 
> For these reasons, I choose to put code in nsRFPService.

I agree with them. Additionally, dom should have only minimal code to implement web standards as far as possible. That must make the code easier to read for new hackers.
(Assignee)

Comment 26

22 days ago
I suggest that we should change the protection strategy here to *hide physical key location on the keyboard* instead of *hide everyone behind a US English QWERTY keyboard*. Hiding everyone behind a US English keyboard is unrealistic since the US English keyboard cannot cover all possible keyboard keys. For example, the 'ü' key doesn't exist in US English keyboard. The website would know a user is using German keyboard if this key is pressed. And I believe there are still many keys only appear on their languages' keyboard. Therefore, unfortunately, it is nearly impossible to hide all users behind one type of keyboard.

Given that, I think hiding the physical key and spoofing it to a given physical key on certain keyboard layout is a viable solution. In other words, we should give a default spoofing code and keyCode for every key. For example, all US English alphabets should be spoofed as US English QWERTY keyboard layout regardless what keyboard layout is using for typing these alphabets. For German specific keys, we should spoof them to one German keyboard and so on.

Actually, it is the methodology of the current patch, but it only covers US English alphabets. And we should extend it to all keys. But it needs a comprehensive understanding of all keyboard layouts. So, I think we should focus on US English alphabets first, and open follow-up bugs for other keys.

What do you think, Masayuki and Arthur?
Flags: needinfo?(masayuki)
Flags: needinfo?(arthuredelstein)
It's difficult to say about KeyboardEvent.keyCode since faking its value may break compatibility with some web apps since most web apps still refer this legacy attribute.

We're now deciding them from ASCII character(s) inputted by the key. When the key causes an ASCII character without Shift/AltGr, the keyCode is decided with the character.  Otherwise, when the key causes an ASCII character with Shift, the keyCode is decided with the shifted character.  Otherwise, when the key causes an ASCII character with primary ASCII capable keyboard layout (on macOS or Linux) or the native key code tells an ASCII character for the key (on Windows), the keyCode is decided with the ASCII character.  Otherwise in the former case, shifted character with the primary ASCII capable keyboard layout is used.  Finally, 0 is used. So, returning 0 if actual .keyCode value doesn't exist in the faking layout (currently, QWERTY for en-US), returning 0 perhaps makes sense.

If I were you, I'd like to do:

1. Create a method to retrieve a keyCode value from a table in nsRFPService which takes WidgetKeyboardEvent and document node or language of the document.
2. If WidgetKeyboardEvent.mKeyNameIndex is KEY_NAME_INDEX_USE_STRING, returns a keyCode value which should come with the mKeyValue when user types it in the faking keyboard layout.  If the character cannot be inputted with the faking keyboard layout, returns 0.
3. If WidgetKeyboardEvent.mKeyNameIndex is not KEY_NAME_INDEX_USE_STRING, returns 0 from the method if the key doesn't exist in the faking keyboard layout.
4. Create another method to retrieve modifier state. If WidgetKeyboardEvent.mKeyNameIndex is KEY_NAME_INDEX_USE_STRING, Shift/Control/AltGraph state should be retrieved from a table of faking keyboard with mKeyValue.  Otherwise, returns actual modifier state.

I guess that similar behavior is also available for .code, but shouldn't touch .charCode and .key values because their value are really important for some web apps and they can be inputted without physical keyboard. In such case, using 0 for .keyCode and empty string for .code is not odd behavior.
Flags: needinfo?(masayuki)
(In reply to Tim Huang[:timhuang] from comment #26)

> What do you think, Masayuki and Arthur?

Hi Tim,

I agree with all you say here. That's the approach we are taking in Tor Browser, to hide the physical key location on the keyboard.
Flags: needinfo?(arthuredelstein)
(Assignee)

Comment 29

7 days ago
Per discussion with Arthur and Masayuki, we all agree with that using different keyboard layouts according to the language of the document is a correct approach. For example, using US English keyboard for documents in English, using one Japanese keyboard layout, which we need to figure out in a latter bug, for documents in Japanese and so on.

But, Here is one thing we haven't reached a conclusion: should we really need to spoof the location for NumPad? On the one hand, the website cannot really tell the existence of Numpad since some users may never use Numpad even if it is there. And the existence of the Numpad doesn't really have anything to do with the current Keyboard layout is. On the other hand, using Numpad can be used to differentiate people into categories, people who are using Numpad and who are not. So, it's more like a user behavior indicator. It's also a fingerprinting vector but it's less severe according to [1].

Personally speaking, I think it makes sense to me that spoofing the location of Numpad for fingerprinting resistance since it fits in the category of user behavior fingerprinting. But, It seems not fitting well in terms of keyboard layout fingerprinting here. So, maybe we have to open another bug for this issue. And given the breakages, it may introduce, and the severity of it, we need to think about how we deal this problem. Perhaps, we can use a site permission here. We only spoof the location of Numpad if there is no permission, and we don't spoof it if users give permission. But, anyway, we need another bug for this.

Arthur and Masayuki, what do you think that we move the Numpad issue to another bug and focus on keyboard layout fingerprinting here?

[1] https://www.torproject.org/projects/torbrowser/design/#fingerprinting-scope
Flags: needinfo?(masayuki)
Flags: needinfo?(arthuredelstein)
Sounds like a good idea to me. Thanks, Tim.
Flags: needinfo?(arthuredelstein)
Yeah, I think that it's different issue and should be treated in another bug because of caused by user's behavior, not keyboard layout, as you said.
Flags: needinfo?(masayuki)
(Assignee)

Updated

5 days ago
See Also: → bug 1409974
You need to log in before you can comment on or make changes to this bug.