Repository navigation
Link libsqreen_jni with -z now to survive early GOT writes (APPSEC-69052) - #219
Conversation
…052) With lazy binding, glibc relocates PLT slots as "*slot += load_base". If another in-process component writes a GOT slot of libsqreen_jni.so before the dynamic linker relocates it, the slot ends up as a wild pointer and the first call through it crashes inside JNI_OnLoad. BIND_NOW makes glibc overwrite the slot with the resolved symbol, and full RELRO keeps the GOT read-only afterwards. Add LazyBindingGotClobberTest, which reproduces the crash with an LD_AUDIT module that writes the malloc slot before relocation, and NativeLibBindNowTest, which checks the release binaries for BIND_NOW.
Skip LazyBindingGotClobberTest when run outside Gradle, always destroy the child JVM, keep the temp dir only on failure, and name the ELF offsets parsed by NativeLibBindNowTest.
- Fix the CodeNarc violations that failed codenarcTest in every CI job running check (Println, BitwiseOperatorInConditional, NestedForLoop, UnnecessaryGetter, UnnecessaryGString) - Redirect the child JVM output to a file and bound the wait with waitFor(timeout), so a hung child is always destroyed (a blocked pipe read ignored the JUnit timeout) - Delete the temp dir when the test is skipped for lack of a C compiler
2524212 to
8ae7a07
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cataphract
left a comment
There was a problem hiding this comment.
For the reason presented in the test, you only need -z now, though I don't see anything wrong with passing -z now -z relro to the linker. You might as well combine it with -fno-plt too.
I'm a bit worried, however, that you did not find the ultimate cause of the crash. There must be some other component misbehaving, something other than glibc writing to the GOT. If there is indeed a race:
- You clobber the other components pointers, if done before the relocation, losing an intended override.
- The other component crashes trying to write to read-only memory.
Can't you tell from the crash telemetry and the exact binary the symbol that corresponds to the corrupted got entry? Telemetry should also give you all the loaded components and you can usually infer their exact versions via the sizes of the mapped regions (provided you have access to these binaries ofc). With this, you could scan all the loaded binaries in search for the culprit.
|
I missed that the description already identifies the corrupted entry as the The crash dump also tells us what was written. With lazy binding glibc did
Given
This is still somewhat speculative, but it should either confirm or rule out the profiler quickly. It would also be good to understand how the writer gets to see |
|
Thanks for the detailed review! I followed your suggestion. From the crash registers:
The telemetry only resolves one other module near it: On the side effects you mention:
I'd keep Are you ok with this? Do you think we can continue with this fix in the meantime? |
|
Update: root cause confirmed with the service owners. The writer is the native ddprof profiler v0.19.0 (the JVM runs as So it's the race you described: ddprof's symbol overrides patched our On your two concerns, now with the actual writer:
Does that sound OK to move forward with this PR? |
cataphract
left a comment
There was a problem hiding this comment.
I think this is PR harmless and likely works most of the time as a workaround for the rare cases when the race with the buggy profiler happens.
Depending exactly how ddprof works (I haven't checked), there may still be lingering problems, e.g. between the time ddprof mprotect(RW) and it does the write, glibc could make the pages read-only, but it likely is a rare event on top of a rare event.
Bump io.sqreen:libsqreen to 17.6.0 libsqreen 17.6.0 links libsqreen_jni.so with BIND_NOW (DataDog/libddwaf-java#219). This fixes a SIGSEGV in JNI_OnLoad when the native ddprof profiler writes its malloc hook into the lazily-bound GOT slot before ld.so relocates the library (APPSEC-69052). Update lockfiles for libsqreen 17.6.0 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
What does this PR do?
Links
libsqreen_jni.sowith-Wl,-z,now -Wl,-z,relroon Linux (full RELRO / BIND_NOW), and adds two regression tests:LazyBindingGotClobberTestreproduces the APPSEC-69052 crash deterministically. AnLD_AUDITmodule (src/test/c/got_clobber_audit.c) writes an absolute pointer into themallocGOT slot oflibsqreen_jni.sofromla_objopen(), which runs after the library is mapped but before it is relocated. A child JVM then loads the library throughWaf.initialize(false).NativeLibBindNowTestparses the ELF dynamic section of every Linuxlibsqreen_jni.soon the classpath and requiresBIND_NOW. It needs no compiler, so it also guards the manylinux release binaries in theTestmatrix, whereLazyBindingGotClobberTestis skipped.Motivation
APPSEC-69052: SIGSEGV inside
JNI_OnLoadwhile AppSec loadslibsqreen_jni.so(linux-aarch64 glibc; seen on dd-trace-java 1.61.1 and on 1.67.1, which bundles libsqreen 17.5.0).From the 1.67.1 crash register dump and the disassembly of the released 17.5.0 binary:
JNI_OnLoad+0xcb0calls a helper that doesbl malloc@pltat+0xc000;R30 = +0xc004.mallocGOT slot (R16 = base + 0x300b0) intox17 = 0x0001d9473aa63774, which is the faulting address.base + a plausible absolute pointer.The glibc builds use lazy binding (no
BIND_NOW,.got.pltoutside RELRO). In that mode glibc relocatesJUMP_SLOTentries as*slot += l_addr(both aarch64 and x86_64). If any in-process component writes a GOT slot before the dynamic linker relocates the library, the slot becomespointer + baseand the first call through it crashes. WithBIND_NOW, glibc overwrites the slot with the resolved symbol, so an early write is harmless. Full RELRO also turns a later wild write into a fault in the writer instead of in our library.Which component writes the slot in production is still being investigated (the Datadog profiler, which patches GOT entries, is loaded in the crashing process). This change makes
libsqreen_jni.sorobust regardless.The musl builds are already linked with
BIND_NOW(Alpine toolchain default); only the glibc builds are affected.Jira ticket: APPSEC-69052
Description of the Change
CMakeLists.txt:target_link_libraries(sqreen_jni PRIVATE -Wl,-z,now -Wl,-z,relro)in the existing non-MSVC/non-Apple block.target_link_optionsis not used because the minimum CMake version is 3.12.src/test/c/got_clobber_audit.c: theLD_AUDITmodule used byLazyBindingGotClobberTest, compiled by the test itself.build.gradle: passes the path of the audit module source to the tests (gotClobberAuditSource).LazyBindingGotClobberTestis skipped when not on Linux, on musl (noLD_AUDIT), whenccis unavailable, or under the ASanLD_PRELOAD. In CI it runs inDev_Tests(ubuntu-22.04) andCoverage. It asserts that the audit module actually patched the slot, so it cannot pass without exercising the scenario. The child JVM output goes to a file and the wait is bounded withwaitFor(timeout), so a hung child is always destroyed.Additional Notes
CI evidence that the tests fail without the fix: run 40480401281 on a temporary commit that reverted only the
CMakeLists.txtchange (since dropped from the branch):-z nowLazyBindingGotClobberTestandNativeLibBindNowTestfail (crash assertion after the GOT slot was patched); 203 other tests passNativeLibBindNowTestfails;LazyBindingGotClobberTestskipped (ASanLD_PRELOAD)CI with the fix: run 40481133489 on 8ae7a07 is fully green, including the whole
Testmatrix on the release binaries built in CI (15 Linux jobs: glibc and musl, x86_64 and aarch64, HotSpot and Semeru, CentOS 6/7). ThereNativeLibBindNowTestis neither failed nor skipped (Gradle--infologs only skipped/failed tests), andLazyBindingGotClobberTestis skipped as expected (no C compiler in those images). InDevelopment tests (ubuntu-22.04)andCoverageboth new tests pass.Verified locally in Linux containers:
libsqreen_jni.so+0xc000<-JNI_OnLoad+0xcb0, sameR16/R30as productionlibsqreen_jni.so+0xb85a, pc = written pointer + load basetestsuite with the BIND_NOW library (aarch64, JDK 21)./gradlew check(incl. CodeNarc, JDK 11) with the BIND_NOW libraryThe reproduction does not need
-XX:+UseCompactObjectHeadersor JDK 25: it crashes on JDK 21 with default flags.Ubuntu's gcc only enables
-z nowfor PIE executables, so the debug library built inDev_Testsis lazy-bound without this change, andLazyBindingGotClobberTestfails there.Cost of BIND_NOW: 78 PLT symbols resolved once at
dlopen. The library only requiresGLIBC_2.17, and all imports (libc, libjvm, libddwaf) are always present.Why full RELRO and not only
-z now: with full RELRO the GOT becomes read-only after loading, so a component that patches it late mustmprotectit first. Any GOT patcher already has to handle this, since many distro libraries are built with full RELRO; the Datadog profiler (ddprof) does importmprotect. A wild late write now faults in the writer instead of corrupting our library.Out of scope, follow-up: the prebuilt
libddwaf.soshipped from the libddwaf release tarball is also lazily bound (noBIND_NOW, 84-87JUMP_SLOTrelocations includingmalloc), so it is exposed to the same early-GOT-write corruption. It cannot be relinked here; it needs the same flag in the libddwaf build.