Avoid executing ELF files directly #24
Conversation
LLVM looks at /proc/self/exe to determine which binary has been called. This does not work with the proposed changes for termux-exec: termux/termux-exec-package#24 Luckily the fallback to look at argv0 works instead.
LLVM looks at /proc/self/exe to determine which binary has been called. This does not work with the proposed changes for termux-exec: termux/termux-exec-package#24 Luckily the fallback to look at argv0 works instead.
Without -f/--full, pgrep&pkill matches searches by default only on the binary name, which will not work with the changes proposed in termux/termux-exec-package#24, as there the binary will always be /system/bin/linker64.
Without -f/--full, pgrep&pkill matches searches by default only on the binary name, which will not work with the changes proposed in termux/termux-exec-package#24, as there the binary will always be /system/bin/linker64.
fornwall
force-pushed
the
master
branch
4 times, most recently
from
2a852ec to
6baed3d
Compare
|
Very interesting solution. But yeah, it likely won't be acceptable by play store policies, but could be tried. Although not sure if android could add further selinux restrictions to prevent this. Btw are you sure in testing, you uninstalled all termux apps, rebooted, and reinstalled, since otherwise The executable being |
termux-exec.c
Outdated
|
|
||
| #ifndef TERMUX_BASE_DIR | ||
| # define TERMUX_BASE_DIR "/data/data/com.termux/files" | ||
| #define TERMUX_BASE_DIR "/data/data/com.termux/files" |
There was a problem hiding this comment.
Do not use hardcoded com.termux values here and elsewhere, it affects termux forks. To get prefix, first check TERMUX__PREFIX env variable, then PREFIX, otherwise default to package build time path @TERMUX_BASE_DIR@. In future, multiple rootfs support may also be added and $PREFIX has been deprecated in favour of TERMUX__PREFIX (note double dash as scope), as it conflicts with other packages like npm, etc. Haven't pushed change to termux-app yet.
There was a problem hiding this comment.
Changed to look at TERMUX__PREFIX now, with a TERMUX_BASE_DIR value given at compilation time as fallback.
I think PREFIX is too dangerous to use, as you say it can totally break when reused for other purposes.
There was a problem hiding this comment.
Thanks. But do not use literal com.termux value. The whole path @TERMUX_BASE_DIR@ should be replaced during build time in the source by value set in properties.sh. Possibly tests should use this too.
For inspiration, check termux-tools design by grimler.
https://github.com/termux/termux-tools/blob/master/configure.ac
https://github.com/termux/termux-tools/blob/master/Makefile.am
Also do not limit prefix length to 48, package names can be much longer. For example /data/user/10/net.dinglisch.android.taskerm/files will fail with length 49. Even /data/data paths can be longer. Package name alone on playstore can be of length 50. Probably just check if it starts with / since technically packages can be compiled for any prefix, even /data/local/tmp for adb or /mnt paths for external sd.
There was a problem hiding this comment.
Yeah, good idea not to use $PREFIX, will have to push TERMUX__PREFIX export to termux-app as well.
There was a problem hiding this comment.
But do not use literal com.termux value
With the current version (I made changes and squashed commits): We start by using the TERMUX__PREFIX env variable now. If that does not exist, we fall back to TERMUX_BASE_DIR.
TERMUX_BASE_DIR is given at compilation time, as in make TERMUX_BASE_DIR=/a/path/to/package/prefix (the
It's not really hardcoded any longer, but supplied at compile time (the existing package already supplies that at build time: https://github.com/termux/termux-packages/blob/master/packages/termux-exec/build.sh#L10).
Remaining usage of com.termux are ok I think:
- In
Makefile, as a default value ifmakeis executed without any argument - In
termux-exec.c, but only inside#ifdef UNIT_TEST, so only used for testing. We also test with other prefixes as well in the tests. - In
test-program.c, which is just a utility test program that can be run manually, not part of the package
There was a problem hiding this comment.
Thanks for your thorough review and feedback @agnostic-apollo !
There was a problem hiding this comment.
Will check code in detail later, outside, but if app is installed on adoptable external sd, then its app data parent directory prefix will be /mnt/expand/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/user/0, which alone has length 55, so 100 is not going to be enough, especially if multiple sub dirs under package name.
You are very welcome. :)
There was a problem hiding this comment.
so 100 is not going to be enough
Thanks again! So what about 200, that should be enough for everyone perhaps :)? That allows a path such as /mnt/expand/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/user/0/yyyyyyyyyy-yyyyyyyyyy/yyyyyyyyyy-yyyyyyyyyyyyyyyyyyyy/yyyyyyyyyyyyyyyyyyyy/yyyyyyyyyyyyyyyyyyyy-yyyyyy yyyy/xxxxx-xxxxx/yyyyy-yyyyy/zzzzzz-zzzzz/.
The reason I don't want an arbitrary big length is that it would be good to have a reasonable tight limit - this code is going to be injected in a lot of places, so not taking up more stack or heap space than necessary would be great, to decrease risks of interfering with the surrounding problem.
There was a problem hiding this comment.
lolz technically the only valid length is PATH_MAX(4096 bytes), but that would be for the full path, not just the prefix. That is also what libraries including mine use as the standard for passing to sys calls. You could reduce it if necessary, but couple of hundred bytes won't matter, but at least log a warning if value is ignored.
As Play Store accepts apps that run binaries by |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
I think it may be more likely that they don't know proot is being used, otherwise if someone took interest, proot and this clearly does violate the policy.
|
Thanks, I noticed the behaviour you described as well, was really confusing first! But now I can confirm that I am indeed testing with effective targetSdk of 34! I'll share a branch of termux-app for testing shortly. |
|
I think we should focus on the technical issue here, and leave Google Play policy out of the discussion, since I think that's a separate one. This change is hopefully good on its own, for bumping targetSdk also for distribution outside of Google Play. |
Welcome. Android also sometimes assigns latest target sdk
Fair enough. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
That solution may be problematic. Linker on old Android version does not allow to do this. But I am not sure when it starts. |
Looks like it was introduced in https://android.googlesource.com/platform/bionic/+/8f639a40966c630c64166d2657da3ee641303194, which was first included in Android 8. |
Only supporting android >= 8 (or even a version or two higher) for an app re-introduced to google play seems reasonable to me |
|
Android 10+ solution is not needed to be used on older versions. It can be enabled on-demand, possibly with letting user to opt-out from this solution. Don't forget that:
Though I found one problem with this solution: users will no longer be able to run static binaries. When I attempt to run static binary, I get error |
|
The |
|
@agnostic-apollo I've invited you to get write permissions to the https://github.com/termux-play-store/termux-exec repository, so you should be able to push to the |
agnostic-apollo
force-pushed
the
master
branch
from
bb75f48 to
cf0da45
Compare
|
Looks like that worked, thanks a lot. That should make things easier. |
agnostic-apollo
force-pushed
the
master
branch
from
cf0da45 to
6101ae1
Compare
agnostic-apollo
force-pushed
the
master
branch
3 times, most recently
from
f460791 to
06edc37
Compare
…ue #1072 needs extra attention Fornwall is active again and work is already being done on #1072 for an alternate variant at termux/termux-exec-package#24 and the other alternate termux/termux-app#2155 (comment) will be worked on in future.
…in/linker*` to bypass android app data file exec restriction if using `targetSdkVersion` `>= 28` on Android `>= 10` - See the README file for a description for how `system_linker_exec` works. - If `TERMUX_EXEC_OPTOUT` environment variable is set, then do not intercept `execve()` at all to fix shebang issues or use `system_linker_exec`. - If `TERMUX_EXEC_DEBUG` environment variable is set, then log debug info for `termux-exec`. - Use `TERMUX__PREFIX` environment variable to extract termux rootfs directory path that's used to generate termux bin path to replace `/bin` and `/usr/bin` path in shebang of scripts instead of using hardcoded `TERMUX_BASE_DIR` build variable. - Since when executing with linker, the `/proc/self/exe` will be set to linker path, export `TERMUX_EXEC__PROC_SELF_EXE` environment variable with actual path to executable being executed so that packages can be patched to read it instead. - Added `src/exec-variants.c` to hook the entire `exec()` family of functions, which is required for Android 14. Closes termux/termux-packages#18537, termux/termux-app#3758
…ENSE` to `licenses/termux__termux-exec__Apache-2.0.md` file and use `debian/copyright` format in `LICENSE` file The `LICENSE` file will list other licenses in future. Also fix copyright statement.
…nges in 5ea25ee and add `libtermux-exec_c` - The `libtermux-exec_c` c library has been added to handle all the `LD_PRELOAD` intercepts implementation, and other functionality required for `termux-exec` like the environment/config. This can be used statically or dynamically for `termux-exec` executables/libraries and also for other packages. - The `libtermux-exec-ld-preload.so` library has been added to be set in `$LD_PRELOAD`. It statically depends on `libtermux-exec_c` library and primarily includes `src/TermuxExecLDPreloadEntryPoint.c`, which now exclusively only defines the functions intercepted by `libtermux-exec-ld-preload.so` and directs them to their intercepts in respective source files of `libtermux-exec_c` library. For backward compatibility, `libtermux-exec.so` is also created as a copy of `libtermux-exec-ld-preload.so` so that older clients do not break which have exported path to `libtermux-exec.so` in `$LD_PRELOAD`. - The intercepts implementation of `execve` done by `src/termux-exec.c` has been moved to `lib/termux-exec_c/src/termux/api/termux_exec/exec/ExecIntercept.c`. The `src/exec-variants.c` has been moved to `lib/termux-exec_c/src/termux/api/termux_exec/exec/ExecVariantsIntercept.c` which handles intercepts of the entire `exec()` family of functions except `execve()`. The intercept implementations are now called from `src/TermuxExecLDPreloadEntryPoint.c`. - The `__attribute__((visibility("default")))` needs to be set for all the intercepted functions in `src/TermuxExecLDPreloadEntryPoint.c` as `libtermux-exec-ld-preload.so` is compiled with `-fvisibility=hidden`, so that no other function in the library other than ones explicitly labelled are exported. This can be confirmed by running `nm --demangle --dynamic --defined-only --extern-only /home/builder/.termux-build/termux-exec/src/build/output/usr/lib/libtermux-exec-ld-preload.so` after building the package. - Revert order of functions in `src/exec/ExecVariantsIntercept.c` back to the one in AOSP and uncomment the `NULL` path checks for `execvpe()` and add its original license to file header. Log entries are also added to each variant to know which variant was originally intercepted. - The `system_linker_exec` support added in 5ea25ee has been removed, and will be added again in a later commit. - Removed the singular `TERMUX_EXEC_OPTOUT` environment variable. Opt outs should be confined to specific intercepts and logics. - Added the `bool` environment variable `TERMUX_EXEC__INTERCEPT_EXECVE` for whether `execve` would be intercepted for shebang fix or `system_linker_exec` (to be added later). The default value is `true`. The other wrapper functions in the `exec()` family of functions declared in `unistd.h` are always intercepted to solve some other issues on older Android versions, check [`libc/bionic/exec.cpp`](https://cs.android.com/android/platform/superproject/+/android-14.0.0_r1:bionic/libc/bionic/exec.cpp) git history. - Use `normalizePath()` and `absolutizePath()` path functions from `libtermux-core_c` library for executable and interpreter path processing that has been tested on hundreds of test cases to handle all the required cases. - Fix the order for normalize, absolutize and replacing termux bin prefix for both executable path and interpreter, check comment in `execveIntercept()`. - Fix relative paths for interpreter path by absolutizing it. Previously, only prefix was being replaced. - Fix `argv[0]` for executing shell scripts where it should be set to the original interpreter set in the file as is instead of the `argv[0]` to `execve()` being intercepted for the executable. - Increase buffer size for executable file shebang header from `256` to `340` defined by `FILE_HEADER__BUFFER_LEN` as per termux path limits, check comment in `ExecIntercept.h` file and [Termux File Path Limits](https://github.com/termux/termux-packages/wiki/Termux-file-system-layout#file-path-limits) docs. - Decrease max length of valid `TERMUX__ROOTFS` from `200` to `86` defined by `TERMUX__ROOTFS_DIR___MAX_LEN`, check `TermuxFile.h` file and [Termux File Path Limits](https://github.com/termux/termux-packages/wiki/Termux-file-system-layout#file-path-limits) docs. - Use `termuxPrefixPath()` functions from `libtermux-core_c` library, which also has buffer overflow checks and return errors for it. - Fix replacing prefix if termux rootfs is `/` or `/system` and executable equals the bin directory itself instead of a subfile. - Fix `fexecve()` where executable path would be `/proc/self/fd/<n>` and checking if its under Termux app data directory directory would give wrong results. The `isPathUnderTermuxAppDataDir()` function from `libtermux-core_c` library handles this by getting real path of file and ensuring that the real path is for the same file for which fd was open by comparing `stat.st_dev` and `stat.st_ino`. - Fix checking if executable is under Termux directories, like Termux app data directory (previously rootfs/base). Previously, `strstr(executable_path, termux_base_dir)` was used, which would check first occurrence of the substring `termux_base_dir` in `executable_path`, and not whether `executable_path` is under `termux_base_dir`. This is now properly handled by `isPathInDirPath()` via `isPathUnderTermuxAppDataDir()` from `libtermux-core_c` library. - Do not use `TERMUX__PREFIX` to get Termux rootfs directory by getting its parent directory and use `TERMUX__ROOTFS` environment variable directly. There may also be cases where they `TERMUX__PREFIX` equals `TERMUX__ROOTFS`, and getting parent directory would result in wrong results. Now `getTermuxPrefixDir()` from `libtermux-core_c` library is used, in which if environment variable is not set or is invalid as per `TERMUX__ROOTFS_DIR___MAX_LEN`, then it returns the default Termux prefix for which package was compiled for as long as its executable and readable to ensure `termux-exec` was not compiled for a different package. - The `modifyExecArgs()` function now handles all changes to the arguments. - Fix issues where `errno` may already be set when `execve` is entered, check comment in `init()` function of `TermuxExecInit.c` where it is set to `0`. - Fix hardcoded `com.termux` values being used, all constants are replaced during building including the root scope of environment variables that are read and as per `TERMUX_ENV__S_ROOT` defined in `properties.sh` of `termux-pacakges` as `TERMUX_`. - Use `stringStartsWith()` from `libtermux-core_c` library, instead of `starts_with()` giving wrong results for `NULL` and empty strings. - Added logger framework from `libtermux-core_c` library with multiple log levels with log entries for all the important variable states to track logic. The singular `TERMUX_EXEC_DEBUG` environment variable has been removed. The `int` `TERMUX_EXEC__LOG_LEVEL` environment variable controls the log level based on `(OFF=0, NORMAL=1, DEBUG=2, VERBOSE=3, VVERBOSE=4 and VVVERBOSE=5)`. The default value is `1`. Normally, `termux-exec` does not log anything at default log level `1` (`NORMAL`) for intercepts even and will require setting log level to `>= 2` (`DEBUG`) to see log messages. To enable `VVERBOSE` logging for a command, you can run something like `TERMUX_EXEC__LOG_LEVEL=4 id -u`. - Added `TERMUX_EXEC_PKG__VERSION` `Makefile` parameter that gets logged on intercept for `termux-exec` package version currently installed. - Create or move all build files under the `build/output/` directory. It's better to build a directory structure for the files to be added to prefix under the `build/output/usr` directory than have a mix of using files under both `src/` and `build/` during installation. This way `src/` directory also doesn't get modified and will not contain the files with replaced constants that were created from `*.in` files. This also makes it easier to clean and build the deb since all built files exist under the same directory, which `termux-create-package` can also use with `source_recurse`. - The recursive replacement of termux constants is done with `find -exec sed` with `TERMUX__CONSTANTS__SED_ARGS` passed to it. Using `foreach` with the `call` function for `replace-termux-constants` does not work for some reason if multiple files exists, because somehow function definition itself like `@sed` gets passed to `sed` command as argument. So `replace-termux-constants` is only called for individual files. The sed replacements surround arguments with double quotes instead of single quotes, so it could potentially break on more cases during shell expansion, although regex variables weren't being escaped before either. - The `termux-exec-package.json` will now have correct version and be consistent with `build.sh`, and also include tests files, which wasn't being done before. - The `termux-exec-package.json` previously had hardcoded `aarch64` as architecture, now we find and replace it for the compiler based on which predefined architecture macro is defined.
…and removed in xxxxx - `lib/termux-exec_c/src/termux/api/termux_exec/exec/ExecIntercept.c` from `libtermux-exec_c` handles the `system_linker_exec` now. - Added the `string` `TERMUX_EXEC__SYSTEM_LINKER_EXEC` environment variable for whether to use `system_linker_exec` if `TERMUX_EXEC__INTERCEPT_EXECVE` is enabled. If set to `disable`, `system_linker_exec` will be disabled. If set to `enable`, then `system_linker_exec` will be enabled but only if required. If set to `force`, then `system_linker_exec` will be force enabled even if not required and is supported. The default value is `enable`. Check `shouldSystemLinkerExec()` in `ExecIntercept.h` and `ExecIntercept.c` for more info and how its handled. Docs will be added in a later commit. The `system_linker_exec` will now engage for executable or interpreter paths that are under `TERMUX_APP__DATA_DIR` or `TERMUX_APP__LEGACY_DATA_DIR` instead of `TERMUX__ROOTFS` (`TERMUX_BASE_DIR`). - Use `getAndroidBuildVersionSdk()` from `libtermux-core_c` library to read `ANDROID__BUILD_VERSION_SDK` environment variable exported by Termux app to get Android build version sdk, and if its not set, then from the `android_get_device_api_level()` call provided by `<android/api-level.h>`, which gets it from the system properties, which should be slower. - Fix the environment `envp` being copied twice during `execve` for system bin paths, once for unsetting `LD_` variables and then to set `TERMUX_EXEC__PROC_SELF_EXE`. The `modifyExecEnv()` function now handles all changes to environment with a single copy. - Fix `TERMUX_EXEC__PROC_SELF_EXE` being set even if `system_linker_exec` is not being used on `targetSdkVersion <= 28`, etc, and also not being unset when going from `system_linker_exec` to direct execution like system binaries. Packages will be patched to detect if `system_linker_exec` is being used to modify their behaviour, which shouldn't be modified for direct execution.
…add some tests initially added in 5ea25ee and removed in xxxxx - Move unit tests from `src/termux-exec.c` to `tests/TermuxExecUnitTests.c` since tests shouldn't be in source files. - Added testing framework via `tests/termux-exec-tests.in` and `lib/termux-exec_c/tests/libtermux-exec_c_tests.in` that calls `termux-exec/lib/termux-exec_c/tests/src/libtermux-exec_c_unit-binary-tests.c` to run unit tests, and `lib/termux-exec_c/tests/src/libtermux-exec_c_runtime-binary-tests.c` and `lib/termux-exec_c/tests/scripts/libtermux-exec_c_runtime-script-tests.in` for runtime tests. Old tests files in random places have been removed. The entire `exec()` family of functions is also tested by `lib/termux-exec_c/tests/src/termux/api/termux_exec/exec/ExecIntercept_RuntimeBinaryTests.c`. Docs will be added in a later commit.Tests can be run with `"${TERMUX__PREFIX:-$PREFIX}/libexec/installed-tests/termux-exec/termux-exec-tests -vv all"`.
…_PROC_SELF_EXE` as `/proc/self/exec` would contain linker executable path Related pull termux/termux-exec-package#24
New Changes Since Last Push
Respective pulls/releases for
I will be merging all the pulls sometime after the weekend, as |
This is change to work around the Android 10
R^Wviolation (execution of binary files are not allowed).See the README for more information.
Helping with testing
Help to test this would be great! For context and more information, see
Problem 1andSolution 1in the updated README.NOTE: May totally break your system and need a reinstallation (or require a manual repair in a failsafe session - see below).
Steps (on aarch64 - let me know if you want to test on some other arch):
pkg upcp $PREFIX/lib/libtermux-exec.so $PREFIX/lib/libtermux-exec.so.baktermux-execcontaining this code:curl -o ~/te-997.deb https://fornwall.me/te-997.deb && apt install --reinstall ~/te-997.debAfter this, new terminal sessions will be using the updated
termux-execversion. A way to check this is by runningls -l /proc/self/exe- it should show a symlink pointing to a path ending inbin/linker64, since the linker is what is being executed.Try things out (run scripts, compile programs, etc) and report if something does not work as a comment in this issue!
This should only affect behaviour on Android 10+ devices, but if you have an old Android version you are welcome to test that as well - that nothing has changed there.
Helping with testing: Debugging tools
export TERMUX_EXEC_DEBUG=1to get verbose logging to stderrexport TERMUX_EXEC_OPTOUT=1to opt out from termux-exec - it will not do any modification toexecvecallsHelping with testing: Known issues
zigdoes not work (linker errors withhas unexpected e_type: 2)Helping with testing: To restore
If things are not completely broken,
apt install --reinstall termux-exec=1:1.0will install the stable version oftermux-exec.If the installation is completely broken, so you can't even start a shell:
NEW SESSIONbutton, and clickFAILSAFEin the dialog that shows up.cd $PREFIX/lib && cp libtermux-exec.so.bak libtermux-exec.soapt install --reinstall termux-exec=1:1.0Updates
te-997.deb,apt show termux-execshould show version9:9.7):TERMUX_SELF_EXE, the absolute path to the executed file, which can be used as a/proc/self/execreplacement.