checkpolicy/oss-fuzz: add libfuzz based fuzzer #313
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@cgzones thanks a lot for working on this! I'll take a look tomorrow. I'm not sure why CIFuzz didn't report a memory leak it found though: It says |
evverx
reviewed
Oct 1, 2021
evverx
reviewed
Oct 1, 2021
| @@ -307,6 +307,7 @@ GLBLUB { return(GLBLUB); } | |||
| %% | |||
| int yyerror(const char *msg) | |||
| { | |||
| #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION | |||
There was a problem hiding this comment.
That's a nice touch! I should have done something like that in the cil parser to prevent secilc-fuzzer from excessive logging.
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
f0e583b to
a5472f6
Compare
The fuzzer has the hardcoded setting to always expect a MLS policy (similar to |
Does oss-fuzz use some custom allocator, that fails unreproducible (to test error branches), cause there are probably a couple dozens memory leaks in error branches in the parser? |
No, it doesn't. I think that particular memory leak isn't reproducible because it can't be triggered by a single file (according to CIFuzz at least), which probably means that some kind of state accumulates between runs. |
|
@cgzones could you temporarily apply the following patch: diff --git a/.github/workflows/cifuzz.yml b/.github/workflows/cifuzz.yml
index 5c2233a2..b28eb71a 100644
--- a/.github/workflows/cifuzz.yml
+++ b/.github/workflows/cifuzz.yml
@@ -30,6 +30,7 @@ jobs:
oss-fuzz-project-name: 'selinux'
fuzz-seconds: 180
dry-run: false
+ report-unreproducible-crashes: true
sanitizer: ${{ matrix.sanitizer }}
- name: Upload Crash
uses: actions/upload-artifact@v1so that CIFuzz could upload the file triggering that memory leak to hopefully make it easier to reproduce it locally. |
Done in #314 |
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
2 times, most recently
from
1fafe97 to
120de3c
Compare
True, the crash was an empty input, and after I added cleanup to some global variables it seems to be fixed. |
|
FWIW I'm not sure why unreproducible crashes aren't reported by default by CIFuzz but I think I'd keep it on. OSS-Fuzz would just keep opening and closing "flaky" issues making it hard to to figure out what that was. |
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
663e979 to
f78fb93
Compare
|
CIFuzz failed due to a bug in libClusterFuzz. It should be fixed in google/clusterfuzz#2471 |
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
d38e373 to
007fd27
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
007fd27 to
5a40db7
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
5a40db7 to
1f1f956
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
1f1f956 to
c3960d4
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
c3960d4 to
43afbab
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
43afbab to
1a622bb
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
1a622bb to
a5f7e98
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
a5f7e98 to
dec1f96
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
dec1f96 to
3a27142
Compare
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
2 times, most recently
from
754a3eb to
9e3a301
Compare
Introduce a libfuzz[1] based fuzzer testing the parsing and policy generation code used within checkpolicy(8) and checkmodule(8), similar to the fuzzer for secilc(8). The fuzzer will work on generated source policy input and try to parse, link, expand, optimize, sort and output it. This fuzzer will also ensure policy validation is not too strict by checking compilable source policies are valid. Build the fuzzer in the oss-fuzz script. [1]: https://llvm.org/docs/LibFuzzer.html Signed-off-by: Christian Göttsche <[email protected]>
Close the input file and free all memory by the queue and lexer on a syntax or parse error. Signed-off-by: Christian Göttsche <[email protected]>
Free identifiers removed from the queue but not yet owned by the policy on errors. Signed-off-by: Christian Göttsche <[email protected]>
Signed-off-by: Christian Göttsche <[email protected]>
…tion Signed-off-by: Christian Göttsche <[email protected]>
The passed expression needs to be transferred into the policy or free'd by the sink functions define_constraint() and define_validatetrans(). Signed-off-by: Christian Göttsche <[email protected]>
Calling the parser macro YYABORT allows the parser to cleanup up any allocated resources before returning. Signed-off-by: Christian Göttsche <[email protected]>
Return early on invalid roles in user definition. Signed-off-by: Christian Göttsche <[email protected]>
Convert the only usage of the raw type struct level_datum to use the typedef. Simplifies refactorizations on the type. Signed-off-by: Christian Göttsche <[email protected]>
Add a new member to the struct level_datum to indicate whether the member `level` is owned by the current instance, and free it on cleanup only then. This helps to implement a fix for a use-after-free issue in the checkpolicy(8) compiler. Signed-off-by: Christian Göttsche <[email protected]>
During compilation sensitivity aliases share the level with their prime
sensitivity, until after the level has been fully defined they are
deduplicated. If an error happens by that time the cleanup will free
the shared level multiple times, leading to a use-after-free.
Make use of the added new member of the struct level_datum.
Example policy:
class c sid e class c{i}sensitivity S alias L;
Signed-off-by: Christian Göttsche <[email protected]>
Provide more descriptive error messages by including the identifier or other kind of value if available. Also drop duplicate newlines at the end of messages. Signed-off-by: Christian Göttsche <[email protected]>
Free the temporary bounds type in the error branches. Signed-off-by: Christian Göttsche <[email protected]>
Only assign the computed value on success, since it is not set by
declare_symbol() on failure.
Reported by GCC:
module_compiler.c: In function 'create_role':
module_compiler.c:287:24: warning: use of uninitialized value 'value' [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
287 | datum->s.value = value;
| ~~~~~~~~~~~~~~~^~~~~~~
Signed-off-by: Christian Göttsche <[email protected]>
Sync function parameter names. Drop superfluous return value. The function avrule_merge_ioctls() has no failure conditions and always returns 0. Drop duplicate include. Use native type for ranges. Signed-off-by: Christian Göttsche <[email protected]>
cgzones
force-pushed
the
checkpolicy_fuzz_patch
branch
from
9e3a301 to
d4bb604
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a libfuzz1 based fuzzer testing the parsing and policy
generation code used within checkpolicy(8) and checkmodule(8), similar
to the fuzzer for secilc(8).
The fuzzer will work on generated source policy input and try to parse,
link, expand, optimize, sort and output it.
Build the fuzzer in the oss-fuzz script.
/cc @fishilico @evverx
I am not familiar how the actual integration into oss-fuzz works and if it needs any update.