Form Fill fills disabled and hidden form controls

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: sfraser_bugs, Assigned: alqahira)

Tracking

(Blocks: 2 bugs)

unspecified
PowerPC
Mac OS X
Dependency tree / graph
Bug Flags:
camino1.5 -
camino1.5.1 -
camino2.1.1 +

Details

(Whiteboard: [camino-2.1.1])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
Created attachment 251456 [details]
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.
We argued about this in bug 344356.
Flags: camino1.1?
Er, I guess we argued about this for password autofill, not regular form fill.

Comment 3

12 years ago
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.
(Reporter)

Comment 4

12 years ago
(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).
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"?
Flags: camino1.5?
Flags: camino1.5.1?
Flags: camino1.5-
Assignee: dveditz → nobody

Updated

12 years ago
Assignee: nobody → stuart.morgan
Target Milestone: --- → Camino1.6
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*.
Target Milestone: Camino1.6 → ---
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.
Flags: camino1.5.1? → camino1.5.1-

Comment 8

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

Updated

12 years ago
Assignee: stuart.morgan → dveditz

Updated

12 years ago
Assignee: dveditz → nobody
(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.
Blocks: 187454, 319792
Version: 1.8 Branch → unspecified
(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
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.
Created attachment 569777 [details] [diff] [review]
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.
Assignee: nobody → alqahira
Status: NEW → ASSIGNED
Attachment #569777 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 13

7 years ago
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 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. ;)
Attachment #569777 - Flags: superreview?(stuart.morgan+bugzilla)
Created attachment 570348 [details]
"What do we fill?" testcase

Yes, we do fill <select>s, with some difficulty :P
Created attachment 571134 [details]
Testcase from smfr, with <select>s
Attachment #251456 - Attachment is obsolete: true
Created attachment 571247 [details] [diff] [review]
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?
Attachment #569777 - Attachment is obsolete: true
Attachment #571247 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 18

7 years ago
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).
Attachment #571247 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
Created attachment 581549 [details] [diff] [review]
Stop filling in disabled form fields, v2.2

Like this, then?
Attachment #571247 - Attachment is obsolete: true
Attachment #581549 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 20

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

Yep, just like that :) sr=smorgan
Attachment #581549 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: camino2.1.1? → camino2.1.1+
Resolution: --- → FIXED
Whiteboard: [camino-2.1.1]
You need to log in before you can comment on or make changes to this bug.