Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

firefox micro-targets #1462

Closed
pdknsk opened this issue May 30, 2018 · 93 comments
Closed

firefox micro-targets #1462

pdknsk opened this issue May 30, 2018 · 93 comments

Comments

@pdknsk
Copy link
Contributor

pdknsk commented May 30, 2018

I'd like to know what Googlers (and Mozillians) think of this.

There are currently two Firefox projects here: qcms and spidermonkey. Both build stand-alone and are also useful separate from Firefox. Firefox core does however have many more potential targets. I'll give a simple example: the FTP LIST parser. Please take a look.

For such targets Firefox has a native fuzzing interface, which is integrated into its normal build system, and produces a binary that can be run with the normal libFuzzer flags. (ASAN-only currently.) Quite similar to how spidermonkey seems to be integrated here right now.

Is this still within the scope of oss-fuzz?

@kcc
Copy link
Contributor

kcc commented May 30, 2018

Sounds similar to what we discussed with @choller recently...

@kcc
Copy link
Contributor

kcc commented May 30, 2018

And answering another part of the question:
I think that any code that is part of the Firefox is within the scope of oss-fuzz.
From out previous discussion with @choller there might be some challenges making it scale
(e.g. it might be hard to actually have small binaries for every target).

@alex
Copy link
Contributor

alex commented May 30, 2018 via email

@choller
Copy link
Contributor

choller commented May 30, 2018

Yes, we discussed this before, and this is actually the interface I've been mentioning in our meetings. We are also about to land some additional targets in our codebase, in addition to what we have now already. They should all be coverable with a single build. Our build system does partial instrumentation automatically when building a fuzzing build (it adds the libfuzzer flags only in places where we actually have fuzzing targets). If you want to incorporate any of the targets into oss-fuzz, I'll be glad to give some pointers.

@choller
Copy link
Contributor

choller commented May 30, 2018

It is also correct that we build all of libxul and can't build smaller targets, yes. But how does that stop you from scaling it, if the same build can be used for multiple targets?

@kcc
Copy link
Contributor

kcc commented May 30, 2018

how does that stop you from scaling it

Maybe it doesn't, I am not sure.
Most of our fuzz targets are binaries w/o extra .so deps, but I think your approach should work too...
(unless I miss something).

Anyway, looking forward to adding more Mozilla's fuzz targets :)

@pdknsk
Copy link
Contributor Author

pdknsk commented May 31, 2018

I thought about this when I ported qcms to the native fuzzing interface. (That's a separate discussion I'll move to its bug on the Firefox tracker.)

It seems like the best approach for this would be to have a firefox project, that builds select native targets. Since oss-fuzz seems to attempt to run any binary it finds in $OUT I think this should just work. Or wait, the fuzz target is selected with an environment variable.

FUZZER=name path/to/firefox

So either separate binaries for each fuzz target with the variable somehow hard-coded would need to be made, or oss-fuzz told to run the same binary with different environment variables. Is this right?

@alex
Copy link
Contributor

alex commented May 31, 2018 via email

@kcc
Copy link
Contributor

kcc commented May 31, 2018

These will need to be separate binaries, we don't deal with any env vars.
@oliverchang will separate binaries work if they are linked against the same libxul.so?

@choller
Copy link
Contributor

choller commented May 31, 2018

The firefox fuzzing interface heavily relies on environment variables and you cannot just link another binary to libxul.so. The building/linking is complicated and I strongly suggest to use the same binary + libxul.so package for all targets, then use environment variables to configure. If really necessary, you might be able to build wrapper binaries that exec() to the real binary after setting the environment variables, but that's really a hack then.

@kcc
Copy link
Contributor

kcc commented May 31, 2018

In LLVM we use another hack: we build a single binary, and then copy it to several different onces that have different names, and based on the name of the executable we choose what to fuzz.
Ugly, but works.
Env. vars. and exec are no-go, I'm afraid.

@choller
Copy link
Contributor

choller commented May 31, 2018

You cannot do this in Firefox either with the way to fuzzing system is built. We do not simply link to libxul and call a libfuzzer main, but we do initialization work that has to be done first, then call back to our own libfuzzer main that uses a registry of fuzzing targets. Environment variables are used twice along the way at least, eliminating those will be hard.

Why is setting env vars inside a dummy binary (hard coded) and then using execvpe() a no-go?

@jonathanmetzman
Copy link
Contributor

Why is setting env vars inside a dummy binary (hard coded) and then using execvpe() a no-go?

There may be other reasons, but this seems like it will definitely break AFL

@choller
Copy link
Contributor

choller commented May 31, 2018

There may be other reasons, but this seems like it will definitely break AFL

Yes, I only had libFuzzer in mind here which would already be a good start.

@kcc
Copy link
Contributor

kcc commented May 31, 2018

If this work with libFuzzer for you it may work with libFuzzer for oss-fuzz,
but other things will remain broken (not just AFL, but also e.g. the coverage build, etc).

@pdknsk
Copy link
Contributor Author

pdknsk commented May 31, 2018

Yes, but that was a given anyway. The current scope of firefox is only to have its build system make libFuzzer binaries which oss-fuzz simply runs. It doesn't build anything itself. No oss-fuzz C(XX)FLAGS are used, neither is -lFuzzingEngine.

@kcc
Copy link
Contributor

kcc commented May 31, 2018

Let's try.

@pdknsk
Copy link
Contributor Author

pdknsk commented Jun 1, 2018

I think this will work. Using thin wrapper-binaries that call execve works in base-runner locally. There are still other problems that need so be solved with Firefox itself, related to shared libraries. I'll file a bug for discussion on the Firefox tracker.

@alex
Copy link
Contributor

alex commented Jun 5, 2018

Per our conversation today, here's what a stub binary might look like:

#define STRINGIFY_X(v) #v
#define STRINGIFY(v) STRINGIFY_X(v)


int main(int argc, char* argv[]) {
    if (setenv("MOZ_RUN_GTEST", "1", 1) ||
        setenv("LIBFUZZER", "1", 1) ||
        setenv("FUZZER", STRINGIFY(FIREFOX_FUZZ_TARGET))) {

        perror("Failed to set environment variable");
        exit(1);
    }

    char **new_argv = calloc(argc + 1, sizeof(char *));
    new_argv[0] = xxx_path_to_firefox;
    memcpy(new_argv, argv + 1, argc - 1);
    execv(xxx_path_to_firefox, new_argv);
}

you can then compile 1 per fuzz target we want, using -DFIREFOX_FUZZ_TARGET=xxx.

@kcc
Copy link
Contributor

kcc commented Jun 5, 2018

@oliverchang to confirm that this will work in the CF's environment.
Or we can try and see if it works.

@inferno-chromium
Copy link
Collaborator

@jonathanmetzman was discussing that we have a minor optimization on what we unpack, so in this case, we need to disable that somehow since we have to unarchive everything.

@oliverchang
Copy link
Collaborator

Yes the stub approach should work in OSS-Fuzz's environment. There is also a run_minijail script in base-runner that can be used to test this.

@choller
Copy link
Contributor

choller commented Jun 6, 2018

So right now there seem two major blockers for this to work:

  1. The environment variable problem. This seems to be solvable though by using binary stubs hardcoding the environment as proposed earlier and confirmed by @oliverchang

  2. Runtime dependencies missing in the runner image. Firefox has some runtime dependencies like GTK2 that it expects to be present. Since the Firefox fuzzing build is essentially a full browser build that just has its startup routines diverted in various ways to run the fuzzing code after initialization, it still requires some of these dependencies. In https://bugzilla.mozilla.org/show_bug.cgi?id=1466021 @pdknsk has been working on figuring out which dependencies these are (and also successfully used patchelf to remove some unused dependencies). Nevertheless, I guess some of the dependencies won't be removable and will be required on the runner. We would have to figure out which of the dependencies are essential and get them installed in the runner image.

@pdknsk
Copy link
Contributor Author

pdknsk commented Jun 18, 2018

I have this working in principle, with the exception that NEW_FUNC is missing names in base-runner. It works when run from base-builder, so it's not a problem with the build itself. After I figure that out, I'll submit a PR for discussion and comments.

@pdknsk
Copy link
Contributor Author

pdknsk commented Jun 18, 2018

I've been using execve which doesn't inherit environment variables, so PATH wasn't set. Works with setenv, as suggested above. There is another problem with the Firefox fuzzing interface now, which I'll move to its tracker.

@pdknsk pdknsk mentioned this issue Jun 21, 2018
@pdknsk
Copy link
Contributor Author

pdknsk commented Jun 21, 2018

#1546

@pdknsk
Copy link
Contributor Author

pdknsk commented Jul 26, 2018

Alright it's merged in, and hit the first problem.

Step #4:  0:04.92 checking for rustc... not found
Step #4:  0:04.92 checking for cargo... not found
Step #4:  0:04.92 ERROR: Rust compiler not found.

Now rustc is installed successfully in an earlier step, but apparently it cannot find it.

Step #1: Rust installation complete. You should now have rustc and cargo
Step #1: in /root/.cargo/bin
Step #1: 
Step #1: The installer tries to add these to your default shell PATH, so
Step #1: restarting your shell and running this script again may work.
Step #1: If it doesn't, you'll need to add the new command location
Step #1: manually.
Step #1: 
Step #1: If restarting doesn't work, edit your shell initialization
Step #1: script, which may be called ~/.bashrc or ~/.bash_profile or
Step #1: ~/.profile, and add the following line:
Step #1: 
Step #1:     source /root/.cargo/env
Step #1: 
Step #1: Then restart your shell and run the bootstrap script again.
Step #1: 
Step #1: 
Step #1: Your system should be ready to build Firefox for Desktop!
Step #1: 
Step #1: To build Firefox for Desktop, please restart the shell (Start a new terminal window)

This worked in local testing.

The rust installation is done in Dockerfile, so build.sh should run in a new shell no? Or may it cannot change PATH for some reason. Just adding source to build.sh seems easiest to fix this.

@pdknsk
Copy link
Contributor Author

pdknsk commented Jul 26, 2018

Args, still doesn't work.

Step #4: + source /root/.cargo/env
Step #4: ++ export PATH=/builder/home/.cargo/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/out
Step #4: ++ PATH=/builder/home/.cargo/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/out
...
Step #4:  0:05.18 checking for rustc... not found
Step #4:  0:05.18 checking for cargo... not found
Step #4:  0:05.18 ERROR: Rust compiler not found.

The problem is that /root/.cargo/env contains export PATH="$HOME/.cargo/bin:$PATH".

And $HOME is evidently not /root, unlike in local base-builder. So either /root/.cargo/bin should be added manually to PATH in build.sh, which I'm not sure will work, or mach bootstrap should be moved from Dockerfile to build.sh, which I'm not completely sure works either.

Since the images differ, I think guidance from Googlers is required.

@Dor1s
Copy link
Contributor

Dor1s commented Aug 12, 2018

What I don't understand about profile, is why passing a binary to llvm-cov is necessary. I mean default.profdata already knows the used functions and files.

.profdata files contain only counters. The mapping between the counters and the source code is embedded in the binary.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 16, 2018

Works. The missing puzzle piece was passing the library. The question is how to teach it to oss-fuzz.

$ llvm-cov report obj-fuzz/dist/bin/firefox -object obj-fuzz/dist/bin/libxul.so -instr-profile=default.profdata > llvm-cov.txt

Note that the internal fuzzing system is currently broken, which is also why coverage unexpectedly started building successfully. It'll stop building again. Perhaps best to disable it. Will this work in build.sh?

[[ $SANITIZER = "coverage" ]] && exit 0

@Dor1s
Copy link
Contributor

Dor1s commented Aug 16, 2018

Works. The missing puzzle piece was passing the library. The question is how to teach it to oss-fuzz.

Ahh, I see. I think we expect everything to be statically linked, so I don't handle shared libraries on OSS-Fuzz. Looks like I have to. In Chromium we handle it the following way: https://cs.chromium.org/chromium/src/tools/code_coverage/coverage.py?l=386

Perhaps best to disable it. Will this work in build.sh?

Yes, I think that would be fine. I'll probably delete coverage configuration at all later.

@inferno-chromium
Copy link
Collaborator

@pdknsk - there is a startup leak happening 100% of the time, so no progess, if you want to disable leak checking, just add disabling in <fuzz_target_name>.options file. e.g. see projects/radare2/default.options

https://oss-fuzz.com/v2/performance-report/libFuzzer_firefox_SdpParser/libfuzzer_asan_firefox/latest

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 17, 2018

The problem with using ldd to get the shared libraries is that the target binaries call execv and thus don't return the required libraries nor the binary they call. I think the simplest solution is to just pass all .so files found in $OUT to llvm-cov. Or pass everything in $OUT that ldd recognizes as valid. I assume unprofiled files are ignored by llvm-cov.

Note that profile currently only builds in combination with ASAN. I'm not sure if that's a problem, probably yes.

@inferno-chromium I've submitted a PR that should take care of the outstanding issues.

@Dor1s
Copy link
Contributor

Dor1s commented Aug 17, 2018

I assume unprofiled files are ignored by llvm-cov.

No, llvm-cov raises an error if any of the objects passed does not have coverage data section.

Note that profile currently only builds in combination with ASAN. I'm not sure if that's a problem, probably yes.

Yeah, that's not good, as it creates an additional overhead (while coverage instrumentation is already super heavy), but it still may work fine as an intermediate solution.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 19, 2018

No, llvm-cov raises an error if any of the objects passed does not have coverage data section.

What works to get profiled binaries and libraries is to run this on files in $OUT, minus src and work.

nm $FILE 2> /dev/null | grep -m1 __llvm_coverage_mapping

Only one library is really necessary here though, so if it can be passed manually, that works also.

$ python infra/helper.py profile --fuzz-target SdpParser --corpus-dir $CORPUS firefox -- -object /out/firefox/libxul.so

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 19, 2018

It's making progress, but there's still a problem. I'll change target.c.

Dictionary: 56 entries
INFO: Seed: 3883719171
INFO: Loaded 1 modules   (1264001 inline 8-bit counters): 1264001 [0x7f997be109f0, 0x7f997bf45371),
INFO: Loaded 1 PC tables (1264001 PCs): 1264001 [0x7f997bf45378,0x7f997d28eb88),
MERGE-OUTER: 1689 files, 67 in the initial corpus
MERGE-OUTER: attempt 1
LD_LIBRARY_PATH unexpectedly set
...
MERGE-OUTER: zero succesfull attempts, exiting

Also, exiting the coverage build still causes an error. Can be ignored though.

Step #11: + [[ coverage = \c\o\v\e\r\a\g\e ]]
Step #11: + exit 0
Finished Step #11
Starting Step #12
Step #12: Already have image (with digest): gcr.io/oss-fuzz-base/base-runner
Finished Step #12
Starting Step #13
Step #13: Already have image: gcr.io/oss-fuzz/firefox
Step #13: 	zip warning: name not matched: *
Step #13: 
Step #13: zip error: Nothing to do! (try: zip -r firefox-coverage-201808190653.zip . -i *)
Finished Step #13
ERROR
ERROR: build step 13 "gcr.io/oss-fuzz/firefox" failed: exit status 12

I've also noticed that libFuzzer is apparently supposed to run HEAD.

Revisions:
Libfuzzer: 340134

Then I'll sync the internal libFuzzer to that rather than matching the clang revision that's used to build.

Step #4: Updated libFuzzer to 338452

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 19, 2018

I assume LD_LIBRARY_PATH is not suddenly set by oss-fuzz for the merge, but rather that it's still set from the previous call to the binary in the same run. I didn't think of that.

@Dor1s
Copy link
Contributor

Dor1s commented Aug 19, 2018

Once #1726 gets resolved, you should be able to add -object=path_to_shared_library somewhere in project.yaml as an additional argument, so that coverage script will pass it to llvm-cov.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 19, 2018

Once #1726 gets resolved, you should be able to add -object=path_to_shared_library somewhere in project.yaml as an additional argument, so that coverage script will pass it to llvm-cov.

I've been thinking, will it be able to resolve /out or $OUT correctly?

@Dor1s
Copy link
Contributor

Dor1s commented Aug 20, 2018

I've been thinking, will it be able to resolve /out or $OUT correctly?

It depends on how I'll implement this :) But yeah I should either resolve $OUT or request to specify a path relative to $OUT in project.yaml.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 20, 2018

It turns out that paths relative to the target work, so $OUT handling may not be necessary.

$ python infra/helper.py profile --fuzz-target SdpParser --corpus-dir $CORPUS firefox -- -object firefox/libxul.so

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 21, 2018

I've produced a full profile report now. Works well. Takes about 20 minutes, weighs in at more than 1GB.

I'm not sure about a few hundred of h file errors, which are all symlinks.

error: /out/work/obj-fuzz/dist/include/unicode/uchar.h: No such file or directory
warning: The file '/work/obj-fuzz/dist/include/unicode/uchar.h' isn't covered.

To /src/mozilla-central/intl/icu/source/common/unicode/uchar.h here.

@Dor1s
Copy link
Contributor

Dor1s commented Aug 21, 2018

@pdknsk thanks for noticing this. I probably should resolve symlinks when doing:
https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-builder/compile#L71

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 23, 2018

It's failing with an error I don't understand. Still worked yesterday.

libpng12.so.0: cannot open shared object file: No such file or directory
Couldn't load XPCOM.

The problem is in the build log.

Step #6: Already have image: gcr.io/oss-fuzz/firefox
Step #6: 	zip warning: name not matched: lib/libpng12.so.0

I don't know what the warning means.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 23, 2018

The problem appears to be this. There are three relevant files in the package.

/lib/x86_64-linux-gnu/libpng12.so.0 -> libpng12.so.0.54.0
/lib/x86_64-linux-gnu/libpng12.so.0.54.0
/usr/lib/x86_64-linux-gnu/libpng12.so.0 -> /lib/x86_64-linux-gnu/libpng12.so.0.54.0

build.sh copies them to /out/lib in a somewhat random but stable order. So if the last file is copied last, it overwrites the first, and then the absolute symlink doesn't resolve. Strange that it didn't occur before.

@Dor1s
Copy link
Contributor

Dor1s commented Aug 23, 2018

It probably has changed since I added cp -rL instead of cp -r. Otherwise, many projects were failing as there were symlinks pointing outside of available directories.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 28, 2018

Unfortunately ContentSecurityPolicyParser is negatively affected by run_minijail. I'm not sure why.

@Dor1s
Copy link
Contributor

Dor1s commented Aug 28, 2018

//cc @oliverchang for #1462 (comment)

@oliverchang
Copy link
Collaborator

Looking from the name of the top frame I'm guessting it's from lack of shared memory. We don't provide /dev/shm in the minijail environment.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 29, 2018

I'm not familiar with the code, but from the comment of the class that appears to be the case.

/**
 * This class provides a simple, read-only key-value string store, with all
 * data packed into a single segment of memory, which can be shared between
 * processes.
 *
 * Look-ups are performed by binary search of a static table in the mapped
 * memory region, and all returned strings are literals which reference the
 * mapped data. No copies are performed on instantiation or look-up.
 *
 * Important: The mapped memory created by this class is persistent. Once an
 * instance has been initialized, the memory that it allocates can never be
 * freed before process shutdown. Do not use it for short-lived mappings.
 */

Can anything be done about it? I assume not.

@oliverchang
Copy link
Collaborator

Not in the short term unfortunately. We're planning some changes that will let us remove minijail completely, but that may not be for a while.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 29, 2018

Alright, I'll remove the target then.

@pdknsk
Copy link
Contributor Author

pdknsk commented Aug 29, 2018

The target will live. I'll explain in an upcoming PR.

@pdknsk
Copy link
Contributor Author

pdknsk commented Oct 20, 2018

Integration should be complete now, with address, undefined, and coverage working. I think no new targets will be added until run_minijail is removed.

@pdknsk pdknsk closed this as completed Oct 20, 2018
tmatth pushed a commit to tmatth/oss-fuzz that referenced this issue Oct 22, 2018
tmatth pushed a commit to tmatth/oss-fuzz that referenced this issue Oct 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants