366974 - Form Fill fills disabled and hidden form controls

Status: RESOLVED FIXED

(Camino Graveyard :: Security, defect)

Description

[Simon Fraser [no longer active]]

Attachment: Testcase

Camino's Form Fill fills form controls which are disabled, and also those which have been hidden by style.

This is a serious security issue, as it could easily allow someone to write a page that obtains personal information about the user without the user's knowledge.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We argued about this in bug 344356.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Er, I guess we argued about this for password autofill, not regular form fill.

Comment 3

[Stuart Morgan]

Hidden by style is something that is going to be meaningless to fix without a substantial amount of difficult core support.  I brainstormed after all the myspace password hoopla, and came up with 10 or 20 ways to hide a field, many of which are very hard to detect and none of which are terribly difficult to implement on a page. Form fill is something the user has to explicitly trigger, which mitigates the security implications.

Disabled is easy though, and it's obviously wrong to fill a disabled field.

Comment 4

[Simon Fraser [no longer active]]

(In reply to comment #3)
> Hidden by style is something that is going to be meaningless to fix without a
> substantial amount of difficult core support.

FWIW, Safari fills visiblity:hidden fields too (I'll be filing a Radar).

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Not going to block 1.5 on this; I guess we should get a patch for disabled in 1.5.1, though?

Stuart, do you know if there are any bugs filed on any bits of the "substantial amount of difficult core support"?

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Disabled was spun off to bug 384635, which made 1.5.1.  Minusing this for 1.5.1; comment 6 still stands for the future.

Comment 8

[Stuart Morgan]

Well, disabled for password fill was spun off, but not address book fill, which is primarily what I think this is tracking now.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> (In reply to comment #3)
> > Hidden by style is something that is going to be meaningless to fix without a
> > substantial amount of difficult core support.
> 
> FWIW, Safari fills visiblity:hidden fields too (I'll be filing a Radar).

I guess no-one ever fixed smfr's radar (or that test uses one of Stuart's 10 or 20 other ways of hiding) :( http://jeremiahgrossman.blogspot.com/2010/09/safari-autofill-hack-lives.html

(In reply to comment #3)
> Form fill is something the user has to explicitly trigger,
> which mitigates the security implications.

Thankfully.  We need to fix this bug before we can do bug 187454 (and maybe bug 319792) safely, though.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #3)
> Disabled is easy though, and it's obviously wrong to fill a disabled field.

I guess we want something like the fix for bug 384635 to go about here: http://hg.mozilla.org/camino/file/tip/src/formfill/wallet.mm#l3017 (maybe?)?

Or maybe we should just rewrite Form Fill on top of the much saner password autofill and avoid dealing with 4K lines of cruft and C++ :P

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

From http://support.apple.com/kb/HT4808 ("About the security content of Safari 5.1 and Safari 5.0.6"):

Description: Safari's "AutoFill web forms" feature filled in non-visible form fields, and the information was accessible by scripts on the site before the user submitted the form. This issue is addressed by displaying all fields that will be filled, and requiring the user's consent before AutoFill information is available to the form.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Stop filling in disabled form fields

(In reply to comment #10)
> (In reply to comment #3)
> > Disabled is easy though, and it's obviously wrong to fill a disabled field.
> 
> I guess we want something like the fix for bug 384635 to go about here:
> http://hg.mozilla.org/camino/file/tip/src/formfill/wallet.mm#l3017 (maybe?)?

In fact, we needed exactly that code (plus a few antecedent lines) in exactly that spot :P  I'm not sure why exactly I did see that it fit right in before (I blame the 3 lines of context in the patch :P ;-) )

Stuart, our KeychainService code does a bit more rv checking in the ("shared") code that precedes this (q.v. http://mxr.mozilla.org/camino/source/camino/src/formfill/KeychainService.mm#1481-3, 1491-2); should we port those checks to wallet, too?

I still plan to spin the RC tonight, but if you think this is worthwhile to include, it's a lot more trivial to respin on 2.1 than it used to be on 2.0; otherwise, it can be a ride-along in case we need an RC2, or wait for 2.0.1.

We ought to spin off a placeholder bug for "hidden" fields for both form fill and password autofill; if we ever do figure out a way to prevent some of the cases, the code will likely be very similar.

Comment 13

[Stuart Morgan]

Comment on attachment 569777 [details] [diff] [review]
Stop filling in disabled form fields

Don't we also autofill select elements? This would break that.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 569777 [details] [diff] [review]
Stop filling in disabled form fields

(In reply to Stuart Morgan from comment #13)
> Comment on attachment 569777 [details] [diff] [review] [diff] [details] [review]
> Stop filling in disabled form fields
> 
> Don't we also autofill select elements? This would break that.

I'm not sure I've ever seen us to that successfully, but you're right; we'd need to check for more than just <input>s. ;)

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: "What do we fill?" testcase

Yes, we do fill <select>s, with some difficulty :P

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Testcase from smfr, with <select>s

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Stop filling in disabled form fields, v2.1

I had a few minutes this afternoon and hacked this version together; it appears to work.  I can't guarantee that it's correct, nor that it's the best way to do this (I did try some other ways that didn't work!), but filling in <select>s still works this time around ;)

(In reply to comment #12)
> Stuart, our KeychainService code does a bit more rv checking in the
> ("shared") code that precedes this (q.v.
> http://mxr.mozilla.org/camino/source/camino/src/formfill/KeychainService.
> mm#1481-3, 1491-2); should we port those checks to wallet, too?

This comment from before still applies; I have no idea in which situations we need to to that sort of stuff, and which sorts of things are cargo-culted.

Hmm; the whole "type" check thing is cargo-culted here, isn't it?

Comment 18

[Stuart Morgan]

Comment on attachment 571247 [details] [diff] [review]
Stop filling in disabled form fields, v2.1

Sorry, I should have thought about this more when I did the first review. You don't actually care what the type is (the wallet code handles all the type stuff), you just want to know if it's disabled, and if so filter it out. And HasAttribute is on nsIDOMElement, so you can just:
  do_QI to nsIDOMElement
  if that works, get the "disabled" attribute
You can eliminate the duplication, and the GetType stuff.

Also, I'd say we only want to do 'continue' if things succeed and we end up finding out if it's disabled, not if any of the steps fail along the way (since I'm not sure what failures might be benign).

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Stop filling in disabled form fields, v2.2

Like this, then?

Comment 20

[Stuart Morgan]

Comment on attachment 581549 [details] [diff] [review]
Stop filling in disabled form fields, v2.2

Yep, just like that :) sr=smorgan

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/9a0c2744e253

(In reply to comment #12)
> We ought to spin off a placeholder bug for "hidden" fields for both form
> fill and password autofill; if we ever do figure out a way to prevent some
> of the cases, the code will likely be very similar.

Filed bug 714959 to cover hidden.