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.

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.

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.

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?

> 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

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?

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

FYI: Here is a useful test page for key events: https://w3c.github.io/uievents/tools/key-event-viewer.html

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. Masayuki, could you take a look at these? You might have stronger opinions than I do about this stuff.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document 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.

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.

Comment on attachment 8909814 [details] Bug 1222285 - Part 3: Add test cases 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.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document 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.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document 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?

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

Comment on attachment 8909814 [details] Bug 1222285 - Part 3: Add test cases 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.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document 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 certain keyboard layout according to the content-language of the document 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.

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?

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.

(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.

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

Sounds like a good idea to me. Thanks, Tim.

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.

FYI: I'm really busy this week. So, perhaps, I cannot review them this week, but I'd try to do that if I have much time.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review205158 ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:374 (Diff revision 2) > + > +/* static */ > +bool > +nsRFPService::GetSpoofedKeyCodeInfo(const nsIDocument* aDoc, > + const WidgetKeyboardEvent* aKeyboardEvent, > + FakeKeyboardCode* aOut) I don't like output pointer arguments unless they are absolutely necessary. How about simply using uint_32 as the return type and return 0 if the key code is not available? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:438 (Diff revision 2) > +/* static */ > +bool > +nsRFPService::GetSpoofedModifierStates(const nsIDocument* aDoc, > + const WidgetKeyboardEvent* aKeyboardEvent, > + const nsAString& aModifer, > + bool* aOut) I don't think you need both a boolean return value and boolean `*aOut`. Just return true when the modifier is available. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:466 (Diff revision 2) > + > +/* static */ > +bool > +nsRFPService::GetSpoofedCode(const nsIDocument* aDoc, > + const WidgetKeyboardEvent* aKeyboardEvent, > + nsAString& aOut) Same comment as GetSpoofedKeyCodeInfo. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:493 (Diff revision 2) > + > +/* static */ > +bool > +nsRFPService::GetSpoofedKeyCode(const nsIDocument* aDoc, > + const WidgetKeyboardEvent* aKeyboardEvent, > + uint32_t* aOut) Same comment as GetSpoofedKeyCodeInfo.

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #36) > > +/* static */ > > +bool > > +nsRFPService::GetSpoofedCode(const nsIDocument* aDoc, > > + const WidgetKeyboardEvent* aKeyboardEvent, > > + nsAString& aOut) > > Same comment as GetSpoofedKeyCodeInfo. Sorry, perhaps this one should stay as is.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review205186 ::: dom/events/KeyboardEvent.cpp:45 (Diff revision 2) > > bool > KeyboardEvent::AltKey() > { > - return mEvent->AsKeyboardEvent()->IsAlt(); > + bool altState = mEvent->AsKeyboardEvent()->IsAlt(); > + There's a lot of duplicated code in GetModifierState(), AltKey(), CtrlKey(), ShiftKey(), GetCode(), KeyCode(), and ShouldResistFingerprinting(). It would be good to refactor this code to remove duplication and repeated calls to GetTarget(), GetOwnerGlobal(), etc.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review208296 And perhaps, you need to change KeyboardEvent::GetModifierState() too. ::: dom/events/KeyboardEvent.cpp:50 (Diff revision 2) > + // We need to give a spoofed state for Alt key since it could be used as a modifier > + // key in certain keyboard layout. For example, the '@' key for German keyboard for > + // MAC is Alt+L. > + bool spoofedAltState; > + nsCOMPtr<nsIDocument> doc; > + nsCOMPtr<EventTarget> eventTarget = InternalDOMEvent()->GetTarget(); > + > + if (eventTarget) { > + nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(eventTarget->GetOwnerGlobal()); > + if (win) { > + doc = win->GetExtantDoc(); > + } > + } > + > + if(nsRFPService::GetSpoofedModifierStates(doc, > + mEvent->AsKeyboardEvent(), > + NS_LITERAL_STRING("Alt"), > + &spoofedAltState)) { > + return spoofedAltState; > + } > + > + return altState; Too many times you do same thing. Please create a method which takes |const nsAString& aModifierKey| (or perhaps, using Modifier enum <https://searchfox.org/mozilla-central/rev/45a3df4e6b8f653b0103d18d97c34dd666706358/widget/BasicEvents.h#1133> is better than using string). ::: dom/events/KeyboardEvent.cpp:91 (Diff revision 2) > + // We need to give a spoofed state for Control key since it could be used as a modifier > + // key in certain asian keyboard layouts. > + bool spoofedCtrlState; > + nsCOMPtr<nsIDocument> doc; > + nsCOMPtr<EventTarget> eventTarget = InternalDOMEvent()->GetTarget(); > + > + if (eventTarget) { > + nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(eventTarget->GetOwnerGlobal()); > + if (win) { > + doc = win->GetExtantDoc(); > + } > + } > + > + if (nsRFPService::GetSpoofedModifierStates(doc, > + mEvent->AsKeyboardEvent(), > + NS_LITERAL_STRING("Ctrl"), > + &spoofedCtrlState)) { > + return spoofedCtrlState; > + } > + > + return ctrlState; Same as AltKey(). ::: dom/events/KeyboardEvent.cpp:131 (Diff revision 2) > + bool spoofedShiftState; > + nsCOMPtr<nsIDocument> doc; > + nsCOMPtr<EventTarget> eventTarget = InternalDOMEvent()->GetTarget(); > + > + if (eventTarget) { > + nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(eventTarget->GetOwnerGlobal()); > + if (win) { > + doc = win->GetExtantDoc(); > + } > + } > + > + if (nsRFPService::GetSpoofedModifierStates(doc, > + mEvent->AsKeyboardEvent(), > + NS_LITERAL_STRING("Shift"), > + &spoofedShiftState)) { > + return spoofedShiftState; > + } > + > + return shiftState; Same as AltKey(). ::: dom/events/KeyboardEvent.cpp:219 (Diff revision 2) > + if (!ShouldResistFingerprinting()) { > - mEvent->AsKeyboardEvent()->GetDOMCodeName(aCodeName); > + mEvent->AsKeyboardEvent()->GetDOMCodeName(aCodeName); > + return; > + } > + > + // When fingerprinting resistance is enabled, we will give a spoofed code according Too long line. Please break after "code". ::: dom/events/KeyboardEvent.cpp:219 (Diff revision 2) > + // When fingerprinting resistance is enabled, we will give a spoofed code according > + // to the content-language of the document. > + nsAutoString spoofedCodeName; > + nsCOMPtr<nsIDocument> doc; > + nsCOMPtr<EventTarget> eventTarget = InternalDOMEvent()->GetTarget(); > + > + if (eventTarget) { > + nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(eventTarget->GetOwnerGlobal()); > + if (win) { > + doc = win->GetExtantDoc(); > + } > + } These lines looks same as the code in AltKey(). So, please create a method which returns nsIDocument* and share it between new methods for modifier key methods and these methods. ::: dom/events/KeyboardEvent.cpp:221 (Diff revision 2) > + return; > + } > + > + // When fingerprinting resistance is enabled, we will give a spoofed code according > + // to the content-language of the document. > + nsAutoString spoofedCodeName; Please declare variable immediately before the first use. So, please move this declaration to the previous line of nsRFPService::GetSpoofedCode() call. ::: dom/events/KeyboardEvent.cpp:232 (Diff revision 2) > + if (win) { > + doc = win->GetExtantDoc(); > + } > + } > + > + if (nsRFPService::GetSpoofedCode(doc, mEvent->AsKeyboardEvent(), spoofedCodeName)) { Too long line. Please break after |mEvent->AsKeyboardEvent(),| and start next line below "d" of |doc|. ::: dom/events/KeyboardEvent.cpp:319 (Diff revision 2) > // If this event is initialized with ctor, we shouldn't check event type. > if (mInitializedByCtor) { > return mEvent->AsKeyboardEvent()->mKeyCode; > } > > if (mEvent->HasKeyEventMessage()) { If you revert this condition, you can save the indent level of the following lines. So, please return 0 if this is false. ::: dom/events/KeyboardEvent.cpp:325 (Diff revision 2) > + if (!ShouldResistFingerprinting()) { > - return mEvent->AsKeyboardEvent()->mKeyCode; > + return mEvent->AsKeyboardEvent()->mKeyCode; > - } > + } > + > + // The keyCode should be zero if the char code is given. > + if (CharCode() != 0) { if (CharCode()) { ::: dom/events/KeyboardEvent.cpp:335 (Diff revision 2) > + // according to the content-language of the document. > + nsCOMPtr<nsIDocument> doc; > + nsCOMPtr<EventTarget> eventTarget = InternalDOMEvent()->GetTarget(); > + > + if (eventTarget) { > + nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(eventTarget->GetOwnerGlobal()); Too long line. Please break after |=| and start next line beflow "C" of nsCOMPtr. ::: dom/events/KeyboardEvent.cpp:343 (Diff revision 2) > + } > + } > + > + uint32_t spoofedKeyCode; > + > + if (nsRFPService::GetSpoofedKeyCode(doc, mEvent->AsKeyboardEvent(), &spoofedKeyCode)) { Too long line. Please break after |mEvent->AsKeyboardEvent(),| and start next line below "d" of |doc|. ::: dom/events/KeyboardEvent.cpp:369 (Diff revision 2) > - uint32_t keyCode = mEvent->AsKeyboardEvent()->mKeyCode; > + uint32_t keyCode = KeyCode(); > if (keyCode == NS_VK_RETURN || keyCode == NS_VK_BACK) { > return keyCode; > } I don't think this change is necessary because it checks only if the key event is an Enter or Backspace keypress. Both of them are common keys which are available with any keyboard layouts in the world. So, keep referring raw mKeyCode value here. ::: dom/events/KeyboardEvent.cpp:501 (Diff revision 2) > + // 1. This event is generated by scripts. > + // 2. This event is from Numpad. > + // 3. The pref privcy.resistFingerprinting' is false, we fast return here since > + // we don't need to do any QI of following codes. > + if (mInitializedByCtor || > + mEvent->AsKeyboardEvent()->mLocation == nsIDOMKeyEvent::DOM_KEY_LOCATION_NUMPAD || Too long line. Please break after |==| and indent 2 spaces, i.e., starting from below "v" of mEvent. ::: dom/events/KeyboardEvent.cpp:510 (Diff revision 2) > + > + nsCOMPtr<nsIDocument> doc; > + nsCOMPtr<EventTarget> eventTarget = InternalDOMEvent()->GetTarget(); > + > + if (eventTarget) { > + nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(eventTarget->GetOwnerGlobal()); Too long line. Please break after |=| and start next line below "C" of nsCOMPtr. Although, I don't like to use QI here because it's expensive. However, I have no idea how to skip this... So, let's keep using it for now. ::: dom/events/KeyboardEvent.cpp:517 (Diff revision 2) > + if (window) { > + doc = window->GetExtantDoc(); > + } > + } > + > + return !!doc && !nsContentUtils::IsChromeDoc(doc); I think that |doc && !nsContent...| is okay. ::: toolkit/components/resistfingerprinting/KeyCodeConsensus.h:6 (Diff revision 2) > +/** > + * This file contains the spoofed keycodes for fingerprinting resistance. > + * When privacy.resistFingerprinting is active, we spoof the user's keyboard > + * layout according to the language of the document. > + */ Please explain what should be set to each argument of |CONTROL| and |KEY|. ::: toolkit/components/resistfingerprinting/KeyCodeConsensus.h:16 (Diff revision 2) > + > +/** > + * Spoofed keycodes for English content (US English keyboard layout). > + */ > + > +CONTROL("en", Alt, AltLeft, 18) I guess that the last argument is keyCode value. If so, please use nsIDOMKeyEvent::DOM_VK_* constants instead of raw numbers. And perhaps, you should declare keyboard locales as an enum class whose type is |uint8_t|. Then, it can be stored only sizeof(uint8_t) bytes per key. ::: toolkit/components/resistfingerprinting/KeyCodeConsensus.h:52 (Diff revision 2) > +CONTROL("en", F13, F13, 124) > +CONTROL("en", F14, F14, 125) > +CONTROL("en", F15, F15, 126) > +CONTROL("en", F16, F16, 127) > +CONTROL("en", F17, F17, 128) > +CONTROL("en", F18, F18, 129) > +CONTROL("en", F19, F19, 130) > +CONTROL("en", F20, F20, 131) > +CONTROL("en", F21, F21, 132) > +CONTROL("en", F22, F22, 133) > +CONTROL("en", F23, F23, 134) > +CONTROL("en", F24, F24, 135) Hmm, typically, F13~ keys are not on non-Mac keyboard. Should we expose them? ::: toolkit/components/resistfingerprinting/nsRFPService.h:33 (Diff revision 2) > +// This struct has the information about how to spoof the keyboardEvent.code, > +// keyboardEvent.keycode and modifier states. > +struct FakeKeyboardCode > +{ > + CodeNameIndex mCode; > + uint32_t mKeyCode; Usual keyCode value is 0-255. So, uint8_t is enough in this case. ::: toolkit/components/resistfingerprinting/nsRFPService.h:39 (Diff revision 2) > + Modifiers mModifierStates; > +}; > + > +struct FakeKeyboardInfo > +{ > + nsString mLanguage; So, you should use an enum class whose type is |uint8_t| here. Then, perhaps, it should be last member of this for reducing the instance size. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:59 (Diff revision 2) > Atomic<bool, ReleaseAcquire> nsRFPService::sPrivacyResistFingerprinting; > static uint32_t kResolutionUSec = 100000; > static uint32_t sVideoFramesPerSec; > static uint32_t sVideoDroppedRatio; > static uint32_t sTargetVideoRes; > +nsDataHashtable<KeyboardHashKey, FakeKeyboardCode>* nsRFPService::sFakeKeyboardCodes = nullptr; Although, I don't know the coding rules in this directory. Mozilla's standard coding rules is up to 80 characters per line. So, if you shouldn't use longer line, you should rewrite new long lines by yourself. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:360 (Diff revision 2) > + sFakeKeyboardCodes = new nsDataHashtable<KeyboardHashKey, FakeKeyboardCode>( > + ArrayLength(keyCodeArray) > + ); > + > + // Subtract one from the length because of the trailing null > + for (uint32_t i = 0; i < ArrayLength(keyCodeArray) - 1; ++i) { Why don't you use range based for loop? I.e., |for (const FakeKeyboardInfo& keyboardInfo : keyCodeArray) {| Then, I feel that keyCodeArray is odd name. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:361 (Diff revision 2) > + ArrayLength(keyCodeArray) > + ); > + > + // Subtract one from the length because of the trailing null > + for (uint32_t i = 0; i < ArrayLength(keyCodeArray) - 1; ++i) { > + KeyboardHashKey key(keyCodeArray[i].mLanguage, If you need string name of language, you should create a method into the struct, which returns |const char*| rather than nsString.

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/#review208298 ::: layout/base/PresShell.cpp:7949 (Diff revision 2) > + if (aEvent->IsBlockedForFingerprintingResistance()) { > + aEvent->mFlags.mOnlySystemGroupDispatchInContent = true; > + } Hmm, could you check if this breaks "Alt" key behavior with menubar on Windows? ::: widget/WidgetEventImpl.cpp:493 (Diff revision 2) > + if (keyboardEvent->mKeyNameIndex == KEY_NAME_INDEX_Alt || > + keyboardEvent->mKeyNameIndex == KEY_NAME_INDEX_Shift || > + keyboardEvent->mKeyNameIndex == KEY_NAME_INDEX_AltGraph) { > + return true; > + } Why don't you check KEY_NAME_INDEX_Control? Some keyboard layouts requires Control key to input some characters on Windows.

I'd like to check the expected result in the automated test more. Sorry for taking long time.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review208300 ::: toolkit/components/resistfingerprinting/KeyCodeConsensus.h:23 (Diff revision 2) > +CONTROL("en", ArrowLeft, ArrowLeft, 37) > +CONTROL("en", ArrowRight, ArrowRight, 39) > +CONTROL("en", ArrowUp, ArrowUp, 38) > +CONTROL("en", Backspace, Backspace, 8) > +CONTROL("en", CapsLock, CapsLock, 20) > +CONTROL("en", ContextMenu, ContextMenu, 93) Compact keyboards don't have context menu key. And also on macOS. So, shouldn't we stop exposing ContextMenu key? ::: toolkit/components/resistfingerprinting/KeyCodeConsensus.h:29 (Diff revision 2) > +CONTROL("en", Control, ControlLeft, 17) > +CONTROL("en", Delete, Delete, 46) > +CONTROL("en", End, End, 35) > +CONTROL("en", Enter, Enter, 13) > +CONTROL("en", Escape, Escape, 27) > +CONTROL("en", Help, Help, 6) Perhaps, Help key is available only with a few keyboards like Sun keyboard with Linux. Perhaps, we shouldn't expose this. ::: toolkit/components/resistfingerprinting/KeyCodeConsensus.h:35 (Diff revision 2) > +CONTROL("en", Pause, Pause, 19) > +CONTROL("en", PrintScreen, PrintScreen, 44) > +CONTROL("en", ScrollLock, ScrollLock, 145) Pause, PrintScreen and ScrollLock are available only on non-macOS. However, I'm not sure if they should be stopped being exposed on web contents.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review208296 > Hmm, typically, F13~ keys are not on non-Mac keyboard. Should we expose them? I agree with that we should't expose them since they might reveal the platform. > So, you should use an enum class whose type is |uint8_t| here. Then, perhaps, it should be last member of this for reducing the instance size. I have tested with both placements of this field, the result shows the same in terms of instance size. So, I still put this here, but I did change this into uint8_t.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review208300 > Compact keyboards don't have context menu key. And also on macOS. So, shouldn't we stop exposing ContextMenu key? We should not expose keys which might tell what platform it is or what platform it's not. So, I stop expose this key. > Perhaps, Help key is available only with a few keyboards like Sun keyboard with Linux. Perhaps, we shouldn't expose this. Ditto. > Pause, PrintScreen and ScrollLock are available only on non-macOS. However, I'm not sure if they should be stopped being exposed on web contents. Ditto.

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/#review208298 > Hmm, could you check if this breaks "Alt" key behavior with menubar on Windows? Sorry, I haven't test this since I need to setup my Window. I will update this as soon as I get result. > Why don't you check KEY_NAME_INDEX_Control? Some keyboard layouts requires Control key to input some characters on Windows. Yes, you are right. I missed this.

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/#review208298 > Sorry, I haven't test this since I need to setup my Window. I will update this as soon as I get result. I have tried with my Windows, it doesn't break "Alt" key behavior with menubar.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review213356 ::: dom/events/KeyboardEvent.cpp:168 (Diff revision 4) > + nsAutoString spoofedCodeName; > + > + if (nsRFPService::GetSpoofedCode(doc, mEvent->AsKeyboardEvent(), > + spoofedCodeName)) { > + aCodeName.Assign(spoofedCodeName); > + } Why don't you just do this?: nsRFPService::GetSpoofedCodde(doc, mEvent->AsKeyboardEvent(), aCodeName); GetSpoofedCode is also taking nsAString& as an out param. ::: dom/events/KeyboardEvent.cpp:452 (Diff revision 4) > + mEvent->AsKeyboardEvent()->mLocation == > + nsIDOMKeyEvent::DOM_KEY_LOCATION_NUMPAD || > + !nsContentUtils::ShouldResistFingerprinting()) { Calling WidgetEvent::AsKeyboardEvent() is a virtual call. On the other hand, nContentUtils::ShouldResistFingerprinting() is really fast when the pref is false. For making most users faster, please check nContentUtils::ShouldResistFingerprinting() at second, then, finally, check the location. ::: toolkit/components/resistfingerprinting/KeyCodeConsensus.h:77 (Diff revision 4) > +CONTROL(KeyboardLocale::EN, F8, F8, nsIDOMKeyEvent::DOM_VK_F8) > +CONTROL(KeyboardLocale::EN, F9, F9, nsIDOMKeyEvent::DOM_VK_F9) > +CONTROL(KeyboardLocale::EN, F10, F10, nsIDOMKeyEvent::DOM_VK_F10) > +CONTROL(KeyboardLocale::EN, F11, F11, nsIDOMKeyEvent::DOM_VK_F11) > +CONTROL(KeyboardLocale::EN, F12, F12, nsIDOMKeyEvent::DOM_VK_F12) > +// Leaving "F13" to "F24" key unimplemented; they are MACOS only. FYI: Not only on macOS, but typically, F13~F19 are on Mac's full keyboard. F20~F24 are available on Windows and Linux, although, usual keyboard doesn't have them. Finally, F25~ are only available on Linux. https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/widget/NativeKeyToDOMKeyName.h#711-762 ::: toolkit/components/resistfingerprinting/nsRFPService.h:59 (Diff revision 4) > +{ > +public: > + typedef const KeyboardHashKey& KeyType; > + typedef const KeyboardHashKey* KeyTypePointer; > + > + KeyboardHashKey(const KeyboardLocales aLocale, nit: redundant whitespace between KeyboardLocales and aLocale. ::: toolkit/components/resistfingerprinting/nsRFPService.h:67 (Diff revision 4) > + : mLocale(aLocale) > + , mKeyIdx(aKeyIdx) > + , mKey(aKey) > + {} > + > + explicit KeyboardHashKey(KeyTypePointer other) nit: s/other/aOther ::: toolkit/components/resistfingerprinting/nsRFPService.h:73 (Diff revision 4) > + KeyboardHashKey(KeyType other) > + : mLocale(other.mLocale) > + , mKeyIdx(other.mKeyIdx) > + , mKey(other.mKey) nit: s/other/aOther ::: toolkit/components/resistfingerprinting/nsRFPService.h:82 (Diff revision 4) > + bool KeyEquals(KeyTypePointer other) const > + { > + return mLocale == other->mLocale && > + mKeyIdx == other->mKeyIdx && > + mKey == other->mKey; > + } nit: s/other/aOther ::: toolkit/components/resistfingerprinting/nsRFPService.h:107 (Diff revision 4) > + KeyboardLocales mLocale; > + KeyNameIndexType mKeyIdx; > + nsString mKey; There are some redundant whitespaces between types and member names. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:338 (Diff revision 4) > +nsRFPService::CreateSpoofingKeyCodes() > +{ > + if (sSpoofingKeyboardCodes) { > + return; > + } > + > + static const SpoofingKeyboardInfo spoofingKeyboardInfoTable[] = { > +#define KEY(lang_, key_, _codeNameIdx, _keyCode, _modifier) \ > + { lang_, KEY_NAME_INDEX_USE_STRING, NS_LITERAL_STRING(key_), { CODE_NAME_INDEX_##_codeNameIdx, _keyCode, _modifier } }, > +#define CONTROL(lang_, keyNameIdx_, _codeNameIdx, _keyCode) \ > + { lang_, KEY_NAME_INDEX_##keyNameIdx_, EmptyString(), { CODE_NAME_INDEX_##_codeNameIdx, _keyCode, MODIFIER_NONE } }, > +#include "KeyCodeConsensus.h" > +#undef CONTROL > +#undef KEY When we'll support other keyboard layouts, each keyboard layout should be initialized when it becomes necessary. So, perhaps, this method should take a locale as an argument and need to switch to include which header file with the argument? But how? Do you have some idea for this issue? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:415 (Diff revision 4) > + // We sanitize the lanague here, for example, en-US to en. > + language.StripWhitespace(); > + int32_t idx = language.FindChar('-'); > + if (idx != -1) { > + language.Truncate(idx); > + } > + > + keyboardLocale = GetKeyboardLocale(language); How do you switch for en-US vs. en-UK (en-GB)? IIRC, usual keyboard layouts in America and England are different but those language code is "en". I think that this issue is important for some languages, e.g., "es" and "ar" which are used in a lot of countries. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:478 (Diff revision 4) > +{ > + MOZ_ASSERT(aKeyboardEvent); > + > + SpoofingKeyboardCode keyCodeInfo; > + > + if (GetSpoofedKeyCodeInfo(aDoc, aKeyboardEvent, keyCodeInfo)) { If GetSoofedKeyCodeInfo() returns false, this method always returns false. So, I'd like you to use early return style here. Then, you can save indent level of the following code. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:479 (Diff revision 4) > + nsAutoString code; > + WidgetKeyboardEvent::GetDOMCodeName(keyCodeInfo.mCode, code); If you use aOut directly here, you don't need to copy the string below. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:484 (Diff revision 4) > + if (aKeyboardEvent->mLocation == nsIDOMKeyEvent::DOM_KEY_LOCATION_RIGHT) { > + code.ReplaceSubstring(u"Left", u"Right"); ReplaceSubstring looks like slow for here. Why don't you just check the |code| is ends with "Left" and if so, replace it with just "Right"? StringEndsWith(): https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/xpcom/string/nsReadableUtils.cpp#1101 nsAString::Replace(): https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/xpcom/string/nsTSubstring.h#307 ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:487 (Diff revision 4) > + // We need to change the 'Left' with 'Right' if the location indicates > + // it's a right key. > + if (aKeyboardEvent->mLocation == nsIDOMKeyEvent::DOM_KEY_LOCATION_RIGHT) { > + code.ReplaceSubstring(u"Left", u"Right"); > + } > + aOut.Truncate(); I think that you don't need to call Truncate() immediately before calling Assign().

Comment on attachment 8909814 [details] Bug 1222285 - Part 3: Add test cases to check whether keyEvents been correctly spoofed and modifier keys been correctly suppressed. https://reviewboard.mozilla.org/r/181318/#review213358 ::: browser/components/resistfingerprinting/test/browser/browser_spoofing_keyboard_event.js:67 (Diff revision 4) > + result: { key: "Home", code: "Home", charCode: 0, keyCode: nsIDOMKeyEvent.DOM_VK_HOME, > + location: KeyboardEvent.DOM_KEY_LOCATION_STANDARD, altKey: false, shiftKey: false, > + ctrlKey: false, altGraphKey: false } > + }, > + { key: "KEY_Meta", modifiers: { location: KeyboardEvent.DOM_KEY_LOCATION_LEFT, metaKey: true }, > + expectedKeyEvent: SHOULD_DELIVER_KEYDOWN, Although, must be out of scope of this bug, this odd behavior on macOS will cause leaking platform information because on Windows and Linux, "OS" keys will be mapped "Meta" in the future... ::: browser/components/resistfingerprinting/test/browser/browser_spoofing_keyboard_event.js:736 (Diff revision 4) > + // Test that will system group event get real code and keyCode. > + await new Promise(resolve => { > + gBrowser.addEventListener("keydown", function keyEventHandler(aEvent) { > + is(aEvent.code, "Minus", " The system group event should get real code."); > + is(aEvent.keyCode, 63, " The system group event should get real keyCode."); > + > + gBrowser.removeEventListener("keydown", keyEventHandler); > + resolve(); > + }); > + > + // Send key event to the system group. > + EventUtils.synthesizeKey("\u00DF", { code: "Minus", keyCode: 63 }); > + }); > + > + // Test that will system group event still get suppressed modifier keys > + await new Promise(resolve => { > + gBrowser.addEventListener("keydown", function keyEventHandler(aEvent) { > + is(aEvent.key, "Alt", "The system group event get the suppressed keyboard event.") > + > + gBrowser.removeEventListener("keydown", keyEventHandler); > + resolve(); > + }); > + > + // Send key event to the system group. > + EventUtils.synthesizeKey("KEY_Alt", { altKey: true }); > + }); Hmm. gBrower is in a chrome document and you use addEventListener. So, this doesn't test system event group's behavior actually. When you need to test it, you need to create mochitest and use SpecialPowers.addSystemEventListener().

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review213360 ::: dom/events/KeyboardEvent.cpp:42 (Diff revision 4) > KeyboardEvent::AltKey() > { > - return mEvent->AsKeyboardEvent()->IsAlt(); > + bool altState = mEvent->AsKeyboardEvent()->IsAlt(); > + > + if (!ShouldResistFingerprinting()) { > + return altState; > + } Well, it might be that this is not enough to check for system event group handlers. Currently, you try to check if the document is chrome document or not. However, system event group handlers may be in content document. E.g., Gecko's editor's keyboard event listeners, form controllers' event listeners, etc. So, like Event.defaultPrevented, we might need to mark each attribute of KeyboardEvent as [NeedsCallerType] in KeyboardEvent.webidl and each method of dom::KeyboardEvent needs to take CallerType as its argument. https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/webidl/Event.webidl#39-40 https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/events/Event.cpp#1093,1105-1106 https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/events/Event.h#198-206

In other words, perhaps, you don't need to check if the document is a chrome document if each method gets the context.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review213356 > How do you switch for en-US vs. en-UK (en-GB)? IIRC, usual keyboard layouts in America and England are different but those language code is "en". I think that this issue is important for some languages, e.g., "es" and "ar" which are used in a lot of countries. I have talked with Arthur about this. Yes, we should not only include the language but also the locale here. I will make some changes around this. Thanks.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review213356 > When we'll support other keyboard layouts, each keyboard layout should be initialized when it becomes necessary. > > So, perhaps, this method should take a locale as an argument and need to switch to include which header file with the argument? But how? Do you have some idea for this issue? AFAIK, we cannot really switch what we include in c++ on the fly. What I propose here is that we initialize all keyboard layouts when 'privacy.resistFingerprinting' is flipping into true. I know that it seems a waste of memory if some languages have never be used. But, the reason for why I chose this approach is twofold. 1. It doesn't spend too much memory if we only store a pointer to SpoofingKeyboardCode for each code in the hash table. Right now, we make a new copy of the SpoofingKeyboardCode when we initializing the hash table. So, it's going to waste a lot of memory if the given language has never be touched. We can improve this by using a pointer instead of a whole copy, then the memory consumption would be acceptable. 2. It would be faster. We don't need to spend time on generating a spoofing keyboard table when it becomes necessary since it's generated beforehand. What do you think about this, Masayuki? In addition, I will make generating a new spoofing keyboard layout more friendly, like putting an individual locale into a standalone file and include them in one place.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review213356 > AFAIK, we cannot really switch what we include in c++ on the fly. > > What I propose here is that we initialize all keyboard layouts when 'privacy.resistFingerprinting' is flipping into true. I know that it seems a waste of memory if some languages have never be used. But, the reason for why I chose this approach is twofold. > 1. It doesn't spend too much memory if we only store a pointer to SpoofingKeyboardCode for each code in the hash table. Right now, we make a new copy of the SpoofingKeyboardCode when we initializing the hash table. So, it's going to waste a lot of memory if the given language has never be touched. We can improve this by using a pointer instead of a whole copy, then the memory consumption would be acceptable. > 2. It would be faster. We don't need to spend time on generating a spoofing keyboard table when it becomes necessary since it's generated beforehand. > > What do you think about this, Masayuki? > > In addition, I will make generating a new spoofing keyboard layout more friendly, like putting an individual locale into a standalone file and include them in one place. > 2. It would be faster. We don't need to spend time on generating a spoofing keyboard table when it becomes necessary since it's generated beforehand. Hmm, I don't think so. In a lot of Quantum Flow working, I learned that adding entries to hashtable is too slow. Especially, if the pref is true, this makes startup time slower if there is a lot of keyboard layouts defined. Fortunately, "KeyCodeConsensus.h" is included only from nsRFPService::CreateSpoofingKeyCodes(). So, cannot you stop using macro to define the table? Then, perhaps, you can create static methods to add key definitions to given hashtable for every keyboard layout?

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review218734 Looks good to me except following some nits. And this changes .webidl. That requires DOM peer's review too. So, please ask additional review after modifying this patch. ::: dom/events/KeyboardEvent.h:46 (Diff revision 5) > - bool AltKey(); > - bool CtrlKey(); > - bool ShiftKey(); > + bool AltKey(CallerType aCallerType = CallerType::NonSystem); > + bool CtrlKey(CallerType aCallerType = CallerType::NonSystem); > + bool ShiftKey(CallerType aCallerType = CallerType::NonSystem); > bool MetaKey(); > > - bool GetModifierState(const nsAString& aKey) > + bool GetModifierState(const nsAString& aKey, > + CallerType aCallerType = CallerType::NonSystem) > { > - return GetModifierStateInternal(aKey); > + bool modifierState = GetModifierStateInternal(aKey); > + > + if (!ShouldResistFingerprinting(aCallerType)) { > + return modifierState; > + } > + > + Modifiers modifier = WidgetInputEvent::GetModifier(aKey); > + return GetSpoofedModifierStates(modifier, modifierState); > } > > bool Repeat(); > bool IsComposing(); > uint32_t CharCode(); > - uint32_t KeyCode(); > + uint32_t KeyCode(CallerType aCallerType = CallerType::NonSystem); > virtual uint32_t Which() override; > uint32_t Location(); > > - void GetCode(nsAString& aCode); > + void GetCode(nsAString& aCode, CallerType aCallerType = CallerType::NonSystem); Hmm, default value is NonSystem... That is better for security. However, it's really risky for regression. Looks like that you don't change any caller of these methods. E.g., here: https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/layout/xul/MenuBoxObject.cpp#106 Perhaps, making default caller type to CallerType::System is better since there must not be problem to call them with CallerType::System unexpectedly. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:376 (Diff revision 5) > + { KEY_NAME_INDEX_USE_STRING, NS_LITERAL_STRING(key_), \ > + { CODE_NAME_INDEX_##_codeNameIdx, _keyCode, _modifier } }, > +#define CONTROL(keyNameIdx_, _codeNameIdx, _keyCode) \ > + { KEY_NAME_INDEX_##keyNameIdx_, EmptyString(), \ > + { CODE_NAME_INDEX_##_codeNameIdx, _keyCode, MODIFIER_NONE } }, > +#include "KeyCodeConsensus_En_US.inc" Although I'm not sure, as far as I know, we use .h for this purpose. So, perhaps, you should rename it to .h.

Comment on attachment 8909814 [details] Bug 1222285 - Part 3: Add test cases to check whether keyEvents been correctly spoofed and modifier keys been correctly suppressed. https://reviewboard.mozilla.org/r/181318/#review218736 Assuming that this won't cause random orange.

Hi Smaug, Could you review the change of webidl file for the Part 1? Thanks.

Comment on attachment 8909812 [details] Bug 1222285 - Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. https://reviewboard.mozilla.org/r/181314/#review218794 ::: dom/webidl/KeyboardEvent.webidl:20 (Diff revision 6) > readonly attribute boolean altKey; > + [NeedsCallerType] > readonly attribute boolean ctrlKey; > + [NeedsCallerType] > readonly attribute boolean shiftKey; > readonly attribute boolean metaKey; Could you explain why metaKey doesn't need to be changed.

(In reply to Olli Pettay [:smaug] (please try to find other reviewers for non-web components patches) from comment #69) > > Could you explain why metaKey doesn't need to be changed. > It is because the metaKey doesn't involve the modifier state of keyboard layouts. In other words, we cannot tell what the language of the current keyboard layout is by inspecting metakey since the key won't change no matter whether the meta key is pressed. So, we don't need to spoof this value and that's why we don't change it.

Try looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=122520cca519

Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7140b4a3c47a Part 1: Spoofing the keyboard event to mimc a certain keyboard layout according to the content-language of the document when 'privacy.resistFingerprinting' is true. r=arthuredelstein,masayuki,smaug https://hg.mozilla.org/integration/autoland/rev/b2f55468b241 Part 2: Making the keyboard events of modifier keys been suppressed when 'privacy.resistFingerprinting' is true. r=arthuredelstein,masayuki https://hg.mozilla.org/integration/autoland/rev/d83173f04756 Part 3: Add test cases to check whether keyEvents been correctly spoofed and modifier keys been correctly suppressed. r=arthuredelstein,masayuki

https://hg.mozilla.org/mozilla-central/rev/7140b4a3c47a https://hg.mozilla.org/mozilla-central/rev/b2f55468b241 https://hg.mozilla.org/mozilla-central/rev/d83173f04756

This patch also alters the events seen by webextensions when they use `window.addEventListener("keydown", fn, true)`, is this the expected behavior? If yes, is there a way for webextensions to get the "real" event? If no, is there a flag I can set in about:config in order to disable this specific feature while keeping every other privacy feature offered by privacy.resistFingerprinting? If no, would a patch that would change the answer to any of these questions be welcome?

(In reply to glacambre from comment #76) > This patch also alters the events seen by webextensions See Bug 1450398 (similar to Bug 1377744)