Last Comment Bug 270697 - (autocompleteLeak) Autocomplete data leak
(autocompleteLeak)
: Autocomplete data leak
Status: VERIFIED FIXED
[sg:fix] have fix need SR ben, approval
: fixed-aviary1.0.1, privacy
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: 1.0 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
: David P James
:
Mentors:
Depends on: 286933 381681
Blocks: sg-ff101
  Show dependency treegraph
 
Reported: 2004-11-18 15:13 PST by Daniel Veditz [:dveditz]
Modified: 2007-05-23 15:45 PDT (History)
15 users (show)
caillon: blocking‑aviary1.0.1+
caillon: blocking‑aviary1.5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (3.03 KB, text/html)
2004-11-18 15:19 PST, Daniel Veditz [:dveditz]
no flags Details
Don't pre-fill when the user scrolls through the list with keys (2.60 KB, patch)
2005-02-08 10:28 PST, Christopher Aillon (sabbatical, not receiving bugmail)
mconnor: review+
jst: superreview+
dveditz: approval‑aviary1.0.1-
Details | Diff | Splinter Review
preserve URLbar behavior (10.77 KB, patch)
2005-02-08 14:31 PST, Daniel Veditz [:dveditz]
caillon: superreview-
Details | Diff | Splinter Review
preserve URLbar behavior v2 (10.86 KB, patch)
2005-02-09 17:06 PST, Daniel Veditz [:dveditz]
mconnor: review+
dbaron: approval‑aviary1.0.1+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2004-11-18 15:13:32 PST
From Matt Brubeck:

Mozilla Firefox Test Case: Form Auto-completion Privacy Leak
Introduction

This test case demonstrates a way that a malicious web page can obtain data from
a user's form-fill history. This attack requires the victim to scroll through
the form auto-complete menu using the keyboard.
Test Case Instructions

   1.

      First, enter a value into the "Add data" field in Form 1. Press the submit
button. Enter and submit several more values, all beginning with the same
character (for example: "101", "184", "190").
   2.

      Type same first character into the "Test" field in Form 2. The form
auto-complete menu will appear, populated with the values entered in the
previous step.
   3.

      Press the "down arrow" key to scroll through the auto-complete menu. As
you scroll, the values you scroll past will be added to the field labeled
"Leak", and all of them will be submitted if you submit the form. (A malicious
web page would make the "Leak" field invisible, or send the captured data some
other way.)

Comments

A malicious web page could target a common field name (like "SSN"), or a field
name from a particular web site. Tricks could be used to make it more likely for
victims to scroll through the history: For example, the malicious page could ask
the user to submit a form twice with the same value, which would would appear in
the formfill history the second time. The requested value could be chosen to
appear below likely values for sensitive data.

This bug could be fixed by disabling all event handlers and timer functions
while the form-fill menu is open. Alternately, the "value" attribute of an input
field should not be updated while the user is scrolling through the form-fill menu.

Test case tested and found to work on:

    * Mozilla Firefox nightly build 20041107 (branch, Windows installer)

Not yet tested on:

    * Mozilla Seamonkey
    * Other Firefox builds or platforms
    * Other clients

[dveditz note: Seamonkey is not vulnerable, it fills forms in toto only on
request and does not have an active auto-complete feature]
Comment 1 Daniel Veditz [:dveditz] 2004-11-18 15:19:03 PST
Created attachment 166384 [details]
testcase
Comment 2 Daniel Veditz [:dveditz] 2004-11-18 15:30:44 PST
In e-mail discussion Mike Shaver notes:
> If I skip [step 2] (don't type a letter, just press Down), the attack
> fails, likely because the value isn't put into the text field until it's
> selected.  That sounds like we could fix the problem pretty easily by
> just not pre-filling the field in the complete case, until the entry is
> selected.

For comparison: IE's autocomplete does not move values into the field until the
user hits enter to select it. Sounds like this would be the consensus behavior
to fix this privacy problem.
Comment 3 Christopher Aillon (sabbatical, not receiving bugmail) 2005-02-02 12:31:51 PST
we should try and fix this as per comment 2.
Comment 4 David Baron :dbaron: ⌚️UTC-7 2005-02-07 13:13:09 PST
... which is probably pretty easy, since it's the behavior that we already use
when there are no characters typed.
Comment 5 David Baron :dbaron: ⌚️UTC-7 2005-02-07 13:29:42 PST
Actually, that's not true on the trunk anymore, and I'm not sure why.

But either changing nsAutoCompleteController::CompleteValue or changing when
it's called is probably the solution.
Comment 6 Christopher Aillon (sabbatical, not receiving bugmail) 2005-02-08 10:28:42 PST
Created attachment 173766 [details] [diff] [review]
Don't pre-fill when the user scrolls through the list with keys

Looks like we just need to not change the field value for up/down key events. 
I've tested this and it does indeed fix the problem.
Comment 7 Mike Connor [:mconnor] 2005-02-08 10:37:25 PST
Comment on attachment 173766 [details] [diff] [review]
Don't pre-fill when the user scrolls through the list with keys

r=mconnor@steelgryphon.com
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-08 11:07:05 PST
Comment on attachment 173766 [details] [diff] [review]
Don't pre-fill when the user scrolls through the list with keys

sr=jst, but it might be worth adding a comment there to indicate to developers
that adding this code back is not a goode idea.
Comment 9 Christopher Aillon (sabbatical, not receiving bugmail) 2005-02-08 11:32:57 PST
Oh, fwiw, I added this comment:

      // Note: Do not fill in the text area here.  That happens when 
      // the user selects it.  This holds parity with IE and fixes
      // bug 270697.
Comment 10 Daniel Veditz [:dveditz] 2005-02-08 14:26:00 PST
Arg, now I remember this bug. I wrote a patch right before the holidays and
promptly forgot about it. If you just strip out the code you reference then
autocomplete for various UI bits (e.g. URL bar, search bar) doesn't work the way
people want it to work.
Comment 11 Daniel Veditz [:dveditz] 2005-02-08 14:31:10 PST
Created attachment 173794 [details] [diff] [review]
preserve URLbar behavior
Comment 12 Daniel Veditz [:dveditz] 2005-02-09 13:49:23 PST
Comment on attachment 173766 [details] [diff] [review]
Don't pre-fill when the user scrolls through the list with keys

I think we need to preserve the old urlbar behavior
Comment 13 Christopher Aillon (sabbatical, not receiving bugmail) 2005-02-09 15:49:53 PST
Comment on attachment 173794 [details] [diff] [review]
preserve URLbar behavior

>Index: browser/base/content/browser.xul

>         <textbox id="urlbar" flex="1"
>                  type="autocomplete"
>                  autocompletesearch="history" 
>                  autocompletepopup="PopupAutoComplete"
>+                 completeSelectedIndex="true"


<snip>


>Index: toolkit/content/widgets/autocomplete.xml
>+      <property name="completeSelectedIndex"
>+                onset="this.setAttribute('completeselectedindex', val); return val;"
>+                onget="return this.getAttribute('completeselectedindex') == 'true';"/>
>+


This won't actually work will it?  Since the attribute specified in the browser
XUL is camel case and the getters/setters are using lowercase.

Anyway, I'd prefer it get prefixed with 'auto'.  i.e.
'autocompleteselectedindex' or 'autocompleteselection' perhaps?  (i slightly
favor the latter, but your call).
Comment 14 Daniel Veditz [:dveditz] 2005-02-09 17:06:48 PST
Created attachment 173914 [details] [diff] [review]
preserve URLbar behavior v2

You're right about the case, this patch fixes it. The attribute name matches
the existing completeDefaultIndex so I'll stick with it unless a toolkit guy
wants to rename both.
Comment 15 Christopher Aillon (sabbatical, not receiving bugmail) 2005-02-09 22:01:25 PST
Comment on attachment 173914 [details] [diff] [review]
preserve URLbar behavior v2

I don't like the name of completedefaultindex either, since it doesn't match
the 'auto' prefix, but fair enough. 

>+      // if the completeSelectedIndex attribute is set

Nit: Not entirely true.  It can be set to false, and the code won't get run....

>+      PRBool completeSelection;
>+      mInput->GetCompleteSelectedIndex(&completeSelection);
>+      if (completeSelection)

r=caillon
Comment 16 Mike Connor [:mconnor] 2005-02-10 07:13:36 PST
Comment on attachment 173914 [details] [diff] [review]
preserve URLbar behavior v2

r=mconnor
Comment 17 Daniel Veditz [:dveditz] 2005-02-17 18:05:44 PST
fix checked into aviary 1.0.1 branch
Comment 18 Daniel Veditz [:dveditz] 2005-02-18 04:40:49 PST
Fixed on trunk
Comment 19 sairuh (rarely reading bugmail) 2005-02-18 17:51:24 PST
tested on mac 10.3.8 using 2005021806-1.0.1 firefox bits. when going thru the
attached test case, I saw the following appear in the Leak textfield as I used
the down arrow to scroll through the items in the Test fields autocomplete menu:

::1:1:1

is this correct or buggy behavior?
Comment 20 Matt Brubeck (:mbrubeck) 2005-02-18 18:02:18 PST
(In reply to comment #19)

That is correct (non-buggy) behavior.
Comment 21 sairuh (rarely reading bugmail) 2005-02-18 18:05:10 PST
excellent! (btw, also see the same result using the same build on Windows XP.)

marking verified.
Comment 22 Daniel Veditz [:dveditz] 2005-02-24 17:43:42 PST
This fix did not cause, but may have greatly exacerbated, crash bug 280084

Note You need to log in before you can comment on or make changes to this bug.