Add Unsafe array access sanitizer
#932
Conversation
| @@ -48,6 +48,15 @@ java_library( | |||
| ], | |||
| ) | |||
|
|
|||
| java_library( | |||
| name = "unsafe_array_out_of_bounds", | |||
There was a problem hiding this comment.
The implementation currently actually detects a bit more than just "out-of-bounds access" (e.g. it also detects unaligned object array access, and reading / writing bytes from an object array and the other way around).
So maybe a name like "unsafe_array_invalid_access" or similar would be better?
| if (!componentType.isPrimitive()) { | ||
| // Reading or writing bytes to an array of references; might be possible but seems | ||
| // rather unreliable and might mess with the garbage collector? | ||
| report("Reading or writing bytes from a " + objClass.getTypeName()); | ||
| } |
There was a problem hiding this comment.
Not sure if this is too strict. On the other hand there might be no guarantees on how large object references are so any access where object references are treated as bytes seems error-prone.
| "com.code_intelligence.jazzer.api.FuzzerSecurityIssueCritical", | ||
| ], | ||
| target_class = "com.example.UnsafeArrayOutOfBounds", | ||
| verify_crash_reproducer = False, |
There was a problem hiding this comment.
During my testing when reproducer verification was enabled no sanitizer exception was thrown. Are sanitizers generally not enabled when running in reproducer / regression (?) mode?
That might be rather problematic; in the case of Unsafe that might crash the JVM, and I guess for the other sanitizers (such as the path traversal one) it could also have undesired consequences (e.g. if due to path traversal files are placed at random paths on the machine).
| java_fuzz_target_test( | ||
| name = "UnsafeArrayOutOfBoundsValid", | ||
| srcs = [ | ||
| "UnsafeArrayOutOfBoundsValid.java", | ||
| ], | ||
| allowed_findings = [], | ||
| fuzzer_args = [ | ||
| # Test does not depend on fuzzing input; just run it once | ||
| "-runs=1", | ||
| ], | ||
| target_class = "com.example.UnsafeArrayOutOfBoundsValid", | ||
| ) |
There was a problem hiding this comment.
Not sure if this is a good approach to ensure that the Unsafe sanitizer does not report errors for valid access.
| // TODO: Is this a proper way to implement this? | ||
| Method testMethod = testMethods[data.consumeInt(0, testMethods.length - 1)]; |
There was a problem hiding this comment.
Not sure if this is a good / proper way to ensure that all of the methods cause a sanitizer error.
Once a fuzzing test run finds the first expected failing input (in this case here basically any input), does it keep reusing that? In that case it would never run any of the other test methods here.
Is there a better way to solve this, other than adding dozens of separate classes each with their own fuzzerTestOneInput?
|
I think this PR is now ready for an initial review; any feedback is appreciated! Please also let me know what you think about the points I mentioned above in my review comments. Also, is there a way to let Bazel install the Maven artifacts (especially the Jazzer JUnit artifact and its dependencies) as SNAPSHOT to the local Maven repository (similar to what |
|
|
|
Thanks! That only works for Linux though because I will see if I can maybe get it working with WSL 2 or a Docker container. But if that then only produces the Linux and not the Windows native libraries for Jazzer (not completely sure how the native build for Jazzer works) it will also be a bit cumbersome to then use these artifacts. (Side note: I had to update locally some of the Edit: Was able to build it now in a Docker container, but had to copy the native libraries which I had previously built on Windows into the JAR. The sanitizer seems to work. |
|
If you follow the steps at https://bazel.build/install/windows#install-compilers to install MSYS2 and Visual Studio, The publishing script for |
| } | ||
|
|
||
| private static void report(String message) { | ||
| Jazzer.reportFindingFromHook(new FuzzerSecurityIssueCritical(message)); |
There was a problem hiding this comment.
Might be nice to report also for which Unsafe method this occurred. Since the hooks are run before the actual Unsafe method, it does not appear in the stack trace. For code like unsafe.putLong(dest, destOffset, unsafe.getLong(src, srcOffset)) that makes it a bit more difficult to tell which of the Unsafe calls caused the exception, and you cannot debug inside the Unsafe call either since sanitization already happens before.
Any suggestions for how reporting could be improved (in case you consider this an issue) are welcome.
|
Thanks for the hints!
I worked around it now by building within a Docker container, but maybe other users would benefit from that as well, so thanks for your work on this. |
Adds a sanitizer which looks for invalid array access performed by
sun.misc.Unsafe. Unlike native memory access performed byUnsafe, array access can be sanitized in a stateless way just based on the arguments passed to theUnsafemethod. And multiple Java libraries have usedUnsafefor arrays in the past to improve performance, see related security advisories GHSA-8wh2-6qhj-h7j9 and GHSA-973x-65j7-xcf4.Note that fuzzing likely cannot find all such invalid accesses because some of it occurs for numeric overflow respectively multiple MB of processed data, which the fuzzer might be unable to generate.
See related #891
I used #915 as reference for how to implement a sanitizer. However, I am not very familiar with Bazel and this project setup here, and also have / had issues with building it locally on Windows. Therefore I have marked this PR as Draft for now.
Any feedback is appreciated!