Re: RFR: 8300491: SymbolLookup::libraryLookup accepts strings with terminators [v2]
On Wed, 24 May 2023 17:09:23 GMT, Maurizio Cimadamore wrote: >> There is a difference in behavior between `System::loadLibrary` and >> `SymbolLookup::libraryLookup(String)`. While the former catches library >> names containing NULL chars (because, internally it uses Path/File logic, >> which reject those), `SymbolLookup` does not. As a result, it is possible to >> load libraries such as `libGL.so\0foobar`, because the string is passed >> directly to `dlopen`. >> >> A similar issue arises when we consider `SymbolLookup::find`, as the name is >> again sent unmodified to `dlopen`. In reality, some filtering *does* happen >> in that case, as the native code calls `GetStringUTFChars`: >> >> https://docs.oracle.com/en/java/javase/20/docs/specs/jni/functions.html#getstringutfchars >> >> Which encodes the NULL chars in a different form: >> >> https://docs.oracle.com/en/java/javase/20/docs/specs/jni/types.html#modified-utf-8-strings >> >> So, in practice, even if the input is not validated, a call to `find` with >> one or more NULL chars is unlikely to succeed. >> >> In this patch I have added several checks to make sure that: >> * the name of library passed to `libraryLookup(String)` does not contain >> `\0` and, if so, throw `IllegalArgumentException` (which is already covered >> by current spec; >> * the name of a symbol passed to `SymbolLookup::find` does not contain `\0` >> and, if so, return empty optional. This required changes on all the lookups >> we implement. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add test for system lookup Marked as reviewed by psandoz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14126#pullrequestreview-1442698703
Re: RFR: 8300491: SymbolLookup::libraryLookup accepts strings with terminators [v2]
> There is a difference in behavior between `System::loadLibrary` and > `SymbolLookup::libraryLookup(String)`. While the former catches library names > containing NULL chars (because, internally it uses Path/File logic, which > reject those), `SymbolLookup` does not. As a result, it is possible to load > libraries such as `libGL.so\0foobar`, because the string is passed directly > to `dlopen`. > > A similar issue arises when we consider `SymbolLookup::find`, as the name is > again sent unmodified to `dlopen`. In reality, some filtering *does* happen > in that case, as the native code calls `GetStringUTFChars`: > > https://docs.oracle.com/en/java/javase/20/docs/specs/jni/functions.html#getstringutfchars > > Which encodes the NULL chars in a different form: > > https://docs.oracle.com/en/java/javase/20/docs/specs/jni/types.html#modified-utf-8-strings > > So, in practice, even if the input is not validated, a call to `find` with > one or more NULL chars is unlikely to succeed. > > In this patch I have added several checks to make sure that: > * the name of library passed to `libraryLookup(String)` does not contain `\0` > and, if so, throw `IllegalArgumentException` (which is already covered by > current spec; > * the name of a symbol passed to `SymbolLookup::find` does not contain `\0` > and, if so, return empty optional. This required changes on all the lookups > we implement. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add test for system lookup - Changes: - all: https://git.openjdk.org/jdk/pull/14126/files - new: https://git.openjdk.org/jdk/pull/14126/files/706e9585..52077b57 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14126=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14126=00-01 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14126.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14126/head:pull/14126 PR: https://git.openjdk.org/jdk/pull/14126
Re: RFR: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
On Wed, 24 May 2023 16:30:25 GMT, Paul Sandoz wrote: > Do you also need to test on `SymbolLookup` returned from > `Linker::defaultLookup`? Yeah - some test would be better. - PR Comment: https://git.openjdk.org/jdk/pull/14126#issuecomment-1561568364
Re: RFR: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
On Wed, 24 May 2023 15:22:15 GMT, Maurizio Cimadamore wrote: > There is a difference in behavior between `System::loadLibrary` and > `SymbolLookup::libraryLookup(String)`. While the former catches library names > containing NULL chars (because, internally it uses Path/File logic, which > reject those), `SymbolLookup` does not. As a result, it is possible to load > libraries such as `libGL.so\0foobar`, because the string is passed directly > to `dlopen`. > > A similar issue arises when we consider `SymbolLookup::find`, as the name is > again sent unmodified to `dlopen`. In reality, some filtering *does* happen > in that case, as the native code calls `GetStringUTFChars`: > > https://docs.oracle.com/en/java/javase/20/docs/specs/jni/functions.html#getstringutfchars > > Which encodes the NULL chars in a different form: > > https://docs.oracle.com/en/java/javase/20/docs/specs/jni/types.html#modified-utf-8-strings > > So, in practice, even if the input is not validated, a call to `find` with > one or more NULL chars is unlikely to succeed. > > In this patch I have added several checks to make sure that: > * the name of library passed to `libraryLookup(String)` does not contain `\0` > and, if so, throw `IllegalArgumentException` (which is already covered by > current spec; > * the name of a symbol passed to `SymbolLookup::find` does not contain `\0` > and, if so, return empty optional. This required changes on all the lookups > we implement. Do you also need to test on `SymbolLookup` returned from `Linker::defaultLookup`? - PR Review: https://git.openjdk.org/jdk/pull/14126#pullrequestreview-1442329856
RFR: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
There is a difference in behavior between `System::loadLibrary` and `SymbolLookup::libraryLookup(String)`. While the former catches library names containing NULL chars (because, internally it uses Path/File logic, which reject those), `SymbolLookup` does not. As a result, it is possible to load libraries such as `libGL.so\0foobar`, because the string is passed directly to `dlopen`. A similar issue arises when we consider `SymbolLookup::find`, as the name is again sent unmodified to `dlopen`. In reality, some filtering *does* happen in that case, as the native code calls `GetStringUTFChars`: https://docs.oracle.com/en/java/javase/20/docs/specs/jni/functions.html#getstringutfchars Which encodes the NULL chars in a different form: https://docs.oracle.com/en/java/javase/20/docs/specs/jni/types.html#modified-utf-8-strings So, in practice, even if the input is not validated, a call to `find` with one or more NULL chars is unlikely to succeed. In this patch I have added several checks to make sure that: * the name of library passed to `libraryLookup(String)` does not contain `\0` and, if so, throw `IllegalArgumentException` (which is already covered by current spec; * the name of a symbol passed to `SymbolLookup::find` does not contain `\0` and, if so, return empty optional. This required changes on all the lookups we implement. - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk/pull/14126/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14126=00 Issue: https://bugs.openjdk.org/browse/JDK-8300491 Stats: 33 lines in 5 files changed: 33 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14126.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14126/head:pull/14126 PR: https://git.openjdk.org/jdk/pull/14126
