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

[fuzzuf-cc] Phase 2: Renewal of forkserver mode #53

Merged
merged 17 commits into from Mar 23, 2022

Conversation

K-atc
Copy link
Contributor

@K-atc K-atc commented Mar 17, 2022

Type of PR

  • Changes related to the roadmap (e.g., TODO.md) (type: A) -> Create an issue corresponding to the PR in advance, and refer to this PR in the issue.
  • Changes that are not related to the roadmap
    • Change with multiple possible solutions to the issue (type: B-1) -> Create an issue corresponding to the PR in advance and refer to this PR in the issue.
    • Change with a single solution (type: B-2) -> There is no need to create an issue corresponding to the PR in advance. Please discuss it in this PR.

Related Issue

Importance of PR

  • Importance of the issue
    • Large (based on several days to weeks of discussion and verification, e.g., this issue is a blocking issue for other issues on the roadmap, etc.)
    • Medium (based on a few hours to a day of discussion and verification, e.g., this issue is a blocking issue for another minor issue)
    • Small (apparent changes such as build error)
  • Complexity of the solution (code, tests, etc.)
    • Large (requires several days to several weeks of review)
    • Medium (requires several hours to a day of review)
    • Small (trivial changes, such as build error)

PR Overview

Concerns (Optional)

NOTE: 💣 represents down-side risk

  • Performance

    • In my local environment, the performance of fuzzuc-cc's forkserver is enough.
    • AFL original: average exec speed (execs_per_sec): 3298.99, Crashes: 5795 (34 unique)
    • fuzzuf-cc: average exec speed: 3135.94, Crashes: 7446 (35 unique)
    • Measurement is performed once on each condition. The duration of each campaign is 10 min.
  • Source Code Quality

    • If you feel that the implementation of executor has been improved by defining the protocol specifications for forkserver, then this measure is successful!
    • The communication layer between fuzzer and forkserver is classed as FdChannel.
    • Instead of calling recv() and send() directly in the executor, call API provided by forkserver (see Issue for API list). It might have been better to prepare an API class to abstract the communication layer, but since the available communication method is only a file descriptor method, API is implemented directly in FdChannel.
  • If possible, I would like reviewers to try the entire process at hand, from instrumentation to fuzzing.

    • For the time being, merging fuzzuf-cc-related changes to master branch will not affect users, so it is OK to confirm after all phases are completed.
  • I would like to keep the conventional implementation (NativeLinuxExecutor) and the new implementation (LinuxForkServerExecutor) in parallel for a while.

    • Now that you can switch Executor from the CLI, you can use -e native for the former and -e forkserver for the latter.
    • This PR will not affect users using existing implementation.
    • We will consider when to stop keeping old and new ones and move to a new one later phase.
  • Fear of degradation 💣

    • Rewrote logic of executor to check if a crash has occurred. It is possible to put install bugs unintentionally.
      • Previously, the status value obtained by the forkserver with waitpid() was passed to the executor, and the executor would judge exit status accordingly.
      • In this PR, forkserver passes PUT exit code and signal number to the executor, and the Executor decides accordingly
    • It has been confirmed that crashes can be detected and fuzzing can continue for 10 minutes. (See "Artifacts" section)
  • Known implementation defects 💣

    • fuzzuf-cc does not take care of the case when a PUT is stopped by a signal. This is a problematic implementation if the persistent mode is implemented in the future.
    • We will consider how to deal with it in the next phase 3 (on handling of child processes).

Additional dependencies

Artifacts

Commands to start fuzzing:

$ rm -rf /tmp/fuzzuf-out_dir
$ ./build/fuzzuf afl -e forkserver --log_file=fuzzuf.log --in_dir docs/resources/exifutil/fuzz_input/ -- ~/atla/fuzzuf-cc/put/exifutil/fd-exifutil -f @@

Screenshot:
Untitled

Runtime logs (fuzzuf.log)(抜粋):

Exec Status { is_signaled=true, signal_number=6, exit_status=6 } (pid 0)
Exec Status { is_signaled=true, signal_number=11, exit_status=11 } (pid 0)
PUT Execution Timeout
Exec Status { is_signaled=true, signal_number=9, exit_status=9 } (pid 0)

(Reference) No. 6 is SIGABRT, No. 11 is SIGSEGV, and No. 9 is SIGKILL. SIGKILL is the signal when forkserver exits when the PUT execution times out.

Files marked as a crash can reproduce crash as follows:

$ ~/atla/fuzzuf-cc/put/exifutil/afl-exifutil -f id:000000,sig:06,src:
000000,op:flip1,pos:3
File: id:000000,sig:06,src:000000,op:flip1,pos:3
afl-exifutil: exifutil.c:626: jpeg_custom_app1: Assertion `len_field > sizeof(len_field) + JPEG_EXIF_IDENTIFIER_LEN + sizeof(tiff_header_t)' failed.
中止

$ ~/atla/fuzzuf-cc/put/exifutil/afl-exifutil -f id:000020,sig:11,src:000020,op:flip1,pos:56
File: id:000020,sig:11,src:000020,op:flip1,pos:56
Image description       : 2
Segmentation fault

$ ~/atla/fuzzuf-cc/put/exifutil/afl-exifutil -f ../hangs/id\:000001\,src\:000000\,op\:arith8\,pos\:99\,val\:-18
File: ../hangs/id:000001,src:000000,op:arith8,pos:99,val:-18
Unknown tag (262)       : 2
Orientation             : 1
X Resolution            : 72/1
Y Resolution            : 72/1
^C // Became no response 

NOTE: When binary instrumented with fuzzuf-cc is invoked without forkserver client (i.e. when executed directly from the command line), execution is aborted. So crash reproduction was verified with a binary instrumented with afl-gcc.


The PR author should fill in the following checklist when submitting this PR.

Optional Entries

  • If this PR is a PR type A/B-1, there is a cross-link between this PR and the related issue.

Mandatory Entries

  • The PR title is a summary of the changes.
  • Completed each required field of the PR.

The PR author should fill out the following checklist in the comments to confirm that this PR is ready to be merged

  • CI is green or confirmed test run results.
  • All change suggestions from reviewers have been resolved (fixed or foregone).

The maintainer of this repository will set up a reviewer for each PR.
PR reviewers should review this PR in terms of the checklist below before moving on to a detailed code review. Please comment on their initial response by filling in the checklist below.

Optional Entries

  • The reviewer assigned more reviewers if needed.
  • The reviewer noted that it is necessary to break out some of the changes in this PR into other PRs if needed.
  • The reviewer noted that the initial response is insufficient if needed.

Mandatory Entries

  • The title of this PR summarizes the changes made by this PR properly.
  • The target branch of this PR is as intended.
  • The reviewer understands the issues in this PR.
  • The reviewer plans to review with an appropriate workload based on the importance of this PR.

When the PR reviewer concludes that this PR is ready to be merged, please fill in the checklist below by posting it in the comment. If there is more than one reviewer, please do this on your own.

Optional Entries

  • The reviewer noted that if you believe that new tests are needed to evaluate this PR, they have been noted.
  • If minor refactorings are not mentioned in the PR, I understand the intent.
  • If this PR is a PR type A/B-1, we have agreed on this PR's design, direction, and granularity in the related issue.

Mandatory Entries

  • The reviewer understands how this PR addresses the issue and the specific changes.
  • This PR uses the best possible issue resolution that the reviewer can think of.
  • This PR is ready to be merged.

@K-atc K-atc self-assigned this Mar 17, 2022
@K-atc K-atc added the enhancement New feature or request label Mar 17, 2022
@K-atc K-atc mentioned this pull request Mar 17, 2022
30 tasks
@K-atc K-atc requested a review from potetisensei March 17, 2022 10:40
Copy link
Contributor

@potetisensei potetisensei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a large part of changes introduced by this PR is no problem at all.
Accordingly to fuzzuf-cc, some minor issues in this PR should be fixed probably.
Most of the issues are just that unnecessary code is copied from NativeLinuxExecutor and left as it is.

The maintainer of this repository will set up a reviewer for each PR.
PR reviewers should review this PR in terms of the checklist below before moving on to a detailed code review. Please comment on their initial response by filling in the checklist below.

Optional Entries

  • The reviewer assigned more reviewers if needed.
  • The reviewer noted that it is necessary to break out some of the changes in this PR into other PRs if needed.
  • The reviewer noted that the initial response is insufficient if needed.

Mandatory Entries

  • The title of this PR summarizes the changes made by this PR properly.
  • The target branch of this PR is as intended.
  • The reviewer understands the issues in this PR.
  • The reviewer plans to review with an appropriate workload based on the importance of this PR.

@K-atc K-atc added this to the fuzzuf-cc milestone Mar 22, 2022
@K-atc
Copy link
Contributor Author

K-atc commented Mar 22, 2022

📝 Performed changes to overcome 歴史的経緯 (historical background)

  • Removed signal handler from LinuxForkServerExecutor
  • Removed active_instance
  • Removed ChildState
  • Removed child_pid
  • Stop calling KillChildWithoutWait()
    • Since deconstructor of FdChannel should terminate PUT Process
  • Refactoring last_exit_reason
    • Semantics are same with NativeLinuxExecutor.
  • Removed put_status
  • Stop calling MEM_BARRIER()
    • It seems no effect on LinuxForkServerExecutor
    • MEM_BARRIER() is required to avoid reading shared memory before PUT exit. So I won't remove.

channel/fd_channel.cpp Outdated Show resolved Hide resolved
channel/fd_channel.cpp Outdated Show resolved Hide resolved
channel/fd_channel.cpp Outdated Show resolved Hide resolved
channel/fd_channel.cpp Outdated Show resolved Hide resolved
channel/fd_channel.cpp Outdated Show resolved Hide resolved
utils/common.cpp Outdated Show resolved Hide resolved
include/fuzzuf/utils/common.hpp Show resolved Hide resolved
channel/fd_channel.cpp Outdated Show resolved Hide resolved
K-atc and others added 2 commits March 23, 2022 16:38
@K-atc
Copy link
Contributor Author

K-atc commented Mar 23, 2022

📝 Additional changes:

@K-atc K-atc force-pushed the feature/fuzzuf-cc/phase-2 branch from 2504b30 to 00926ad Compare March 23, 2022 09:03
Copy link
Contributor

@potetisensei potetisensei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GG

@K-atc
Copy link
Contributor Author

K-atc commented Mar 23, 2022

ジージー

@K-atc K-atc merged commit f1ecd47 into master Mar 23, 2022
@K-atc K-atc deleted the feature/fuzzuf-cc/phase-2 branch March 23, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants