[Autofill] Sanitize all data that comes in over IPC.

[Autofill] Sanitize all data that comes in over IPC.

BUG=329978
TEST=none
R=palmer@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243833

[Ilya Sherman, 2013-12-20 06:45:13]

Hopefully this is along the lines of what you were thinking off.  It seems like
parts of this -- such as sanitizing GURLs -- ought to be redundant with existing
code.  Do you know whether that's so; and if so, where the code I should be
re-using is?

[palmer, 2013-12-20 19:53:36]

Thank you for doing this!

LGTM with some nits and ramblings.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager.cc:341: // Bail if the
arguments appear to be corrupt.
Nit: I'd drop the instances of this comment. The function names say it all.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
File components/autofill/core/common/autofill_data_sanitizer.cc (right):

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.cc:14: // Constants to
enforce data size caps, so as to avoid sending overly large
Nit: Don't need to repeat these comments from the .h file.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.cc:18: const size_t
kMaxDataLength = 1024;
If you find in the future that you need to increase this limit (or that of
|kMaxListSize|), that is OK. FWIW.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.cc:23: bool
IsSanitizedString(const std::string& str) {
FYI: "Sanitized" often means *transformed* in some way (e.g. POSIX shell
metacharacters escaped, or the like). Here, you are simply asserting some notion
of validity, without doing any transformation. I might therefore call these
functions isFooValid(const Foo& foo).

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.cc:32: return
url.is_valid() && url.spec().size() <= kMaxDataLength;
By doing this, you are in effect asserting a limit on GURLs distinct from any
internal limits GURL may assert (a very quick skim suggests it's not asserting
any limits at the moment, but perhaps it could someday).

That is OK, since the limit you set is (presumably) reasonable for Autofill. But
something to keep in mind.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
File components/autofill/core/common/autofill_data_sanitizer.h (right):

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.h:21: // Constants to
enforce data size caps, so as to avoid sending overly large
As used, I think these are more to immediately reject overlarge messages once
sent. Although if you (want to) use them for both purposes, the comment could
say both.

I come from the anti-comment camp, though, so do what you like. :)

[Ilya Sherman, 2013-12-20 23:54:51]

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager.cc:341: // Bail if the
arguments appear to be corrupt.
On 2013/12/20 19:53:36, Chromium Palmer wrote:
> Nit: I'd drop the instances of this comment. The function names say it all.

Done.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
File components/autofill/core/common/autofill_data_sanitizer.cc (right):

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.cc:14: // Constants to
enforce data size caps, so as to avoid sending overly large
On 2013/12/20 19:53:36, Chromium Palmer wrote:
> Nit: Don't need to repeat these comments from the .h file.

Whoops, copy/pasta.  Done.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.cc:18: const size_t
kMaxDataLength = 1024;
On 2013/12/20 19:53:36, Chromium Palmer wrote:
> If you find in the future that you need to increase this limit (or that of
> |kMaxListSize|), that is OK. FWIW.

Ack.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.cc:23: bool
IsSanitizedString(const std::string& str) {
On 2013/12/20 19:53:36, Chromium Palmer wrote:
> FYI: "Sanitized" often means *transformed* in some way (e.g. POSIX shell
> metacharacters escaped, or the like). Here, you are simply asserting some
notion
> of validity, without doing any transformation. I might therefore call these
> functions isFooValid(const Foo& foo).

Done.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.cc:32: return
url.is_valid() && url.spec().size() <= kMaxDataLength;
On 2013/12/20 19:53:36, Chromium Palmer wrote:
> By doing this, you are in effect asserting a limit on GURLs distinct from any
> internal limits GURL may assert (a very quick skim suggests it's not asserting
> any limits at the moment, but perhaps it could someday).
> 
> That is OK, since the limit you set is (presumably) reasonable for Autofill.
But
> something to keep in mind.

Ack.

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
File components/autofill/core/common/autofill_data_sanitizer.h (right):

https://codereview.chromium.org/98753005/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_data_sanitizer.h:21: // Constants to
enforce data size caps, so as to avoid sending overly large
On 2013/12/20 19:53:36, Chromium Palmer wrote:
> As used, I think these are more to immediately reject overlarge messages once
> sent. Although if you (want to) use them for both purposes, the comment could
> say both.
> 
> I come from the anti-comment camp, though, so do what you like. :)

Done.

[commit-bot: I haz the power, 2013-12-20 23:58:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/98753005/60001

[commit-bot: I haz the power, 2013-12-20 23:58:45]

Failed to apply patch for components/autofill/core/browser/autofill_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/autofill/core/browser/autofill_manager.cc
  Hunk #9 FAILED at 629.
  Hunk #10 FAILED at 637.
  2 out of 10 hunks FAILED -- saving rejects to file
components/autofill/core/browser/autofill_manager.cc.rej

Patch:       components/autofill/core/browser/autofill_manager.cc
Index: components/autofill/core/browser/autofill_manager.cc
diff --git a/components/autofill/core/browser/autofill_manager.cc
b/components/autofill/core/browser/autofill_manager.cc
index
b780de8caceffd1700b55c08cc61196f365f5c1e..5aab9dc63be56d128b4c9ca763a4aae22e0aed7e
100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -35,6 +35,7 @@
 #include "components/autofill/core/browser/personal_data_manager.h"
 #include "components/autofill/core/browser/phone_number.h"
 #include "components/autofill/core/browser/phone_number_i18n.h"
+#include "components/autofill/core/common/autofill_data_validation.h"
 #include "components/autofill/core/common/autofill_pref_names.h"
 #include "components/autofill/core/common/autofill_switches.h"
 #include "components/autofill/core/common/form_data.h"
@@ -230,8 +231,15 @@ void
AutofillManager::SetExternalDelegate(AutofillExternalDelegate* delegate) {
   autocomplete_history_manager_->SetExternalDelegate(delegate);
 }
 
+void AutofillManager::ShowAutofillSettings() {
+  manager_delegate_->ShowAutofillSettings();
+}
+
 bool AutofillManager::OnFormSubmitted(const FormData& form,
                                       const TimeTicks& timestamp) {
+  if (!IsValidFormData(form))
+    return false;
+
   // Let Autocomplete know as well.
   autocomplete_history_manager_->OnFormSubmitted(form);
 
@@ -301,6 +309,9 @@ bool AutofillManager::OnFormSubmitted(const FormData& form,
 void AutofillManager::OnFormsSeen(const std::vector<FormData>& forms,
                                   const TimeTicks& timestamp,
                                   autofill::FormsSeenState state) {
+  if (!IsValidFormDataVector(forms))
+    return;
+
   bool is_post_document_load = state == autofill::DYNAMIC_FORMS_SEEN;
   // If new forms were added dynamically, treat as a new page.
   if (is_post_document_load)
@@ -325,6 +336,9 @@ void AutofillManager::OnFormsSeen(const
std::vector<FormData>& forms,
 void AutofillManager::OnTextFieldDidChange(const FormData& form,
                                            const FormFieldData& field,
                                            const TimeTicks& timestamp) {
+  if (!IsValidFormData(form) || !IsValidFormFieldData(field))
+    return;
+
   FormStructure* form_structure = NULL;
   AutofillField* autofill_field = NULL;
   if (!GetCachedFormAndField(form, field, &form_structure, &autofill_field))
@@ -355,6 +369,9 @@ void AutofillManager::OnQueryFormFieldAutofill(int query_id,
                                                const FormFieldData& field,
                                                const gfx::RectF& bounding_box,
                                                bool display_warning) {
+  if (!IsValidFormData(form) || !IsValidFormFieldData(field))
+    return;
+
   std::vector<base::string16> values;
   std::vector<base::string16> labels;
   std::vector<base::string16> icons;
@@ -442,6 +459,9 @@ void AutofillManager::OnFillAutofillFormData(int query_id,
                                              const FormData& form,
                                              const FormFieldData& field,
                                              int unique_id) {
+  if (!IsValidFormData(form) || !IsValidFormFieldData(field))
+    return;
+
   const AutofillDataModel* data_model = NULL;
   size_t variant = 0;
   FormStructure* form_structure = NULL;
@@ -522,10 +542,6 @@ void AutofillManager::OnFillAutofillFormData(int query_id,
   driver_->SendFormDataToRenderer(query_id, result);
 }
 
-void AutofillManager::OnShowAutofillDialog() {
-  manager_delegate_->ShowAutofillSettings();
-}
-
 void AutofillManager::OnDidPreviewAutofillFormData() {
   if (test_delegate_)
     test_delegate_->DidPreviewFormData();
@@ -599,9 +615,13 @@ void AutofillManager::SetTestDelegate(
 }
 
 void AutofillManager::OnAddPasswordFormMapping(
-      const FormFieldData& form,
+      const FormFieldData& username_field,
       const PasswordFormFillData& fill_data) {
-  external_delegate_->AddPasswordFormMapping(form, fill_data);
+  if (!IsValidFormFieldData(username_field) ||
+      !IsValidPasswordFormFillData(fill_data))
+    return;
+
+  external_delegate_->AddPasswordFormMapping(username_field, fill_data);
 }
 
 void AutofillManager::OnShowPasswordSuggestions(
@@ -609,6 +629,11 @@ void AutofillManager::OnShowPasswordSuggestions(
     const gfx::RectF& bounds,
     const std::vector<base::string16>& suggestions,
     const std::vector<base::string16>& realms) {
+  if (!IsValidString16Vector(suggestions) ||
+      !IsValidString16Vector(realms) ||
+      suggestions.size() != realms.size())
+    return;
+
   external_delegate_->OnShowPasswordSuggestions(suggestions,
                                                 realms,
                                                 field,
@@ -617,7 +642,9 @@ void AutofillManager::OnShowPasswordSuggestions(
 
 void AutofillManager::OnSetDataList(const std::vector<base::string16>& values,
                                     const std::vector<base::string16>& labels)
{
-  if (values.size() != labels.size())
+  if (!IsValidString16Vector(values) ||
+      !IsValidString16Vector(labels) ||
+      values.size() != labels.size())
     return;
 
   external_delegate_->SetCurrentDataListValues(values, labels);

[commit-bot: I haz the power, 2013-12-27 00:21:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/98753005/100001

[commit-bot: I haz the power, 2013-12-27 01:02:17]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-27 01:49:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/98753005/140002

[commit-bot: I haz the power, 2013-12-27 03:48:43]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-09 02:04:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/98753005/520001

[commit-bot: I haz the power, 2014-01-09 05:21:19]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-09 07:57:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/98753005/520001

[commit-bot: I haz the power, 2014-01-09 10:35:24]

Change committed as 243833