SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.46k stars 3.18k forks source link

Possible use-of-uninitialized-memory error in add_possible_value() in LibJS #24560

Closed Heinzeen closed 3 months ago

Heinzeen commented 3 months ago

Summary

The variable possible_pointer might be used uninitialized in add_possible_value() when a specific input is given.

Details

In function gather_conservative_roots(), in Heap.cpp:335 a buffer jmp_buf buf is declared but not initialized. In the following line, a call to setjmp() is performed and buf is given as an argument. The buffer is then used in a for loop (line 345) where it is read 8 bytes at a time and each time the data is passed to add_possible_value.

In linux, it is not specified whether setjmp saves the signal mask or not. In our testing environment (Ubuntu 20.04 x86_64) it does not and iterating through buf results in using uninitialized data since the fields related to signals inside buf are not initialized, but are still used inside add_possible_value.

We checked how setjmp is implemented inside SerenityOS and we saw that it initializes all the data including did_save_signal_mask and saved_signal_mask. This should prevent this problem from occurring when executing inside SerenityOS.

Nevertheless, we thought we should bring this issue to your attention for two reasons: first, many fuzzing sessions will likely be performed in other Linux-based operating systems which will still experience the bug and this might prevent us from discovering bugs deeper in the execution; second, we did not find documentation explicitly saying that setjmp and _setjmp functions initialize all the data fields, so in case there are any changes to this policy in the future, you may start experiencing this bug.

PoC

In the attached archive you will find:

The binary we used in our tests was generated from one of the fuzzing harnesses available for OSS-Fuzz, with a main function that simply opens a file and calls LLVMFuzzerTestOneInput with the input data. The harness was compiled with clang 18.

To reproduce the bug, a tool to detect memory-related bugs like valgrind is needed. With valgrind, you can reproduce the bug with a command similar to valgrind --track-origins=yes --exit-on-first-error=yes --error-exitcode=1 ./FuzzJs testcase.

Proposed solution

To solve the problem, the jmp_buf struct might be initialized when it is first declared. If you do not want to make a redundant initialization (since SerenityOS’s implementation of setjmp initializes those fields) you may want to explicitly initialize the data only in fuzzing mode, to allow fuzzing to go as deep as possible without encountering false positives.

nico commented 3 months ago

Great write-up, thanks!