improve/correct shell commands, quoting, inconsistencies, dangerous commands

[hg42]

Originally posted by @hg42 in https://github.com/machiav3lli/oandbackupx/issues/288#issuecomment-767552480

Though, I don't think we can switch from shell commands to getting root permissions and using APIs instead. APIs and libraries using them seem to be quite limited in Java/Android. E.g. I didn't find a good API for selinux contexts and file attributes only handle rwx but ignore special flags (SUID etc.). Though, I'm neither an Android nor a Java expert, so it might well be, that I simply didn't find the better APIs... It is also confusing, that I often find multiple solutions for the same problems, but none seems to solve 100%.

We currently have two separated parts of file handling:

• the system side (apps and data) uses su shell commands
• the backup files use APIs

Interestingly, most of the limitations happen on the API side. I don't know the exact reasons, but our API based tar extraction code once uncompressed directly to the system, but it was changed to uncompressing to a temporary directory and moving that to the system.

Shell commands have a stable interface (despite everyone tells a different story). Though, future changes are possible. It would eventually help to use our own binaries.

Things I plan to visit:

• have a look at mixed API + shell code
• have a look at quoting, e.g. I see "..." quoting, usually I would use '...', I am quite sure dollar, backslash etc. in file names will create problems. Though, some of the code isn't used currently, e.g. suUncompressTo TarUtils
• eliminate String.format
• the interface to shell commands should use arrays of parameters and the whole quoting/escaping should be hidden inside ShellHandler / ShellCommands
• integrating a good native tar command could help...I am sure, someone already solved all the problems for us (= compiled a working GNU tar), this would give us selinux contexts, xattr, correct attributes. However, native subprojects are a new arena for us.

Shell commands using arrays has another problem: ShellHandler.runAsRoot("cd \"$targetDir\" && ln -s \"${file.absolutePath}\" \"${tarEntry.linkName}\"; cd -") With array parameters this would quote or escape the && (and pipes etc.).

Alternatively parameters coming from variables could be inserted by a quoting function: ShellHandler.runAsRoot("cd "+ShellHandler.quote(targetDir)+" && ln -s "+ShellHandler.quote(file.absolutePath)*+" "+ShellHandler.quote(tarEntry.linkName)+"; cd -")

This way we could also change the code incrementally instead of changing the interface. On the other hand, changing the interface would ensure all places are found and changed.

[hg42]

I'm working on it...

• added ShellHandler.quote
• added ShellHandler.quoteMultiple (anyone come up with a better name?) for an array of parameters used at several places
• there are several inconsistencies
  • handling of parameter arrays
  • handling of utilbox (prependUtilbox(...), "${shell.utilboxPath} ...", "$UTILBOX_PATH ...", String.format("%s ...*", UTILBOX_PATH, ...), ...)
  • UTILBOX_PATH is a constant ("toybox"), but this doesn't use the path detected at runtime (which might also be a busybox in the future)
  • "rm " + array vs. array.map { "rm " + it } (max. number of arguments?)
• have a look at rm commands, make sure they don't remove too much, e.g. something like "rm -r /data/lib/$packageName/*"
• I found a "rm -r /data/lib/$packageName" instead of "rm -r /data/lib/$packageName/" (missing slash), which could have deleted packages with names staring with the current package name, not very likely, but possible
• utilBoxVersion should be determined from the same user who runs it, so runAsRoot
• if tempDir doesn't exist, the targetDir should not be wiped

[hg42]

unfortunately this affects a lot of places / files

[hg42]

we have ShellCommands and ShellHandler...I am not sure about the difference, from memory it relates to introducing SAF?

[Jakeler]

I don't think the APIs would be a serious limitation. For example the SDK provides direct access to syscalls in android.system.Os, so it could just call chmod,setxattr, etc... otherwise the high level java.nio.file API should be suitable. SELinux does not necessarily need an additional API, because it is simply stored in xattr, for example on app data:

raphael:/data/data/org.fdroid.fdroid # getfattr -d -m'.' files/
# file: files/
security.selinux="u:object_r:app_data_file:s0:c512,c768"

Main problem is that these APIs can't be used from the app process currently, because of lacking privileges. App isolation stops reading/writing other app data of course. I assume that is the reason for the uncompressing into a temp dir, it is the only way to have full control over the process, to do decryption and other preprocessing in the app.

If Shizuku/Sui works as described the app could:

use Android APIs directly in Java as the identity of root, and "user service", start app's own AIDL-style Java service under root. This will make root app developing much more comfortable.

This service could have a well defined custom interface. Also the performance should be much better. I will look into it more when I find the time... In any case a fallback to shell operation would still be needed, especially if the Magisk modules are not available, as using a shell is the standard in other root solutions as well.

The other shell approach would be to let tar handle the complete IO. btw: The Termux project regularly builds many tools for Android, this also includes a full fledged GNU tar.

Adding a quote function is a very good idea. In Kotlin it can be used also in string templates, no need to concatenate with +:

runAsRoot("cd ${quote(targetDir)} && ln -s ${quote(file.absolutePath)} ${quote(tarEntry.linkName)}; cd -")

I was also wondering why it is mostly quoted with " while using ' should be safer. On the other hand if the text contains a ' it will unavoidable fail, because then backslash escapes are also ignored. So another approach would be to escape all problematic characters in that function.

About ShellCommands vs ShellHandler: According to git most of the current implementation was made in #64. This introduced ShellHandler with functions to gather information and helper functions to run commands. As far as I understand it the ShellCommands contains stuff that actually changes the system state, like uninstall, wipe, etc... That being said, ShellCommands.getUsers() would have to be moved to stay consistent with that logic...

I guess documentation is also a topic, a short description (docstring) on every class would help to understand codebase.

[hg42]

we already use java.nio.file. But look at this: https://docs.oracle.com/javase/8/docs/api/java/nio/file/AccessMode.html only rwxrwxrwx nothing else. ok, android.system.Os (which is also used in OABX) can be used for that one. But it's another level, it's usually a bad idea to mix levels, it might work, but it's often not guarantied (e.g. because of caching etc.).

Nice, you seem to know about selinux (I don't use it, so I have no clue and cannot test it). I wasn't aware, that selinux contexts are stored in xattr (which I don't use either). So this part is easy (and which API can we use for these?). Storing them in a tar file is the other problem. I think they are stored in pax headers, and I read on linux a Redhat name is used, but this seems to be kind of non-standard, do you know about this? is there a standard for tar? also how to store xattr, would fit nice in pax headers I think... but there seems to be potential for name conflicts, because pax headers are used for other attributes, too?

Termux has it's local storage, I think OABX cannot call it's commands? Does it store selinux? I think, at some point, I found an Android GNU tar somewhere else... then it comes to adding native programs to the project.

Your runAsRoot line is exactly how I implemented it.

The double quotes come from a Windows user I guess...

just tried on linux (zsh):

% echo 'xy''z'
xy'z

I never used that before, it may be a zsh feature...and yes using double quotes and escape everything should be ok. My intention at this point was to change to a quote function. How it is exactly implemented will be a next step. But you are right I should add a way for the singe quote. Will try when I get some free time...

The ShellHandler/Commands question was more of a question to myself...but thanks for the pointer to #64, saves some work. ShellCommands is something else than I thought, before reading the sources...it's only a collection/abstraction of some commands, which I didn't expect, becasue not all commands are abstracted, e.g. there are a lot of mv, cp, rm, which could also be ShellCommands. The strategy changed at some point.

[hg42]

@Jakeler would be nice, if you could test my PR #298, I look forward to your comments (also from everyone else)

[hg42]

mostly fixed with #298