fuzzers for VerifyNameMatch and VerifyNameInSubtree

fuzzers for VerifyNameMatch and VerifyNameInSubtree

BUG=

Committed: https://crrev.com/afe43b899be59473af5620af17d5512555a9c7d6
Cr-Commit-Position: refs/heads/master@{#390467}

[mattm, 2016-04-28 03:43:44]

mattm@chromium.org changed reviewers:
+ eroman@chromium.org

[mattm, 2016-04-28 03:43:44]

Just the first thing I thought of for dividing the input data into two chunks,
let me know if you have better ideas. (Initially I was using 2 bytes for the
ratio, but the fuzzer seems to default to using a max size of 64 anyway so I
figured that was unnecessary.)

[mattm, 2016-04-28 16:06:40]

mattm@chromium.org changed reviewers:
+ mmoroz@chromium.org

[mattm, 2016-04-28 16:06:41]

+mmoroz Any thoughts on this method of breaking the input into two parts?

[eroman, 2016-04-28 18:27:10]

https://codereview.chromium.org/1929703003/diff/1/net/cert/internal/verify_na...
File net/cert/internal/verify_name_match_fuzzer.cc (right):

https://codereview.chromium.org/1929703003/diff/1/net/cert/internal/verify_na...
net/cert/internal/verify_name_match_fuzzer.cc:13: // Use the first byte of data
as a ratio to divide the rest of data into
The problem with using a 1 byte ratio like this is we won't be able to fuzz
every possible length.

Instead I suggest just using 2 bytes as first_part_size, and then the remaining
bytes for the second part.

We have //net specific helper routines for this. Some is not yet checked in, but
take a look at:
https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p...

With that paradigm you can write this as:

// Allow the first length to be up to 65535 bytes long (uses 2 bytes of fuzzer
input for this)
size_t first_part_size = fuzzed_data.ConsumeValueInRange(0, 0xFFFF);

base::StringPiece first_part = fuzzed_data.ConsumeBytes(first_part_size);
base::StringPiece second_part = fuzzed_data.ConsumeRemainingBytes();

ConsumeRemainingBytes() doesn't exist yet, but I suggest you add it to that
class -- should just be a 1 liner and is generally useful. (I know Chris has a
change that needs similar functionality.)

[mattm, 2016-04-28 18:46:03]

On 2016/04/28 18:27:10, eroman wrote:
>
https://codereview.chromium.org/1929703003/diff/1/net/cert/internal/verify_na...
> File net/cert/internal/verify_name_match_fuzzer.cc (right):
> 
>
https://codereview.chromium.org/1929703003/diff/1/net/cert/internal/verify_na...
> net/cert/internal/verify_name_match_fuzzer.cc:13: // Use the first byte of
data
> as a ratio to divide the rest of data into
> The problem with using a 1 byte ratio like this is we won't be able to fuzz
> every possible length.

So you think I should update the BUILD rule to specify a larger default max_len
value?
It would be cool if the test got the max_len value as an input so it could
choose an appropriate range to use..
 
> Instead I suggest just using 2 bytes as first_part_size, and then the
remaining
> bytes for the second part.
> 
> We have //net specific helper routines for this. Some is not yet checked in,
but
> take a look at:
>
https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p...
> 
> With that paradigm you can write this as:
> 
> // Allow the first length to be up to 65535 bytes long (uses 2 bytes of fuzzer
> input for this)
> size_t first_part_size = fuzzed_data.ConsumeValueInRange(0, 0xFFFF);
> 
> base::StringPiece first_part = fuzzed_data.ConsumeBytes(first_part_size);
> base::StringPiece second_part = fuzzed_data.ConsumeRemainingBytes();
> 
> ConsumeRemainingBytes() doesn't exist yet, but I suggest you add it to that
> class -- should just be a 1 liner and is generally useful. (I know Chris has a
> change that needs similar functionality.)

Cool, I'll update once that CL lands.

[eroman, 2016-04-28 19:02:34]

On Thu, Apr 28, 2016 at 11:46 AM, <mattm@chromium.org> wrote:

> On 2016/04/28 18:27:10, eroman wrote:
> >
>
>
https://codereview.chromium.org/1929703003/diff/1/net/cert/internal/verify_na...
> > File net/cert/internal/verify_name_match_fuzzer.cc (right):
> >
> >
>
>
https://codereview.chromium.org/1929703003/diff/1/net/cert/internal/verify_na...
> > net/cert/internal/verify_name_match_fuzzer.cc:13: // Use the first byte
> of
> data
> > as a ratio to divide the rest of data into
> > The problem with using a 1 byte ratio like this is we won't be able to
> fuzz
> > every possible length.
>
> So you think I should update the BUILD rule to specify a larger default
> max_len
> value?
> It would be cool if the test got the max_len value as an input so it could
> choose an appropriate range to use..
>

I don't think that will matter -- shouldn't be necessary to set
that libfuzzer_options here.

I could be wrong (Max can correct me here), but my understanding is that
max_len sets an *upper bound* on how large fuzzed inputs can get. To
prevent exploring arbitrarily large inputs when all the good stuff is known
to be in smaller inputs (for instance maybe you are parsing something that
always rejects inputs over a certain length, so little value in exploring
inputs beyond that threshold).

In this case if you leave it unspecified, (I believe) the test harness will
determine appropriate lengths to use based on the corpus and coverage
achieved so far, and grow as necessary (up to its own internally configured
max length).

Either way, I think we should leave this up to the fuzzing harness to
decide. We can tweak it for performance reasons if you find it isn't
converging quickly enough.


>
>
> > Instead I suggest just using 2 bytes as first_part_size, and then the
> remaining
> > bytes for the second part.
> >
> > We have //net specific helper routines for this. Some is not yet checked
> in,
> but
> > take a look at:
> >
>
>
https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p...
> >
> > With that paradigm you can write this as:
> >
> > // Allow the first length to be up to 65535 bytes long (uses 2 bytes of
> fuzzer
> > input for this)
> > size_t first_part_size = fuzzed_data.ConsumeValueInRange(0, 0xFFFF);
> >
> > base::StringPiece first_part = fuzzed_data.ConsumeBytes(first_part_size);
> > base::StringPiece second_part = fuzzed_data.ConsumeRemainingBytes();
> >
> > ConsumeRemainingBytes() doesn't exist yet, but I suggest you add it to
> that
> > class -- should just be a 1 liner and is generally useful. (I know Chris
> has a
> > change that needs similar functionality.)
>
> Cool, I'll update once that CL lands.
>
>
> https://codereview.chromium.org/1929703003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mattm, 2016-04-28 19:20:00]

Patchset #2 (id:20001) has been deleted

[mattm, 2016-04-28 19:22:50]

updated to use FuzzedDataProvider

[eroman, 2016-04-28 19:32:11]

lgtm

[mattm, 2016-04-28 19:33:35]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2016-04-28 19:34:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929703003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1929703003/60001

[commit-bot: I haz the power, 2016-04-28 20:41:03]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-04-30 17:20:48]

Patchset 3 (id:??) landed as
https://crrev.com/afe43b899be59473af5620af17d5512555a9c7d6
Cr-Commit-Position: refs/heads/master@{#390467}

[commit-bot: I haz the power, 2016-04-30 17:57:17]

Description was changed from

==========
fuzzers for VerifyNameMatch and VerifyNameInSubtree

BUG=
==========

to

==========
fuzzers for VerifyNameMatch and VerifyNameInSubtree

BUG=

Committed: https://crrev.com/afe43b899be59473af5620af17d5512555a9c7d6
Cr-Commit-Position: refs/heads/master@{#390467}
==========

[mmoroz, 2016-05-04 13:47:48]

Sorry for my delayed response (due to OOO).

Do I understand correctly that it would be better to have max_len equal (2 +
0xFFFF + 0xFFFF) = 131072?

Also would be great to generate some valid testcases (2 bytes of part1 length +
part1 + part2) and use them as seed_corpus in repo ot upload there:

https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer...

https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer...

[mattm, 2016-05-16 19:15:18]

On 2016/05/04 13:47:48, mmoroz wrote:
> Sorry for my delayed response (due to OOO).
> 
> Do I understand correctly that it would be better to have max_len equal (2 +
> 0xFFFF + 0xFFFF) = 131072?

I don't know exactly what the tradeoffs are for fuzzing different sizes. And we
aren't clear if the fuzzer will automatically try different sizes or if you need
to manually specify max_len (in the absence of manually provided testcases).

Is that large a value okay, or is it more efficient to have smaller max len? We
probably don't really need that large a value, so we could modify the fuzzer to
use less if you think that's better. If that value is fine, then yes, the
current version of the test would probably want a max length of 131072.

> Also would be great to generate some valid testcases (2 bytes of part1 length
+
> part1 + part2) and use them as seed_corpus in repo ot upload there:
> 
>
https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer...
> 
>
https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer...

I get the same error for these as I mentioned on the other CL: "You don't have
permission to view the Storage Browser or Storage Settings pages in this
project."

[mmoroz, 2016-05-20 08:24:20]

On 2016/05/16 19:15:18, mattm wrote:
> On 2016/05/04 13:47:48, mmoroz wrote:
> > Sorry for my delayed response (due to OOO).
> > 
> > Do I understand correctly that it would be better to have max_len equal (2 +
> > 0xFFFF + 0xFFFF) = 131072?
> 
> I don't know exactly what the tradeoffs are for fuzzing different sizes. And
we
> aren't clear if the fuzzer will automatically try different sizes or if you
need
> to manually specify max_len (in the absence of manually provided testcases).
> 
> Is that large a value okay, or is it more efficient to have smaller max len?
We
> probably don't really need that large a value, so we could modify the fuzzer
to
> use less if you think that's better. If that value is fine, then yes, the
> current version of the test would probably want a max length of 131072.
> 

Well, to be honest, we also don't know if it is much efficiently to use smaller
max_len.
We definitely know that if max_len value is a big one, libFuzzer will try
smaller testcases as well.
However, if we use smaller value, those smaller testcases will be used more
frequently. For some targets it is useful, so we are trying to use small values.
Bur for other targets (e.g. woff2) we cannot hit anything with small max_len -
we have to use something about 810K because valid corpus files have such size.

My concern here is: by default we have no more than 10K max_len. But size of the
first part will be in range of[0, 65535]. So probably in more than 5/6 of
possible cases first_part will eat all |data| and second part will be empty. May
be smart libFuzzer will avoid that, but not sure.

I will play locally with it and check if it changes anything.


> > Also would be great to generate some valid testcases (2 bytes of part1
length
> +
> > part1 + part2) and use them as seed_corpus in repo ot upload there:
> > 
> >
>
https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer...
> > 
> >
>
https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer...
> 
> I get the same error for these as I mentioned on the other CL: "You don't have
> permission to view the Storage Browser or Storage Settings pages in this
> project."

Weird, it should be accessible to all @google.com accounts :/

[mmoroz, 2016-05-20 09:36:56]

After some testing I think max_len doesn't matter without seed corpus in this
case. Cannot find any new coverage locally with any max_len. Let's generate some
valid inputs and then adjust max_len according to them.