511615 - (CVE-2009-3370) Satchel should ignore untrusted events

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Paul Stone]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)

A web page can trigger the autocomplete popup on a text field, and then generate 'cursor down' events to select a particular item from the drop down. When the user presses the left or right arrow keys, the textbox value will be set to the currently selected item. The web page can then grab the value.

If a web page can convince a user to hold down or repeatedly press the left or right arrow keys, it can systematically grab all the data. I don't think this can be completely automated, since only user-generated left, right or enter key events cause the textbox to be filled.

Depending on the 'name' attribute of the textbox, different sets of data can be grabbed. e.g. 'searchbar-history' will give the autocomplete data from the Firefox search bar. 'q' will give Google search queries. This could also be used to get usernames, email addresses and even credit card numbers (I have seen a number of shopping sites that don't set autocomplete="off" on their payment pages)

Reproducible: Always

Steps to Reproduce:
See testcase (tested on Windows XP only)
Actual Results:  
Synthesized key events can manipulate the autocomplete popup.

Expected Results:  
Only user generated key events should trigger or affect autocomplete.

Comment 1

[Paul Stone]

Attachment: Testcase

This has only been tested on Windows XP. It works more reliably if Firebug is not enabled for the page.

Comment 2

[Paul Stone]

If you opened the testcase in a backgroud tab, you may have to refresh it to make it work correctly (due to focus issues)

Comment 3

[Paul Stone]

Attachment: Testcase (for Firefox trunk with 'awesomecomplete')

The Firefox trunk has the new 'awesomecomplete' behaviour and makes this testcase work a little more efficiently. By typing a single 'space' character, the full autocomplete history is shown.

For Firefox 3.5, a synthesized 'down key' or mouse click didn't trigger the full popup, but a single character does (e.g 'a' will show all entries starting with 'a'). So the first testcase has to loop through a-z0-9 to get the full results, which takes a bit longer.

Comment 4

[Brandon Sterne (:bsterne)]

Confirming.  Setting to sg:moderate because user interaction is required.

Comment 5

[Jesse Ruderman]

We should only show autocomplete in response to user-generated events.  But see also bug 381681 :(

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This should fix the problem; I've added a check to all the event listener entry points to ignore untrusted events. The testcase in this bug no longer works, although I'd like to do a bit more testing and write a Mochitest for this.

Most of the places were we were accepting trusted events were just stubs or otherwise not very interesting, but it seemed safer to just close off everything than try to figure out what events and (combinations of events) would be safe.

Comment 7

[Justin Dolske [:Dolske]]

Moving to Form Mananger, since the generic autocomplete code doesn't seem to be involved (no calls to addEventListener).

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 397395 [details] [diff] [review]
Patch v.1

I dunno if a testcase is worthwhile here... There's already test coverage to make sure we don't break normal usage. Testing this bug's fix, specifically, doesn't seem worthwhile because it requires a mix of privileged and unprivileged code, and I'm not very confident that it would reliably catch some future regressions of this type.

Comment 9

[Justin Dolske [:Dolske]]

One further comment: The bug here is that unprivileged events were able to select arbitrary items from the autocomplete popup. The popup still seems to require a trusted event (ie, real user interaction) to actually select one of the items and fill it into the <input>. [I'm not sure what code is specifically enforcing that, though. The obvious places where arrows/enter trigger something happening don't seem to be involved with selecting an entry.]

That seems to be why the original testcase/POC requires you to "hold down the right-arrow key", as it's that event that activates the selected entry. The testcase doesn't work if I modify it to generate fake right-arrow keypress events.

Comment 10

[Justin Dolske [:Dolske]]

Bah, this was nagging at me so I wrote a test today. Mostly done, will attach later.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Now with tests!

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 398471 [details] [diff] [review]
Patch v.2

>+    // try a keypress
>+    case 14:
>+        doKeyUnprivledged('v');
>+        nextTestPrivledged = true;
>+        break;
>+    case 15:
>+        // XXX even with patch the popup is still opened.
>+        // the keypress triggers a form input oninput / onchange
>+        todo(false, "popup should be closed");
>+        checkPopupOpen(true, -1);

This ends up looking complicated to fix... I tried moving the code in nsFormFillController::Input to ::KeyPress, but found that event fires before the form input's value has been updated. Making the satchel/autocomplete code deal with things in that state might be possible, but would be a fragile hack. That would be less ugly than trying to make the input event untrusted in some cases, though.

It would be nice to seal this up tighter, but I don't think it's required to fix the issue in this bug.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 398471 [details] [diff] [review]
Patch v.2

belt AND suspenders?

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 398471 [details] [diff] [review]
Patch v.2

unfortunately how to abuse this is pretty obvious from the patch -- we'll need to land everywhere in one bang, and want to catch the nearest available trains to release.

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz

Comment 15

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/909bac277b78
Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b9eaa02ac19d
Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b89c79449fb4

Paul Stone: Thanks for finding this!

Comment 16

[Justin Dolske [:Dolske]]

Pushed test fix to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/15e78acc5aae

These two subtests send spaces to the input, which is handled differently on 1.9.2 and trunk due to the landing of form awesomecomplete.

Comment 17

[Justin Dolske [:Dolske]]

Pushed to 190 (patch + testfix):

Checking in toolkit/components/satchel/src/nsFormFillController.cpp;
  new revision: 1.102; previous revision: 1.101
Checking in toolkit/components/satchel/src/nsFormFillController.h;
  new revision: 1.19; previous revision: 1.18
Checking in toolkit/components/satchel/test/Makefile.in;
  new revision: 1.3; previous revision: 1.2
Checking in toolkit/components/satchel/test/satchel_common.js;
  new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/satchel/test/test_bug_511615.html;
  initial revision: 1.1

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0 using test_bug_511615.html.