Closed Bug 1343955 Opened 8 years ago Closed 6 years ago

Implement trustedKeyEvent in FuzzingFunctions

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: mccr8, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

Bug 1343955 - part 1: Add automated tests for synthesizeKey() of EventUtils.js
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
Bug 1343955 - part 2: Implement _guessCodeFromKeyName() in EventUtils.js with C++ and make it accessible with nsITextInputProcessor for EventUtils.js
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
Bug 1343955 - part 3: Implement similar C++ utility of _computeKeyCodeFromChar() in EventUtils.js and make it accssible with nsITextInputProcessor for EventUtils.js
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
Bug 1343955 - part 4: Implement static API to synthesize keyboard events into FuzzingFunctions
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
Bug 1343955 - part 0: Fix bug of browser_panel_keyboard_navigation.js
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review 
I've seen at least one test case with this method from domFuzz.
Priority: -- → P2
Assignee: nobody → continuation
What does this actually do? Mark the script created event trusted or that and also dispatch event to widget level code? Latter might be more useful for testing. Either somehow exposing parts of the functionality of DOMWindowUtils or nsITextInputProcessor?
This is defined here:
  https://github.com/MozillaSecurity/funfuzz/blob/master/dom/extension/content/fuzzPriv.js#L122
It runs as chrome. Maybe it isn't a realistic thing to have.
yeah, ok, that just dispatches trusted key events, which means it bypasses EventStateManager and such.
So it covers only parts of key event default handling.
It looks like bug 894118 was to track these bugs, but I'm not sure how complete it is. Gary do you know anything about this?
Blocks: fuzz-keys
Yeah, it looks like nothing has been filed against that bug for 4 years, so this probably isn't too high priority. A bug was reported recently (bug 1343642) using it, but it looks like it isn't clear if it is a bug.
(In reply to Jesse Schwartzentruber (:truber) from comment #4)
> It looks like bug 894118 was to track these bugs, but I'm not sure how
> complete it is. Gary do you know anything about this?

It may already have been part of domfuzz, see bug 894118 comment 1 which references:

https://github.com/MozillaSecurity/funfuzz/blob/master/dom/fuzzer/modules/keyboard-events.js
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #6)
> It may already have been part of domfuzz, see bug 894118 comment 1 which
> references:

Yeah, but this bug is about reimplementing that function in fuzzing builds of Firefox itself because domfuzz will stop working once XUL addons no longer work.
Assignee: continuation → nobody
API to synsthesize KeyboardEvents via widget won't deprecated even after 57 because that tests a lot in mochitest. If you need such API to WebExtensions, I think that you should request it and the team should declare such dangerous API being allowed only for some special extensions which need to test browser itself (with special privilege?).

BTW, if you need an API to synthesize keyboard event with realistic path, it should be:

* using nsITextInputProcessor to dispatch keyboard events via widget and to share actual dispatching event path, TextEventDispatcher, with actual products.
* not dispatching synchronously, because user input is never handled in the stack after event listener. (Dispatching user input events from event listener may cause impossible situation in some classes.)

Although, nsITextInputProcessor isn't e10s aware (dispatching KeyboardEvent and CompositionEvent via PuppetWidget if you use it in content process).
If we can get this implemented as part of a WebExtension, would it be possible to have the other fuzzPriv functions added as well?

Memory Functions:
fuzzPriv.ccSlice
fuzzPriv.finishCC
fuzzPriv.CC
fuzzPriv.MP
fuzzPriv.forceShrinkingGC
fuzzPriv.forceGC
fuzzPriv.schedulePreciseShrinkingGC
fuzzPriv.schedulePreciseGC
fuzzPriv.CCLog
fuzzPriv.gczeal

Misc Functions:
fuzzPriv.zoom
fuzzPriv.printToFile
fuzzPriv.getMemoryReports
fuzzPriv.callDrawWindow
fuzzPriv.enableBookmarksToolbar
fuzzPriv.disableBookmarksToolbar
Blocks: 1346339
These probably can't be implemented as a WebExtension. Instead, I am adding them to FuzzingFunctions, implementing them in C++. Then a WebExtension could call into those methods, or fuzzers could call them directly.

CC and forceGC are just the same thing as what I implemented in bug 1322400.

I did a search in bugzilla for "fuzzPriv" to try to find test cases to see what has actually ever been used. Actually, just doing a Google search for "fuzzPriv.schedulePreciseShrinkingGC" or whatever is probably more effective. Feel free to file separate bugs for ones that might actually be useful, cc me, and make them blocking bug 1346339. (For instance, IIRC ccSlice only ever found bugs with the ccSlice method, not actual bugs in the CC.) Most of these are probably just a few lines of code, but we should prioritize the useful ones.
Masayuki, do you have time to reimplement this in the FuzzingFunctions webidl interface? It would be useful for enabling the verification of security bugs on branches besides Nightly. I would look into it, but I'm not sure how the JS in comment 11 translates into C++. Thanks.
Flags: needinfo?(masayuki)
Unfortunately, I don't have much time to work on this. Although if somebody requests reviews to me, I'll do that around input API.

I still don't understand why the API dispatches trusted but script generated keyboard events. Especially, I don't like that such structure allows nested event loop. E.g., dispatching a key event and listens to it by the caller itself, then, the listener can dispatch another event.  This is impossible case in real world.  Similarly, not sending key events to PresShell nor EventStateManager allows impossible cases, e.g., KeyboardEvent.isComposing is never initialized, access keys are never triggered, a default action of preceding keydown event isn't handled but maybe following keypress event handler may assume the result of default handler.  Therefore, currently, I'm thinking that such events shouldn't be acceptable in anywhere because reducing code path makes us simpler and safer.

Note that we need to generate beforeinput event when ESM receives eKeyPress or eCompositionChange event and an editor has focus. So, such bypassing path is really annoying to me.
Flags: needinfo?(masayuki)
Yeah, that makes sense. This does seem to keep finding things when people fuzz, but it doesn't really ever seem clear how much of a real issue the things it finds are.
While working on a security bug recently that used the DomFuzz Helper
extension I found that it's increasingly hard to make the legacy
version work.  This is bad since the newer Webextension is lacking
features that the old legacy extension has, like "trustedKeyEvent".

Someone needs to fix this soon, because I fear that the old legacy
extension will probably stop working altogether as some point.
(In reply to Mats Palmgren (:mats) from comment #16)

> Someone needs to fix this soon, because I fear that the old legacy
> extension will probably stop working altogether as some point.

Fix what, exactly? AFAIK, the things missing from the webextension are largely missing because they can't be done in the webextension. 

Adding Jesse since he works on the webextension and isn't going to see a bug comment if no one puts him on the bug. :-)
Flags: needinfo?(jschwartzentruber)
And, duh, then I look up and see what bug I'm in... in any case, Jesse should probably be on this bug.
Flags: needinfo?(jschwartzentruber)
Masayuki, are you going to be able (and willing) to do any work on this soon? Based on discussion within my fuzzing team and from Mats here, we think this work needs to be done. If you're not willing to do it, is there anyone else that could do this work?
Flags: needinfo?(masayuki)
(In reply to [PTO until July 27] from comment #19)
> Masayuki, are you going to be able (and willing) to do any work on this
> soon? Based on discussion within my fuzzing team and from Mats here, we
> think this work needs to be done. If you're not willing to do it, is there
> anyone else that could do this work?

Unfortunately, I don't have much time and no people who were working on around DOM events and has much time.

On the other hand, fixing this must not be so difficult if you do not want to JS generated events because we've already have similar API for mochitests:
https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/testing/mochitest/tests/SimpleTest/EventUtils.js#883-961
(FYI: nsITextInputProcessor does NOT run only native key nor IME event handling code. In other words, it can test PresShell, EventStateManager and any event listeners in the DOM tree.)

So, there is somebody who is aware of WebExtensions API and permissions, he/she can fix this.
Flags: needinfo?(masayuki)
For SVG the context-{fill,stroke,etc.} feature, we only enable it for select addons from Mozilla/Testpilot:

  https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/layout/svg/SVGContextPaint.cpp#42-82

We could take a similar approach here and only enable this API for the fuzzPriv extension, and only if signed by Mozilla or something of the sort.
Hi David, if I've understood this bug correctly, it seems like we want to add an API that can be accessed by certain Web Extensions for fuzz testing (specifically the fuzzPriv extension). Assuming we can do the platform work to add the API and only enable it for certain add-ons, is there someone on the add-ons team who can give guidance about what tests we need to add when adding a new Web Extensions API?
Flags: needinfo?(ddurst)
FuzzingFunctions is a WebIDL interface that you can call from content (in fuzzing builds with a certain pref enabled), so There shouldn't need to be any WebExtensions API needed for this.
Masayuki, it sounds like you don't need to do anything Web Extensions-specific after all.
Flags: needinfo?(ddurst)
Bug 1348028 has an example of how you add a new function:
1) Add a new method to FuzzingFunctions.webidl.
2) Implement the WebIDL method in C++.
That's enough for the fuzzers to start using it, once the fuzzing people know about it.
Hmm, sunds like simpler. Is it build only with specific build option?
https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/dom/webidl/moz.build#1026-1029

Dispatching trusted key events is really dangerous even used by usual add-ons. So, we shouldn't allow that anybody except our controllable add-ons.
Flags: needinfo?(continuation)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #26)
> Hmm, sunds like simpler. Is it build only with specific build option?
> https://searchfox.org/mozilla-central/rev/
> d160ac574a5798010eda4e33e91ee628aa268a33/dom/webidl/moz.build#1026-1029
> 
> Dispatching trusted key events is really dangerous even used by usual
> add-ons. So, we shouldn't allow that anybody except our controllable add-ons.

Yes, this is only allowed in builds that are made with
  ac_add_options --enable-fuzzing
(which is not set in the usual opt or debug builds) and you also have to have the pref fuzzing.enabled set to true. (I think enable-fuzzing may actually only work with ASan builds right now, if you are trying it locally.)
Flags: needinfo?(continuation)
See Also: → 1348213
Hmm, I guess that we should add a method (FuzzingFunctions.setTrust?) to set trusted bit (calling Event::SetTrusted) on fuzzing build only.
Taking. Perhaps, we should use nsITextInputProcessor internally in the new API since dispatching only DOM events into the DOM causes that such events may be ignored a lot. For example, such events do not have WidgetGUIEvent::mWidget and if event listener wants to handle events with widget, they may just return error if mWidget is nullptr.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Here is a prototype patch:
https://hg.mozilla.org/try/rev/e2dce786d489f58a89137418a30f5d0358eaf169

I've not tested this patch yet because I've not succeeded to build with |ac_add_options --enable-fuzzing| (failed to link). If somebody checks whether the API design is fine or not, and whether it works fine or not.
(not yet aware of macOS)
https://hg.mozilla.org/try/rev/c85df05ae5be3757311ade1e46154cde634a3239
https://hg.mozilla.org/try/rev/c14795eb002b7266ab6bb0a9c933bfa464e23936
https://hg.mozilla.org/try/rev/ecd4072376b4fab4593df8d9c3573277499cc596
https://hg.mozilla.org/try/rev/b474717fbc8a5d5a742b1403ad3d523c3c001f36

Here are the patches to implement new API for fuzzing. Do you agree with this new API? Unfortunately, I've not tested the new API yet since I've not succeeded to build it due to linker errors. So, I'd be happy if you'd check both API design and the API works well.
Flags: needinfo?(continuation)
FYI: I'll be back to my editor work soon. So, I'd like to fix this as soon as possible before staking my patches onto my local repository.
Flags: needinfo?(twsmith)
Flags: needinfo?(jschwartzentruber)
I think that's a better question for a fuzzing person. (Al has needinfo'd the relevant fuzzing people.) They should even be able to try your patches in their fuzzers.
Flags: needinfo?(continuation)
I tried an opt build and was only able to get the API to throw NS_ERROR_FAILURE or NS_ERROR_NOT_AVAILABLE with testcases like:

<input type=text id=a value="123"></input>
<script>
addEventListener("DOMContentLoaded", () => {
  FuzzingFunctions.synthesizeKeyboardEvents("End");
  FuzzingFunctions.synthesizeKeyboardEvents("h");
  FuzzingFunctions.synthesizeKeyboardEvents("e");
  FuzzingFunctions.synthesizeKeyboardEvents("l");
  FuzzingFunctions.synthesizeKeyboardEvents("l");
  FuzzingFunctions.synthesizeKeyboardEvents("o");
  FuzzingFunctions.synthesizeKeyboardEvents("a", { controlKey: true });
})
</script>

I will try a debug build tomorrow morning to see what warnings are generated. It would be nice if these were in the exception that is thrown. I like the format of the API a lot.
Flags: needinfo?(twsmith)
Thank you and sorry for the bug of the API. I still don't have idea of the cause. But I guess that it fails to get a focused widget from nsFocusManager...
Flags: needinfo?(jschwartzentruber)
>   FuzzingFunctions.synthesizeKeyboardEvents("a", { controlKey: true });

nit: ctrlKey

but I guess, this does not cause the exception.
Ah, sorry. I had a.focus(); as the first line in the event handler.

<input type=text id=a value="123"></input>
<script>
addEventListener("DOMContentLoaded", () => {
  a.focus();
  FuzzingFunctions.synthesizeKeyboardEvents("End");
  FuzzingFunctions.synthesizeKeyboardEvents("h");
  FuzzingFunctions.synthesizeKeyboardEvents("e");
  FuzzingFunctions.synthesizeKeyboardEvents("l");
  FuzzingFunctions.synthesizeKeyboardEvents("l");
  FuzzingFunctions.synthesizeKeyboardEvents("o");
  FuzzingFunctions.synthesizeKeyboardEvents("a", { ctrlKey: true });
})
</script>

I also tried setting the event on a.onclick, which also throws.

<input type=text id=a value="123"></input>
<script>
a.addEventListener("click", () => {
  FuzzingFunctions.synthesizeKeyboardEvents("End");
  FuzzingFunctions.synthesizeKeyboardEvents("h");
  FuzzingFunctions.synthesizeKeyboardEvents("e");
  FuzzingFunctions.synthesizeKeyboardEvents("l");
  FuzzingFunctions.synthesizeKeyboardEvents("l");
  FuzzingFunctions.synthesizeKeyboardEvents("o");
  FuzzingFunctions.synthesizeKeyboardEvents("a", { ctrlKey: true });
})
</script>

Btw. I got controlKey from the examples given in the webIDL.
Thanks, which warning(s) do you see in your terminal when you run it on debug build?
(if you use "./mach run", you may need to use "2>&1 | tee" to see the warning(s).)
(And maybe MOZ_DISABLE_CONTENT_SANDBOX=1 is required.)
I wasn't able to get debug to build locally, but it works in try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=26c742af503f53ea65e09896513276dcb5e3f909&selectedJob=197635975)

With both tests I see this:

[Child 5597, Main Thread] WARNING: '!window', file /builds/worker/workspace/build/src/dom/base/FuzzingFunctions.cpp, line 177
JavaScript error: file:///home/truber/builds/bug1343955-try/test.html, line 4: NS_ERROR_FAILURE:

That's with the env var set, but it doesn't seem to matter.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #50)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=466dd262faf29884120693db5c67b46b626b41da

Okay, this looks work well. Could you check it? If it's fine, I'll request reviews for them.
Flags: needinfo?(jschwartzentruber)
Yes, this does work now. Thanks!

Ctrl+a seems to just input 'a', and not select the contents of the input. Is this expected?
Flags: needinfo?(jschwartzentruber)
(In reply to Jesse Schwartzentruber (:truber) from comment #52)
> Yes, this does work now. Thanks!
> 
> Ctrl+a seems to just input 'a', and not select the contents of the input. Is
> this expected?

Hmm, no. I should do "Select All". I'll fix it before requesting review.
We need to port synthesizeKey() to FuzzingFunctions.  So, its helper code
should be in WidgetKeyboardEvent or TextInputProcesor and should be shared
by both synthesizeKey() and FuzzingFunctions to keep easier to maintain.

This patch moves non-printable key part of _guessCodeFromKeyName() into
WidgetKeyboardEvent and makes it accessible from EventUtils.js via
nsITextInputProcessor.
This implements a helper API to *guess* code value of a printable key with
assuming active keyboard layout is US-English.
This implement a helper API to guess keyCode value of a printable key with
assuming active keyboard layout is US-English.  The reason why this stops
computing key value from key value is, most users of such API probably want
to emulate input from US-English keyboard layout if they don't specify the
detail.  Therefore, the new API simply maps each ASCII character to a
DOM keyCode which is usually mapped in US-English keyboard layout.
Synthesizing keyboard events is dangerous and such API is requested only by
fuzzing test.  So, we should add it into FuzzingFunctions which is built
only when |ac_add_options --enable-fuzzing| is specified and enabled by
the pref.

This patch implements the API as synthesizing keyboard events in the focused
widget and the synthesized events are propagated as native key events except
APZ (because keyboard events are synthesized only in the process).  This
behavior allows to test including any default action handlers such as
EventStateManager and setting WidgetGUIEvent::mWidget since some C++ handler
checks if it's nullptr.
Comment on attachment 9008021 [details]
Bug 1343955 - part 4: Implement static API to synthesize keyboard events into FuzzingFunctions

Olli Pettay [:smaug] has approved the revision.
Attachment #9008021 - Flags: review+
browser_panel_keyboard_navigation.js sets "Tab" to first argument of
EventUtils.synthesizeKey().  However, this causes inputting "T", "a", "b" with
a key press.  The test intended to emulate pressing the "Tab" key.  So, it
should be "KEY_Tab".

Note that this will cause permanent orange with the following patches since
synthesizeKey() will detect this kind of mistake.
Attachment #9008018 - Attachment description: Bug 1343955 - part 1: Implement non-printable key part of _guessCodeFromKeyName() in EventUtils.js with C++ and make it accessible with nsITextInputProcessor for EventUtils.js → Bug 1343955 - part 1: Add automated tests for synthesizeKey() of EventUtils.js
Attachment #9008019 - Attachment description: Bug 1343955 - part 2: Implement printable key part of _guessCodeFromKeyName() in EventUtils.js with C++ and make it accessible with nsITextInputProcessor for EventUtils.js → Bug 1343955 - part 2: Implement _guessCodeFromKeyName() in EventUtils.js with C++ and make it accessible with nsITextInputProcessor for EventUtils.js
Comment on attachment 9013578 [details]
Bug 1343955 - part 0: Fix bug of browser_panel_keyboard_navigation.js

Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has approved the revision.
Attachment #9013578 - Flags: review+
Comment on attachment 9008018 [details]
Bug 1343955 - part 1: Add automated tests for synthesizeKey() of EventUtils.js

Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has approved the revision.
Attachment #9008018 - Flags: review+
Comment on attachment 9008020 [details]
Bug 1343955 - part 3: Implement similar C++ utility of _computeKeyCodeFromChar() in EventUtils.js and make it accssible with nsITextInputProcessor for EventUtils.js

Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has approved the revision.
Attachment #9008020 - Flags: review+
Comment on attachment 9008019 [details]
Bug 1343955 - part 2: Implement _guessCodeFromKeyName() in EventUtils.js with C++ and make it accessible with nsITextInputProcessor for EventUtils.js

Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has approved the revision.
Attachment #9008019 - Flags: review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f54d08819067
part 0: Fix bug of browser_panel_keyboard_navigation.js r=smaug
https://hg.mozilla.org/integration/autoland/rev/1a1ef374eba6
part 1: Add automated tests for synthesizeKey() of EventUtils.js r=smaug
https://hg.mozilla.org/integration/autoland/rev/fea8e039767a
part 2: Implement _guessCodeFromKeyName() in EventUtils.js with C++ and make it accessible with nsITextInputProcessor for EventUtils.js r=smaug
https://hg.mozilla.org/integration/autoland/rev/4a3ba199155a
part 3: Implement similar C++ utility of _computeKeyCodeFromChar() in EventUtils.js and make it accssible with nsITextInputProcessor for EventUtils.js r=smaug
https://hg.mozilla.org/integration/autoland/rev/ade489313a3d
part 4: Implement static API to synthesize keyboard events into FuzzingFunctions r=smaug
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: