Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1776323002:
[libfuzzer] Add fuzzer for FT_New_Memory_Face() from third_party/freetype2.
Description[libfuzzer] Add fuzzer for FT_New_Memory_Face() from third_party/freetype2.
R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org
BUG=569578
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fix the nit. #
Messages
Total messages: 14 (3 generated)
Description was changed from ========== [libfuzzer] Add fuzzer for FT_New_Memory_Face() from third_party/freetype2. R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org BUG=539572 ========== to ========== [libfuzzer] Add fuzzer for FT_New_Memory_Face() from third_party/freetype2. R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org BUG=569578 ==========
inferno@chromium.org changed reviewers: + bungeman@chromium.org
lgtm. +cc bungeman@ for this freetype review. https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/f... File testing/libfuzzer/fuzzers/ft_new_memory_face_fuzzer.cc (right): https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/f... testing/libfuzzer/fuzzers/ft_new_memory_face_fuzzer.cc:12: FT_Error g_init_freetype_result; nit: s/g_init_freetype_result/g_init_freetype_error to match other variable names.
https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/B... File testing/libfuzzer/fuzzers/BUILD.gn (right): https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/B... testing/libfuzzer/fuzzers/BUILD.gn:332: "//third_party/freetype2", While this is the freetype we run the tests with, it isn't very interesting to fuzz because it's really old (it's the one in precise). It is unlikely that any findings would be current. https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/f... File testing/libfuzzer/fuzzers/ft_new_memory_face_fuzzer.cc (right): https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/f... testing/libfuzzer/fuzzers/ft_new_memory_face_fuzzer.cc:31: extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { This already exists better at https://chromium.googlesource.com/chromium/src/third_party/freetype2/+/master... which is run continuously on gce, see https://github.com/google/libfuzzer-bot/blob/master/freetype/README.md
https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/B... File testing/libfuzzer/fuzzers/BUILD.gn (right): https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/B... testing/libfuzzer/fuzzers/BUILD.gn:332: "//third_party/freetype2", On 2016/03/09 18:34:35, bungeman2 wrote: > While this is the freetype we run the tests with, it isn't very interesting to > fuzz because it's really old (it's the one in precise). It is unlikely that any > findings would be current. Just wanted to point out that running this against any of the targets in Chromium is strictly less interesting that running against FreeType tip of tree using their build system. We do try to keep up to date these days, but there is a big difference between continuous and once a month or so.
aarya@google.com changed reviewers: + aarya@google.com, kcc@chromium.org
On 2016/03/09 22:14:18, aarya wrote: bungeman@, for context, https://github.com/google/libfuzzer-bot/blob/master/freetype/README.md is not continuous testing. It is one-time fuzzing effort done by Kostya's team and we are collaborating with them so that this can be run continuously on ClusterFuzz. For stuff to run on ClusterFuzz, right now we are relying on third party libs in chromium codebase (it will be changed in future where we will just use those libs from trunk). Till then, we prefer that if you can update the copy with trunk in Chromium repo, that is much appreciated. We were already hitting bugs with local run, so updating this copy in chromium will help validate them.
On 2016/03/09 22:17:04, aarya wrote: > On 2016/03/09 22:14:18, aarya wrote: > > bungeman@, for context, > https://github.com/google/libfuzzer-bot/blob/master/freetype/README.md is not > continuous testing. It is one-time fuzzing effort done by Kostya's team and we > are collaborating with them so that this can be run continuously on ClusterFuzz. > For stuff to run on ClusterFuzz, right now we are relying on third party libs in > chromium codebase (it will be changed in future where we will just use those > libs from trunk). Till then, we prefer that if you can update the copy with > trunk in Chromium repo, that is much appreciated. We were already hitting bugs > with local run, so updating this copy in chromium will help validate them. If you're finding issues with the third_party/freetype2 build (and you are and will) then you need to report those issues against Debian and Ubuntu as potential security issues in Precise. Also, we don't ship that build (as noted in the README.chromium). Basically, the DEPS for third_party/freetype2 aren't moving any time soon. That being said, fuzzing the third_party/freetype-android build might be interesting (we ship it), but it's also not very interesting to FreeType development, as it will always be out of date and not catch things quickly. If you're planning on building from FreeType master and using their build eventually, what is the impediment to doing so now? I can of course bump the DEPS on third_party/freetype-android right now, and that helps for the moment, but isn't interesting in even the medium term, since anything I roll to right now is already rather well fuzzed. Also, this is an enormous downgrade to the fuzzer driver. FreeType already has two much better ones (one for parsing and one for rendering).
Thanks bungeman@. As Abhishek already said we care about the code which is used in Chromium tree. Our goal is to decrease number of bugs there, this is why we fuzz not ToT versions at the moment. You are right, we will find bugs in old versions of software, but this is a good driver to update libraries (for example, recent libxml update or deletion of libexif). Can we remove third_party/freetype2 since we don't ship it? Looking at README.chromium, "it is only to be linked into test binaries". Does it mean that one day freetype2 will be linked not only in test binaries? Speaking about existing fuzzers, thank you for the links. The bug linked to this CL is "Port target functions from https://github.com/google/libfuzzer-bot/", so initially I was going to use that existing fuzzer (ftfuzzer.cc). However, it uses libarchive which I don't like because: 1) I cannot find libarchive in Chromium tree 2) I don't know why it is needed 3) It seems to be not good dependency to build on platforms different from Linux Thus, I decided to start with this simple fuzzer. It doesn't invoke many interesting functions like FT_Set_Var_Design_Coordinates(), FT_Attach_Stream(), FT_Load_Glyph(), etc. for two reasons: 1) with random data this fuzzer usually doesn't go after first FT_New_Memory_Face() call 2) in does fuzzing of FT_New_Memory_Face() only and explicitly called "third_party_ft_new_memory_face_fuzzer" Since it gives crashes locally, I want to ship it and run on ClusterFuzz. Sure, we will add some corpus, new fuzzers or update target function of this fuzzer later. I've already started to work on dictionary generation for this. Thank you for the suggestion to fuzz third_party/freetype-android, will do this.
https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/f... File testing/libfuzzer/fuzzers/ft_new_memory_face_fuzzer.cc (right): https://codereview.chromium.org/1776323002/diff/1/testing/libfuzzer/fuzzers/f... testing/libfuzzer/fuzzers/ft_new_memory_face_fuzzer.cc:12: FT_Error g_init_freetype_result; On 2016/03/09 17:16:43, inferno wrote: > nit: s/g_init_freetype_result/g_init_freetype_error to match other variable > names. Done.
The bug I found locally seems to be so old: 1) CVE-2012-1134 by Mateusz (j00ru): https://bugzilla.redhat.com/show_bug.cgi?id=800592 2) then in 2015 by Kostya: http://savannah.nongnu.org/bugs/?func=detailitem&item_id=45955 What for reason we keep such code in Chromium repo?
> What for reason we keep such code in Chromium repo? We should clearly update freetype in chromium to the most recent release. There were ~50 bugs fixed in the last couple quarters. And then we should be fuzzing both the chromium version and tot. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
