New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Sep 2010
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security
M-7

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment
link

Issue 51727: autocomplete entries submitted by javascript should not be stored in db (similar to autofill bug 48225)

Reported by infe...@chromium.org, Aug 10 2010 Project Member

Issue description

subject says it all. we dont want auto submitted javascript to submit spam entries in a form field. that was fixed for autofill ( bug 48225 ), but i still see it in autocomplete entries.

While we do restrict no of entries per autocomplete field (http://jeremiahgrossman.blogspot.com/2010/07/in-firefox-we-cant-read-auto-complete.html), we shouldn't be storing any one at all since it can kick off legitimate entries.
 

Comment 1 by infe...@chromium.org, Aug 10 2010

Comment 2 by infe...@chromium.org, Aug 10 2010

Status: WillMerge
Needs to be merged to 472 and 375.

Comment 3 by bugdro...@gmail.com, Aug 10 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=55626 

------------------------------------------------------------------------
r55626 | inferno@chromium.org | 2010-08-10 15:39:30 -0700 (Tue, 10 Aug 2010) | 5 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/autocomplete_history_manager.cc?r1=55626&r2=55625

Not store autocomplete entries in DB for forms submitted using javascript.

BUG= 51727 

Review URL: http://codereview.chromium.org/3149003
------------------------------------------------------------------------

Comment 4 by lcam...@gmail.com, Aug 10 2010

Out of curiosity, do we care about auto-complete pollution on user-initiated submits?

(i.e., I can have a form with 1000 off-screen fields)

Comment 5 by infe...@chromium.org, Aug 10 2010

@lcamtuf: nothing serious in this one. just submitted patch for synchronous behavior with autofill. also, i remember if we have 1000 fields with same name on the same page (and we ask user to submit a button), we just store data from the first field. so, we pollute one entry in a particular field(name) at a time from user initiated behavior.

Comment 6 by infe...@chromium.org, Aug 10 2010

let me double check this.

Comment 7 by bugdro...@gmail.com, Aug 10 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=55632 

------------------------------------------------------------------------
r55632 | inferno@chromium.org | 2010-08-10 16:19:17 -0700 (Tue, 10 Aug 2010) | 5 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/autocomplete_history_manager_unittest.cc?r1=55632&r2=55631

Unittest fix. Need to put usersubmitted = true for unittests.

BUG= 51727 

Review URL: http://codereview.chromium.org/3151006
------------------------------------------------------------------------

Comment 8 by infe...@chromium.org, Aug 11 2010

@lcamtuf:: the situation is a little ugly here.

for autofill, there is no issue since we use only one field from form per name. so, when we persuade user to click the submit button (other fields can be hidden with css), only one spam profile gets created.

for autocomplete, situation is ugly. we can have multiple fields in the same form with one name and different ids. autocomplete history manager will store them seperately. So, effectively, we could achieve the same entry spamming by having multiple fields per form (with same name, different id) like 1000 and just ask user to click submit. we cap the no of the entries, but they can still be overwritten. this is a headache, but i dont see any fix possible for this.

Comment 9 by lcam...@gmail.com, Aug 11 2010

How about capping the number of text fields per page (or better, per origin) that we add to the autocomplete cache, with FIFO pruning or so?

Comment 10 by infe...@chromium.org, Aug 11 2010

autocomplete entries are not origin specific and are available for use on all sites, so we cannot use origin for tracking. we currently cap the no of entries available for a particular form field (name specific). lets brainstorm the capping of no of text fields per page. I need to think more on how this can be useful. From an attacker perspective, he or she might want to pollute data in generic fields like name, email, phone, so limiting the no of text fields per page (so something like 1000) will still be able to pollute data in those fields.

James, David - any thoughts. this will be m7, but good to get your thoughts whether this could be fixed or not. like autofill, i dont know if we can just store 1 form field per name (and ignore rest of entries with same name) without breaking any normal behavior.

Comment 11 by infe...@chromium.org, Aug 11 2010

Labels: Mstone-6
Status: Available
Just a fyi, my changes are merged to 472. So javascript spamming vector is gone. (Bugdroid looks down)

I had a chat with David on this and this is what i purpose to fix the user click on a button vector.

1) only use the first field for a particular name.
2) cap no of fields per form (have to be cautious on this number, i dont expect any real world site to have more than 256 form fields e.g.).

Comment 12 by dhollowa@chromium.org, Aug 11 2010

Status: Started

Comment 13 by dhollowa@chromium.org, Aug 11 2010

Status: Fixed
The user-click fix is in at http://src.chromium.org/viewvc/chrome?view=rev&revision=55781.  (Not sure why bugdroid is not updating this bug report...).

I'm not planning to merge this fix to 472, since it is M7.

Comment 14 by infe...@chromium.org, Aug 11 2010

Status: WillMerge
Fix is simple enough, will wait a week to bake on dev channel before merging to v6.

Comment 15 by bugdro...@gmail.com, Aug 12 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=55662 

------------------------------------------------------------------------
r55662 | inferno@chromium.org | 2010-08-10 20:31:01 -0700 (Tue, 10 Aug 2010) | 8 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/472/src/chrome/browser/autocomplete_history_manager_unittest.cc?r1=55662&r2=55661

Merge 55632 - Unittest fix. Need to put usersubmitted = true for unittests.

BUG= 51727 

Review URL: http://codereview.chromium.org/3151006

TBR=inferno@chromium.org
Review URL: http://codereview.chromium.org/3110007
------------------------------------------------------------------------

Comment 16 by bugdro...@gmail.com, Aug 12 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=55661 

------------------------------------------------------------------------
r55661 | inferno@chromium.org | 2010-08-10 20:29:40 -0700 (Tue, 10 Aug 2010) | 8 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/472/src/chrome/browser/autocomplete_history_manager.cc?r1=55661&r2=55660

Merge 55626 - Not store autocomplete entries in DB for forms submitted using javascript.

BUG= 51727 

Review URL: http://codereview.chromium.org/3149003

TBR=inferno@chromium.org
Review URL: http://codereview.chromium.org/3145007
------------------------------------------------------------------------

Comment 17 by bugdro...@gmail.com, Aug 12 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=55781 

------------------------------------------------------------------------
r55781 | dhollowa@chromium.org | 2010-08-11 14:03:29 -0700 (Wed, 11 Aug 2010) | 8 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/webdata/web_database.cc?r1=55781&r2=55780
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/webdata/web_database.h?r1=55781&r2=55780
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/webdata/web_database_unittest.cc?r1=55781&r2=55780

Autocomplete entries submitted are limited in number.

Limits the number of Autocomplete entries added to the WebDB, per form submission, to a maximum of 256.  If elements occur that have duplicate names, only the first occurrence is added.

BUG= 51727 
TEST=WebDatabaseTest.Autofill_AddFormFieldValues

Review URL: http://codereview.chromium.org/3143005
------------------------------------------------------------------------

Comment 18 by infe...@chromium.org, Aug 12 2010

Status: Fixed

Comment 19 by bugdro...@gmail.com, Aug 12 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=55894 

------------------------------------------------------------------------
r55894 | inferno@chromium.org | 2010-08-12 09:49:29 -0700 (Thu, 12 Aug 2010) | 11 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/472/src/chrome/browser/webdata/web_database.cc?r1=55894&r2=55893
   M http://src.chromium.org/viewvc/chrome/branches/472/src/chrome/browser/webdata/web_database.h?r1=55894&r2=55893
   M http://src.chromium.org/viewvc/chrome/branches/472/src/chrome/browser/webdata/web_database_unittest.cc?r1=55894&r2=55893

Merge 55781 - Autocomplete entries submitted are limited in number.

Limits the number of Autocomplete entries added to the WebDB, per form submission, to a maximum of 256.  If elements occur that have duplicate names, only the first occurrence is added.

BUG= 51727 
TEST=WebDatabaseTest.Autofill_AddFormFieldValues

Review URL: http://codereview.chromium.org/3143005

TBR=dhollowa@chromium.org
Review URL: http://codereview.chromium.org/3169008
------------------------------------------------------------------------

Comment 20 by infe...@chromium.org, Aug 12 2010

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 21 by dhollowa@chromium.org, Aug 17 2010

Comment 22 by bugdro...@gmail.com, Aug 18 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=56469 

------------------------------------------------------------------------
r56469 | rsimha@chromium.org | 2010-08-17 17:45:26 -0700 (Tue, 17 Aug 2010) | 15 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/live_sync/two_client_live_autofill_sync_test.cc?r1=56469&r2=56468
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/live_sync/two_client_live_preferences_sync_test.cc?r1=56469&r2=56468

Fix failing sync integration tests.

TwoClientLivePreferencesSyncTest.Security was failing due to a couple of
EXPECT_NEs that ought to have been EXPECT_EQs.

TwoClientLiveAutofillSyncTest.Client1HasData was failing due to a change
in autofill behavior introduced in r55781 to fix security bug  crbug.com/51727 .

This changelist also includes a clarification in the logic used by
TwoClientLiveAutofillSyncTest.BothHaveData.

BUG= 51727 , 51956 
TEST=sync_integration_tests

Review URL: http://codereview.chromium.org/3169019
------------------------------------------------------------------------

Comment 23 by bugdro...@gmail.com, Aug 31 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=58038 

------------------------------------------------------------------------
r58038 | inferno@chromium.org | 2010-08-31 12:16:51 -0700 (Tue, 31 Aug 2010) | 11 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/472/src/chrome/browser/autocomplete_history_manager.cc?r1=58038&r2=58037

Revert 55661 - Merge 55626 - Not store autocomplete entries in DB for forms submitted using javascript.

BUG= 51727 , 52940 

Review URL: http://codereview.chromium.org/3149003

TBR=inferno@chromium.org
Review URL: http://codereview.chromium.org/3145007

TBR=inferno@chromium.org
Review URL: http://codereview.chromium.org/3286006
------------------------------------------------------------------------

Comment 24 by bugdro...@gmail.com, Aug 31 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=58039 

------------------------------------------------------------------------
r58039 | inferno@chromium.org | 2010-08-31 12:18:11 -0700 (Tue, 31 Aug 2010) | 11 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/472/src/chrome/browser/autocomplete_history_manager_unittest.cc?r1=58039&r2=58038

Revert 55662 - Merge 55632 - Unittest fix. Need to put usersubmitted = true for unittests.

BUG= 51727 , 52940 

Review URL: http://codereview.chromium.org/3151006

TBR=inferno@chromium.org
Review URL: http://codereview.chromium.org/3110007

TBR=inferno@chromium.org
Review URL: http://codereview.chromium.org/3276006
------------------------------------------------------------------------

Comment 25 by infe...@chromium.org, Aug 31 2010

Labels: -Mstone-6 Mstone-7
Status: Assigned
Reopening pending the issue that came up in bug http://code.google.com/p/chromium/issues/detail?id=52940. Note that David's fix part is not reverted. Also, only changes to 472 are reverted. Trunk needed to be fixed.

Comment 26 by infe...@chromium.org, Sep 3 2010

Labels: Restrict-View-SecurityTeam

Comment 27 by scarybea...@gmail.com, Sep 9 2010

Labels: -Restrict-View-SecurityTeam

Comment 28 by dhollowa@chromium.org, Sep 9 2010

Status: Started

Comment 29 by dhollowa@chromium.org, Sep 10 2010

Status: Fixed
WebKit fix submitted.  Marking this bug fixed, pending WebKit CL landing and roll).

https://bugs.webkit.org/show_bug.cgi?id=45128

Comment 30 by infe...@chromium.org, Sep 17 2010

Labels: OS-All
Status: FixUnreleased

Comment 31 by scarybea...@gmail.com, Nov 3 2010

Labels: -Restrict-View-SecurityNotify
Status: Fixed

Comment 32 by jsc...@chromium.org, Mar 21 2011

Labels: Type-Security

Comment 33 by jsc...@chromium.org, Oct 5 2011

Labels: SecImpacts-Stable
Batch update.

Comment 34 by bugdroid1@chromium.org, Oct 13 2012

Project Member
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.

Comment 35 by bugdroid1@chromium.org, Mar 10 2013

Project Member
Labels: -Area-Internals -SecSeverity-Low -Mstone-7 -Type-Security -SecImpacts-Stable Security-Severity-Low Security-Impact-Stable M-7 Cr-Internals Type-Bug-Security

Comment 36 by bugdroid1@chromium.org, Mar 13 2013

Project Member
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Comment 37 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Severity-Low Security_Severity-Low

Comment 38 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Impact-Stable Security_Impact-Stable

Comment 39 by sheriffbot@chromium.org, Oct 1 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 40 by sheriffbot@chromium.org, Oct 2 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 41 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic

Sign in to add a comment