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

Issue 827945 link

Starred by 0 users
Status: Fixed
Owner:
█ vabr@chromium.org
hobby only
Closed: Apr 2018
Cc:
dvadym@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , iOS , Mac
Pri: 3
Type: Task



Sign in to add a comment

Fuzz password manager form parsing on iOS

Project Member Reported by vabr@chromium.org, Apr 2 2018 
Password manager code needs to transform (parse) HTML representation of forms into structured data (struct PasswordForm). This would benefit from fuzzing, because the HTML data consist from many free-form strings coming from outside Chrome. Adding a fuzzer is tracked in this bug.

Form parsing on iOS is done by FormParser (currently in ios/chrome/browser/passwords/). This functionality is nicely isolated from unnecessary dependencies and so it is easy to set up a fuzzer for it.

(On other platforms parsing is for now done in components/autofill/content/renderer/password_form_conversion_utils.*, which are hard to decouple from dependencies and hence hard to fuzz. However, there are plans to switch follow the iOS approach on other platforms as well, at which point the fuzzing  added here for iOS will be easy to reuse.)

There is a caveat: fuzzers are only available on Linux and Mac [1]. To be able to fuzz FormParser, it will need to be additionally compiled on those platforms. This is no problem thanks to its reduced dependencies, but its location will need to change from //ios to //components/password_manager.

The rough plan is:
(1) Move FormParser to the component.
(2) Create the fuzzer and run it locally.
(3) If needed, fix issues found by the fuzzer.
(4) Land the fuzzer.

Adding dvadym@, the author of FormParser, in Cc, so that future review requests are not so surprising. :)


[1] https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/README.md

Comment 1 by vabr@chromium.org, Apr 2 2018

Cc: dvadym@chromium.org
> Adding dvadym@, the author of FormParser, in Cc, so that future review requests are not so surprising. :)

And of course I forgot to extend the Cc, so doing now. Vadym, please feel free to read the issue description on crbug.com for more context.

Comment 2 by vabr@chromium.org, Apr 2 2018 

Turns out that generating FormData from an arbitrary byte string is interesting. https://docs.google.com/document/d/1SycfoqYdWV95C0KepB3luLk7TI1c-uQv0O0czYULP6o/edit?usp=sharing is a design in progress, accessible to all @chromium.org and @google.com accounts.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 3 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25ce5ac58661c27112d4a341ee56d93989951f21

commit 25ce5ac58661c27112d4a341ee56d93989951f21
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Apr 03 08:44:50 2018

Move FormParser to components

FormParser provides parsing PasswordForm structures from HTML data. It
currently is only used on iOS, and hence resides inside //ios. However,
in order to fuzz it, it has to be compiled on Linux and/or Mac.
Therefore the file itself is being moved to the component.

A side effect of this change is:
 * Dropping iOS PlatformTest fixture, because the test does not need it.
 (The fixture was responsible for draining the autorelease pool, but the
 test is pure C++, no Objective C).
 * Renaming to IOSFormParser, to ensure that iOS is still mentioned in
 the file path. It will also help to keep two different form parsers
 once the FormParser for desktop is added into the new target directory.

Bug:  827945 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I114e01af40d5d29f221e15e44f0fdf635ea1c8b4
Reviewed-on: https://chromium-review.googlesource.com/989952
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547650}
[modify] https://crrev.com/25ce5ac58661c27112d4a341ee56d93989951f21/components/password_manager/core/browser/BUILD.gn
[add] https://crrev.com/25ce5ac58661c27112d4a341ee56d93989951f21/components/password_manager/core/browser/form_parsing/BUILD.gn
[rename] https://crrev.com/25ce5ac58661c27112d4a341ee56d93989951f21/components/password_manager/core/browser/form_parsing/ios_form_parser.cc
[rename] https://crrev.com/25ce5ac58661c27112d4a341ee56d93989951f21/components/password_manager/core/browser/form_parsing/ios_form_parser.h
[rename] https://crrev.com/25ce5ac58661c27112d4a341ee56d93989951f21/components/password_manager/core/browser/form_parsing/ios_form_parser_unittest.cc
[modify] https://crrev.com/25ce5ac58661c27112d4a341ee56d93989951f21/ios/chrome/browser/passwords/BUILD.gn
[modify] https://crrev.com/25ce5ac58661c27112d4a341ee56d93989951f21/ios/chrome/browser/passwords/password_controller.mm
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 6 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8478a8f1f1ba6267f392f725a7a921587b7fb243

commit 8478a8f1f1ba6267f392f725a7a921587b7fb243
Author: Vaclav Brozek <vabr@chromium.org>
Date: Fri Apr 06 11:27:45 2018

Turn IOSFormParser into a static function

IOSFormParser is a class used on iOS to turn a HTML description of
password forms into an internal PasswordForm structure. The class has
no data members and the only method, Parse(), is thus effectively
static.

It was created as a class for two reasons: (1) to allow mocking, and
(2) to enable holding some server-supplied state.

I chatted with dvadym@ and we agreed that once this actually needs
being mocked, we can assess the concrete needs, and perhaps change
some alternative (e.g., providing a fake result for testing) to mocking
which would still require adding a virtual interface and other
boilerplate. Holding the server-supplied state might also likely
end up in PasswordFormManager.

Because it is confusing to create IOSFormParser instances in the
current code and the two concerns above seem addressed, this CL
turns IOSFormParser into a single static function, ParseFormData(),
following the existing pattern found in
components/password_manager/core/browser/import/csv_reader.h.

Bug:  827945 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Idbbd5087f301bb07974364f7b8bd324ed42a0fa5
Reviewed-on: https://chromium-review.googlesource.com/990333
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548740}
[modify] https://crrev.com/8478a8f1f1ba6267f392f725a7a921587b7fb243/components/password_manager/core/browser/form_parsing/ios_form_parser.cc
[modify] https://crrev.com/8478a8f1f1ba6267f392f725a7a921587b7fb243/components/password_manager/core/browser/form_parsing/ios_form_parser.h
[modify] https://crrev.com/8478a8f1f1ba6267f392f725a7a921587b7fb243/components/password_manager/core/browser/form_parsing/ios_form_parser_unittest.cc
[modify] https://crrev.com/8478a8f1f1ba6267f392f725a7a921587b7fb243/ios/chrome/browser/passwords/password_controller.mm
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 6 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36baddf27e21f4cc31aae61bffb22269b3787e5e

commit 36baddf27e21f4cc31aae61bffb22269b3787e5e
Author: Vaclav Brozek <vabr@chromium.org>
Date: Fri Apr 06 14:33:41 2018

Introduce DataAccessor (FormData parser fuzzing support)

This is part of the effort to introduce a fuzzer for code parsing
FormData into PasswordForms.

As described in the design linked from  https://crbug.com/827945#c2 , in
order to create FormData from an arbitrary input string supplied by the
fuzzer framework, a DataAccessor class should be created to wrap turning
the input string into bits, numbers and strings.

This CL adds DataAccessor, including tests (because the contained logic
is not straightforward).

Bug:  827945 
Change-Id: Ib7b2fe54d74bc096afa7e8bc8ff72ab68a8c5977
Reviewed-on: https://chromium-review.googlesource.com/992312
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548772}
[modify] https://crrev.com/36baddf27e21f4cc31aae61bffb22269b3787e5e/components/password_manager/core/browser/BUILD.gn
[add] https://crrev.com/36baddf27e21f4cc31aae61bffb22269b3787e5e/components/password_manager/core/browser/form_parsing/fuzzer/BUILD.gn
[add] https://crrev.com/36baddf27e21f4cc31aae61bffb22269b3787e5e/components/password_manager/core/browser/form_parsing/fuzzer/data_accessor.cc
[add] https://crrev.com/36baddf27e21f4cc31aae61bffb22269b3787e5e/components/password_manager/core/browser/form_parsing/fuzzer/data_accessor.h
[add] https://crrev.com/36baddf27e21f4cc31aae61bffb22269b3787e5e/components/password_manager/core/browser/form_parsing/fuzzer/data_accessor_unittest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 6 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1

commit f14109feb8f96c236dcf523b4177e2c9cbb0d3b1
Author: Vaclav Brozek <vabr@chromium.org>
Date: Fri Apr 06 15:38:48 2018

Introduce FormData parsing fuzzer

This CL adds a function to generate FormData based on a given
DataAccessor. The CL further uses this function to add a fuzzer for the
code parsing FormData into PasswordForms (on iOS, for now).

Bug:  827945 
Change-Id: I910a19b8e54b6383f91476c5c0fd698521d11eb2
Reviewed-on: https://chromium-review.googlesource.com/992496
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548783}
[modify] https://crrev.com/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1/components/password_manager/core/browser/form_parsing/BUILD.gn
[modify] https://crrev.com/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1/components/password_manager/core/browser/form_parsing/fuzzer/BUILD.gn
[modify] https://crrev.com/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1/components/password_manager/core/browser/form_parsing/fuzzer/data_accessor.cc
[modify] https://crrev.com/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1/components/password_manager/core/browser/form_parsing/fuzzer/data_accessor.h
[add] https://crrev.com/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1/components/password_manager/core/browser/form_parsing/fuzzer/form_data_producer.cc
[add] https://crrev.com/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1/components/password_manager/core/browser/form_parsing/fuzzer/form_data_producer.h
[add] https://crrev.com/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1/components/password_manager/core/browser/form_parsing/fuzzer/form_parser_fuzzer.cc
[add] https://crrev.com/f14109feb8f96c236dcf523b4177e2c9cbb0d3b1/components/password_manager/core/browser/form_parsing/fuzzer/form_parser_fuzzer.dict

Comment 8 by vabr@chromium.org, Apr 6 2018

Status: Fixed (was: Started)
The fuzzer is now checked in.
There is also bug 828705 to write an alternative version with libfuzzer-protobuf and keep the faster one, but that's beyond the scope of this issue, which was simply to fuzz parsing.

As the form parsing will get extended to cover platforms outside iOS, the fuzzer will get extended to match that. That's not being tracked here, but with the main work on parsing.
►

Sign in to add a comment