termux / termux-api-package

Termux package containing scripts to call functionality in Termux:API.
MIT License
994 stars 314 forks source link

Connect over a unix socket before trying to use am #147

Closed tareksander closed 2 years ago

tareksander commented 2 years ago

This is the corresponding PR for this PR for Termux:API.

Preparing the arguments for transmission is a bit ugly, because of string handling in C. If the transmission fails for whatever reason, it falls back to calling am. That makes it work reliably, even if the Termux:API process gets killed while the transmission is in progress.

Would it be possible to use C++ for this file instead, so you can just concatenate the strings and operate on them?

I also added 2 new scripts to start and stop the new service easily.

tareksander commented 2 years ago

Scripts using -a some-action are not current handled, termux-usb -l and termux-volume music 0 for example gives something like Unsupported options: -alist.

Fixed that now, I don't thing there should ever be spaces inside the action string, so that doesn't need to be wrapped in quotes.
Also using quotes for the string extras was just the easiest way I could think of to make whitespace in strings work, I don't know if there is a better system for that. Passing each argument as a separate line would have the same problems with newlines, so I don't know what system would be better. Maybe using null bytes for string separation is better, C strings can't contain null bytes anyways, so there is no need to escape any characters.

agnostic-apollo commented 2 years ago

The termux-api-package scripts can create a single string with escaped single quotes for all intent extras and then you can pass it as is in termux-api.c and in App.java you can use ArgumentTokenizer.java to convert the string into an array and then loop on it as done by am. This way you won't need double quoted length calculation either.

The following functions are part of a new "posixy" compliant termux library I'll be pushing hopefully soon and should work fine for complex data with am command, which is also currently broken (#130) since no care is taken for special characters. The termux-api-package scripts should have placeholders so that such functions are automatically added at build time to each script with sed. Also need to use placeholders to add support for root detection and if termux-api is installed or not as per termux/termux-app@0cf3cef7 to solve hanging issues. Let me know if I missed some function, there are part of different files/libs. Since scripts will be running in bash, you can use some of the commented out quote instead, except the printf ones and data__escape_single_quotes sed alternative, since that will be significantly slower for relatively shorter strings due to sub shell opening.

DATA__NL='
'

##
# Add an key or key/value extra to an intent string.
# .
# .
# android__intent__add_intent_extra `intent_string_variable_name` `data_type` `extra_key` `extra_value` [`add_only_if_set` `enusre_set`]
##
android__intent__add_intent_extra() {

    local data_type="$2"
    local extra_key="$3"
    local extra_value="$4"
    local add_only_if_set="$5"
    local enusre_set="$6"

    local data_type_argument
    local extra_key_argument
    local extra_value_argument

    case "$data_type" in
        "String")
            data_type_argument="--es";;
        "boolean")
            data_type_argument="--ez";;
        "int")
            data_type_argument="--ei";;
        "long")
            data_type_argument="--el";;
        "float")
            data_type_argument="--ef";;
        "String[]")
            data_type_argument="--esa";;
        "List<String>")
            data_type_argument="--esal";;
        "Integer[]")
            data_type_argument="--eia";;
        "List<Integer>")
            data_type_argument="--eial";;
        "Long[]")
            data_type_argument="--ela";;
        "List<Long>")
            data_type_argument="--elal";;
        "Float[]")
            data_type_argument="--efa";;
        "List<Float>")
            data_type_argument="--efal";;
        "Component")
            data_type_argument="--ecn";;
        "Uri")
            data_type_argument="--eu";;
        "Key")
            data_type_argument="--esn";;
        *)
            logger__log_errors "Unsupported data_type \"$data_type\" passed to android__intent__add_intent_extra"
            return 1
    esac

    if [ "$data_type_argument" = "Key" ]; then
        shell__arg__validate_argument_count eq $# 3 android__intent__add_intent_extra "$@" || return $?
    else
        shell__arg__validate_argument_count or $# 4 6 android__intent__add_intent_extra "$@" || return $?
    fi

    shell__validate_variable_set extra_key android__intent__add_intent_extra || return $?
    data__escape_single_quotes extra_key_argument "$extra_key" || return $?

    if [ "$data_type_argument" = "Key" ]; then
        shell__append_to_variable "$1" " $data_type_argument '$extra_key_argument'" || return $?
    else
        if [ -z "$extra_value" ]; then
            if [ "$add_only_if_set" = "1" ]; then
                return 0
            elif [ "$enusre_set" = "1" ]; then
                logger__log_errors "The \"$extra_key\" value is not set while running \"android__intent__add_intent_extra\""
                return 1
            fi
        else
            data__escape_single_quotes extra_value_argument "$extra_value" || return $?
        fi

        shell__append_to_variable "$1" " $data_type_argument '$extra_key_argument' '$extra_value_argument'" || return $?
    fi

}

# Is a valid boolean extra that can be passed to `--ez` parameter of `am` command.
# https://cs.android.com/android/platform/superproject/+/android-11.0.0_r3:frameworks/base/core/java/android/content/Intent.java;l=7574
# .
# .
# android__intent__is_intent_extra_boolean `value`
##
android__intent__is_intent_extra_boolean() {

    case "$1" in
        "true"|"t") return 0;;
        "false"|"f") return 0;;
    esac

    data__is_unsigned_int "$1"

}

##
# android__intent__validate_is_intent_extra_boolean `label` `value` [`error_output_variable_name`]
##
android__intent__validate_is_intent_extra_boolean() {

    local label="$1"
    local value="$2"
    local error

    if ! android__intent__is_intent_extra_boolean "$value"; then
        error="$label is not a valid intent extra boolean: \"$value\""
        if [ -n "$3" ]; then
            shell__set_variable "$3" "$error" || return $?
        else
            logger__log_errors "$error"
        fi
        return 1
    else
        return 0
    fi

}

##
# data__is_unsigned_int `value`
##
data__is_unsigned_int() {

    case "$1" in
        ''|*[!0-9]*) return 1;;
        *) return 0;;
    esac

}

##
# Print a string to stdout and append a newline.
# .
# .
# data__println_string `string`
##
data__println_string() {

    #if [ "$DATA__IS_PRINTF_SHELL_BUILTIN" = "1" ]; then
        printf "%s\n" "$1"
    #else
    #    echo "$1"
    #fi

}

##
# Print a string to stdout with `printf` and append a newline.
# .
# .
# data__printfln_string `string`
##
data__printfln_string() {

    #if [ "$DATA__IS_PRINTF_SHELL_BUILTIN" = "1" ]; then
        printf "%s\n" "$1"
    #else
    #    data__printf_string_inner "%s" "%s\n" "$1"
    #fi

}

##
# Replace a single quotes `'` in string with `'"'"'`.
# .
# .
# data__escape_single_quotes `output_variable_name` `string`
##
data__escape_single_quotes() {

    data__replace_character_in_string "$1" "$2" "'" "'\"'\"'" || return $?

    # Alternative
    #shell__set_variable "$1" "$(data__printf_string "${string}" | sed "s/'/'\"'\"'/g")"

}

##
# Replace a character with another string.
# .
# .
# data__replace_character_in_string `output_variable_name` `string` `character_to_replace` `replace_with`
##
data__replace_character_in_string() {

    local string="$2"
    local character_to_replace="$3"
    local replace_with="$4"

    local __new_string
    local char
    #local string_rest

    while [ -n "$string" ]; do
        #string_rest="${string#?}" #1:end
        #char="${string%"$string_rest"}" # 0:1
        #string="$string_rest" # 1:end

        char="${string:0:1}" # 0:1
        string="${string:1}" # 1:end

        case "$char" in
            "$character_to_replace") __new_string="${__new_string}${replace_with}";;
            *) __new_string="${__new_string}${char}";;
        esac
    done

    shell__set_variable "$1" "$__new_string"

}

##
# shell__validate_variable_set `variable_name` [`function_name`]
##
shell__validate_variable_set() {

    local curr_value
    shell__copy_variable curr_value "$1" || return $?

    if [ -z "$curr_value" ]; then
        if [ -n "$2" ]; then
            logger__log_errors "The $1 is not set while running \"$2\""
        else
            logger__log_errors "The $1 is not set"
        fi
        return 1
    else
        return 0
    fi

}

##
# shell__set_variable `variable_name` `variable_value` `export_variable`
##
shell__set_variable() {

    local variable_name="$1"

    # If variable_name is not a valid shell variable_name
    if ! shell__is_valid_shell_variable_name "$variable_name"; then
        logger__log_errors "The variable_name \"$variable_name\" passed to \"shell__set_variable\" is not a valid shell variable name"
        return 1
    fi

    #printf -v "$variable_name" "%s" "$2"
    eval "$variable_name"=\"\$2\"

}

##
# shell__copy_variable `output_variable_name` `input_variable_name`
##
shell__copy_variable() {

    local output_variable_name="$1"
    local input_variable_name="$2"

    # If variable_name is not a valid shell variable_name
    if ! shell__is_valid_shell_variable_name "$output_variable_name"; then
        logger__log_errors "The output_variable_name \"$output_variable_name\" passed to \"shell__copy_variable\" is not a valid shell variable name"
        return 1
    fi

    # If variable_name is not a valid shell variable_name
    if ! shell__is_valid_shell_variable_name "$input_variable_name"; then
        logger__log_errors "The input_variable_name \"$input_variable_name\" passed to \"shell__copy_variable\" is not a valid shell variable name"
        return 1
    fi

    eval "$output_variable_name"=\"\$$input_variable_name\"

}

##
# shell__append_to_variable `variable_name` `value_to_append`
##
shell__append_to_variable() {

    local curr_value
    shell__copy_variable curr_value "$1" || return $?
    shell__set_variable "$1" "$curr_value$2" || return $?

}

##
# shell__arg__print_arguments_string `arguments...`
##
shell__arg__print_arguments_string() {

    local arguments_string
    shell__arg__get_arguments_string arguments_string "$@" || return $?
    data__println_string "$arguments_string"

}

##
# shell__arg__get_arguments_string `output_variable_name` `arguments...`
##
shell__arg__get_arguments_string() {

    local output_variable_name="$1"
    shift 1

    local argument
    local __arguments_string
    local i=1

    while [ $# -ne 0 ]; do
        argument="$1"
        __arguments_string="$__arguments_string$i: \`$argument\`"

        if [ $# -gt 1 ]; then
            __arguments_string="$__arguments_string$DATA__NL"
        fi

        i=$((i + 1))
        shift
    done

    shell__set_variable "$output_variable_name" "$__arguments_string" || return $?

}

##
# shell__arg__validate_argument_count `"eq"`|`"lt"`|`"le"`|`"gt"`|`"ge"` `arguments_received` `arguments_actual` `label`
# shell__arg__validate_argument_count `"or"` `arguments_received` `arguments_actual_1` `arguments_actual_2` `label`
# shell__arg__validate_argument_count `"between"` `arguments_received` `arguments_actual_min` `arguments_actual_max label`
##
shell__arg__validate_argument_count() {

    local return_value=0

    case "$1" in
        eq) if [ "$2" -eq "$3" ]; then :; else logger__log_errors "Invalid argument count \"$2\" to \"$4\". Expected \"$3\" argument(s)."; shift 4; return_value=1; fi;;
        lt) if [ "$2" -lt "$3" ]; then :; else logger__log_errors "Invalid argument count \"$2\" to \"$4\". Expected less than \"$3\" argument(s)."; shift 4; return_value=1; fi;;
        le) if [ "$2" -le "$3" ]; then :; else logger__log_errors "Invalid argument count \"$2\" to \"$4\". Expected less than or equal to \"$3\" argument(s)."; shift 4; return_value=1; fi;;
        gt) if [ "$2" -gt "$3" ]; then :; else logger__log_errors "Invalid argument count \"$2\" to \"$4\". Expected greater than \"$3\" argument(s)."; shift 4; return_value=1; fi;;
        ge) if [ "$2" -ge "$3" ]; then :; else logger__log_errors "Invalid argument count \"$2\" to \"$4\". Expected greater than or equal to \"$3\" argument(s)."; shift 4; return_value=1; fi;;
        or) if [ "$2" -eq "$3" ] || [ "$2" -eq "$4" ]; then :; else logger__log_errors "Invalid argument count \"$2\" to \"$5\". Expected \"$3\" or \"$4\" argument(s)."; shift 5; return_value=1; fi;;
        between) if [ "$2" -ge "$3" ] && [ "$2" -le "$4" ]; then :; else logger__log_errors "Invalid argument count \"$2\" to \"$5\". Expected between \"$3\" and \"$4\" (inclusive) argument(s)."; shift 5; return_value=1; fi;;
        *) logger__log_errors "The comparison \"$1\" while running \"shell__arg__validate_argument_count\" is invalid"; return 1;;
    esac

    [ $return_value -eq 0 ] && return 0

    if [ $# -gt 0 ]; then
        shell__arg__print_arguments_string "$@" || return $?
    fi

    return $return_value

}

##
# logger__log_errors `log_string...`
##
logger__log_errors() { data__printfln_string "$@" 1>&2; }

You can use it like

android__intent__add_intent_extra __args_string "String" "com.termux.string.extra" "$value" 0 1 || return $?
android__intent__add_intent_extra __args_string "String[]" "com.termux.string_array.extra" "$value" 1 0 || return $?

android__intent__validate_is_intent_extra_boolean "bool.extra value" "$value" || return $?
android__intent__add_intent_extra __args_string "boolean" "com.termux.bool.extra" "$value" 1 0 || return $?
tareksander commented 2 years ago

I just had another idea:
When the Termux process is killed, all subprocesses are killed with it.
That means the Termux application process is always running when programs run. If you can merge am into the main Termux application and use a UNIX socket to transfer the arguments, you don't have to create an extra DalvikVM for am and the speed is also fast. That would also make all other am commands faster, and not just the Termux:API scripts.

I got that working, but I had to set the targetSdkVersion from 28 to 27. Termux still compiles and works, but I don't know enough about the app code, so I don't know if that could have any other side-effects. I also integrated ArgumentTokenizer.java to parse the arguments for am. I made a quick python implementation of am using the socket and it has the same performance as the python script I wrote for testing the socket with Termux:API.

agnostic-apollo commented 2 years ago

If you can merge am into the main Termux application

That is a good idea. I was also planning to include termux-api kinda socket connections in termux-app for future shell APIs, so this can be useful. The termux-api.c ideally shouldn't hard code to only TermuxApiReceiver.

That would also make all other am commands faster, and not just the Termux:API scripts.

That's great.

I had to set the targetSdkVersion from 28 to 27.

Is is because world readable access is not available for sockets when using targetSdkVersion >= 28 mentioned here., or something else? Check logcat for avc denial entries. And we can't downgrade. Android even wouldn't allow updating termux-app with lower targetSdkVersion and users would have to uninstall and reinstall the app. But if it is the same reason, then access should be there since both app processes will have same user id.

If access is not there, then what can be done is termux-app starting its own listener for termux-app specific APIs like termux-setup-storage, termux-reload-settings, and others in future and termux-api can run a Service that starts the listener and if its not running, then it can be started. So slow response should only be there for first request or after Service gets killed.

For that you termux-am should not be added to termux-app repo itself. Just publish TermuxAm library with Jitpack like termux-shared is published and add it as a dependency in termux-shared/build.gradle. The ArgumentTokenizer should be added to com.termux.shared.shell package in termux-shared. The TermuxAmListener should be added to com.termux.shared.shell package if you want to release it as Apache 2.0/MIT license for integration with any non GPL code. Otherwise, if you want to release it as GPLv3, then add it to com.termux.shared.termux.shell package. Since termux-app/app already uses and termux-api/app will use termux-shared after termux/termux-api#470 is merged, they can each use the code via the library.

I made a quick python implementation of am using the socket and it has the same performance as the python script I wrote for testing the socket with Termux:API.

That's great then.

My only concern is the socket shouldn't be connectable to other apps, hopefully the uid check is enough and safe.

tareksander commented 2 years ago

My only concern is the socket shouldn't be connectable to other apps, hopefully the uid check is enough and safe.

Citing from the paper "The Misuse of Android Unix Domain Sockets and Security Implications" you linked: "UID/GID checks efficiently prevent unauthorized peers, as UID and GID can never be spoofed or modified."(page 87).
Of course that only holds true for unrooted phones, but if a malicious app has root permissions, sending Intents disguised as Termux will be your last problem.

The only thing that could happen with the UID check is a DOS attack where another app creates an abstract UNIX socket with the same name before Termux is started. Now that I think about it, a filesystem UNIX socket may be a better idea, but Android only has library support for abstract sockets. But since Termux already uses C, you can just make a JNI interface for that. Should that got into the app component or termux-shared?

Is is because world readable access is not available for sockets when using targetSdkVersion >= 28 mentioned here., or something else?

java.lang.RuntimeException: Couldn't find method with matching signature
        at com.termux.termuxam.CrossVersionReflectedMethod.invoke(CrossVersionReflectedMethod.java:164)
        at com.termux.termuxam.IActivityManager.broadcastIntent(IActivityManager.java:194)
        at com.termux.termuxam.Am.sendBroadcast(Am.java:780)
        at com.termux.termuxam.Am.onRun(Am.java:371)
        at com.termux.termuxam.BaseCommand.run(BaseCommand.java:53)
        at com.termux.termuxam.Am.callWithArgsString(Am.java:74)
        at com.termux.app.TermuxAmListener$AmListenerRunnable.run(TermuxAmListener.java:116)
        at java.lang.Thread.run(Thread.java:919)

It seems a method am uses was removed or disabled for targetSdkVersion 28.
I will look if all the reflection and stuff is even necessary and can't be accomplished with the normal API.
If you can use the normal API, targetSdkVersion shouldn't be a problem anymore.

Another solution could be to integrate the socket connection into termux-am instead and have a new flag to start it as a daemon, but I don't know if the Android system would kill it, because it seems to be running as an app process, but has no alive components.

tareksander commented 2 years ago

I managed to rewrite am to use the standard Android APIs and also removed all the commented code and cleaned it up a bit. The only thing I couldn't find a replacement for were the warnings for starting an Activity, but that should not be too bad.

I also made a new branch in my Termux fork called am-integration that uses the rewritten am as a library.

I haven't tested it much, but it seems to work.

agnostic-apollo commented 2 years ago

as UID and GID can never be spoofed or modified

Well, that is a relief. Root is not really a concern, it can directly access all termux data anyways and send intents.

a filesystem UNIX socket may be a better idea, but Android only has library support for abstract sockets. But since Termux already uses C, you can just make a JNI interface for that.

If that can be done and you think its better than do it, then do it, sockets is not really my domain currently. You can place it under [Context.getFilesDir()](https://developer.android.com/reference/android/content/Context#getFilesDir()) since $TMPDIR may not exist at start up or even accessible, like if app is installed on external sd card (some phones have weird issues). Do not use cache dir since it can be wiped by android under low storage conditions and "cleaner apps".

Should that got into the app component or termux-shared?

Yeah, probably better, if you create a class with a path argument, then it should be usable by both termux-app and termux-api.

Couldn't find method with matching signature

The TermuxAm/build.gradle already mentions the reason. The android.content.IIntentSender is marked as @hide which returns null when mIntentSenderSendMethod is set. Such restrictions should be bypassable with AndroidHiddenApiBypass if really needed.

Android system would kill it, because it seems to be running as an app process, but has no alive components.

Yes, android would likely kill it after some time since it would be considered as an empty process. Socket threads must be part of main app process for persistence.

I managed to rewrite am to use the standard Android APIs

Great. I don't know why IActivityManager and CrossVersionReflectedMethod were actually added and if they are necessary. Possibly @michalbednarski knows why.

I haven't tested it much, but it seems to work.

Will probably have to test various shell calls for all am calls activity|service|broadcast for different android versions.

find a replacement for were the warnings for starting an Activity

Warnings shouldn't be removed, they help in debugging, so no need to worry about those.

I also made a new branch in my Termux fork

You should create a static function in LocalSocketListener with arguments for socket name instead of this.

Use Logger.logWarn() for this and also log the Credentials like uid, etc so user can get notified about malicious use. We can also show a notification for that I guess later. Moreover, debug and verbose logs are not logged unless explicitly enabled in settings, only error, warn and info are logged at default log level, so use functions accordingly wherever needed.

https://github.com/termux/termux-app/blob/v0.117/termux-shared/src/main/java/com/termux/shared/logger/Logger.java#L23

michalbednarski commented 2 years ago

I managed to rewrite am to use the standard Android APIs

Great. I don't know why IActivityManager and CrossVersionReflectedMethod were actually added and if they are necessary. Possibly @michalbednarski knows why.

am was using internal APIs because it was running outside of Android app and therefore had no Context, so if you're moving it into app then there is no reason to keep using them

I haven't tested it much, but it seems to work.

Will probably have to test various shell calls for all am calls activity|service|broadcast for different android versions.

If you want (Instrumentation) tests, ones I've had in termux-am might be good starting point (just replace mAm calls with invocations to your implementation)

agnostic-apollo commented 2 years ago

am was using internal APIs because it was running outside of Android app and therefore had no Context, so if you're moving it into app then there is no reason to keep using them

Ah, I see. Thanks.

Thinking more on this, maybe we shouldn't modify TermuxAm and keep it Context free since other apps are apparently using it too themselves. I guess we can just add the modified am to com.termux.shared.am, few classes anyways.

We shouldn't modify $PREFIX/bin/am either since users might be using it without termux app process running, like with root from tasker, etc. The python am wrapper should exist at a different path.

michalbednarski commented 2 years ago

Thinking more on this, maybe we shouldn't modify TermuxAm and keep it Context free since other apps are apparently using it too themselves. I guess we can just add the modified am to com.termux.shared.am, few classes anyways.

We shouldn't modify $PREFIX/bin/am either since users might be using it without termux app process running, like with root from tasker, etc. The python am wrapper should exist at a different path.

While I'm neither for nor against moving am to app, I'd add that TermuxAm is hardcoded to use com.termux callingPackage, so in order to be called from non-Termux uid it needed to be recompiled anyway (I've seen such forks to exist, although don't know if they are used)

agnostic-apollo commented 2 years ago

although don't know if they are used

I am not sure either (I mis-looked earlier), but no point in breaking something that already works and will preserve backward compatibility. It still is useful anyways even if we move. Patching a hard coded string in a fork isn't too big an issue. We can also create a new branch in TermuxAm if it shouldn't be added to termux-shared but then it will still need to be published as a library to jitpack or something. Although, I am not sure of termux-shared future either as a single library since it has grown too much at this point, lot of changes have been made locally which I haven't pushed yet.

tareksander commented 2 years ago

We shouldn't modify $PREFIX/bin/am either

I also think we should also keep am as a separate program, at least until the new implementation is sufficiently tested.

Warnings shouldn't be removed, they help in debugging, so no need to worry about those.

Sorry, I meant the warnings are only possible for the old version of am, because the Android API provides no way of knowing what happens once you sent the Intent, except if no Activity is found. So the Warnings like "Activity brought to front", etc. aren't possible.

The next thing I'll do is implement a filesystem Unix socket interface via JNI, and also rewrite the little python script I made as a C program for extra speed.

agnostic-apollo commented 2 years ago

at least until the new implementation is sufficiently tested.

Later too IMO. You can name your wrapper something appropriate, like termux-am or whatever you like.

Sorry, I meant the warnings are only possible for the old version of am

Ah, sorry, should have thought of that. No worries, users can check logcat for that then. The termux-app now on github has inbuilt actions for that too from long hold terminal -> More -> Report Issue.

rewrite the little python script I made as a C program for extra speed.

Yeah, C would be preferred since otherwise would be need to add python as a dependency too and will need to include that in bootstrap as well, which would further increase the size, which without dependencies is like 8MB.

tareksander commented 2 years ago

No worries, users can check logcat for that then.

I checked, at least for "Warning: Activity not started, intent has been delivered to currently running top-most instance." the warning doesn't get logged to logcat by the system, and I suspect for the other warnings it's the same.

I now made a working filesystem UNIX socket interface and tested the rejection and logging of connections from other users by using root in my emulator (I just started the termux bash binary in the adb shell, interestingly netcat segfaulted, but python worked fine, so I just used the script from before).
Connection checking is made in the new filesystem socket interface directly, so you can use the filesystem sockets with other protocols than the simple one I made for calling am.
That also solves the badly implemented timeout support for Android's LocalSocket, you can set a deadline and if the timeout triggers, the read/write fails. That means a connection can only be alive for timeout+deadline time, making an accidental DOS less likely.
To do all that I had to add C support in the termux-shared module, I hope that works with Jitpack. I used ndk build as it's already used in the app module.

Later too IMO

That's also good. Keeping it as a faster but less verbose (missing warnings) version of am used internally to communicate with the app itself and the plugins is fine enough, and makes for less testing cases needed.

ccaapton commented 2 years ago

@tareksander Hi, I'm very excited about this work. Regarding the uid checks for non-termux clients, I think we should allow some other clients to connect if they can be authenticated in some form, for instance, zeromq with curve keypairs . We can add the public key of other allowed clients to some places similar to ssh authorized_keys. It is more capability-based security like instead of UID-style, and the permission model is much more flexible. Would you mind considering this?

tareksander commented 2 years ago

Regarding the uid checks for non-termux clients

Non-Termux apps shouldn't even be able to access the socket file, because it's in the Termux app files.
If you write a null char before the path it would create the socket in the abstract linux namespace accessible for other apps. I guess I can check for that and drop the filesystem checks in that case, so it doesn't give an error. Implementing RSA crypto is not feasable, so I'll only consider that if the android cypro livrary supports it.

But unix sockets shouldn't be used to communicate across apps, Android provides Intents and Services for such things, and those already have capability-based security with permissions.

And besides, other apps can already send commands to Termux to execute with the right permission, that would also bypass the uid check, so you could just use that instead.

ccaapton commented 2 years ago

But unix sockets shouldn't be used to communicate across apps, Android provides Intents and Services for such things, and those already have capability-based security with permissions.

I hope to use termux-api functionality from my linux-deploy chroot environment. It is a full linux environment with vanilla debian/alpine packages. The normal user uid is 60000.

tareksander commented 2 years ago

I hope to use termux-api functionality from my linux-deploy chroot environment. It is a full linux environment with vanilla debian/alpine packages. The normal user uid is 60000.

If you have a full linux environment, what prevents you from running a ssh server in termux and use termux-api over ssh?

agnostic-apollo commented 2 years ago

ssh or RUN_COMMAND intent is the ideal way for other app access. Although, can allow exception for uid 0 since some users run commands with root if it can work.

ccaapton commented 2 years ago

I hope to use termux-api functionality from my linux-deploy chroot environment. It is a full linux environment with vanilla debian/alpine packages. The normal user uid is 60000.

If you have a full linux environment, what prevents you from running a ssh server in termux and use termux-api over ssh?

If other user can access the socket, then I can connect the socket using python directly, instead of wrapping my intentions through ssh subprocess commands. The time cost of ssh connection initialization could be as much as the am call.

agnostic-apollo commented 2 years ago

The time cost of ssh connection initialization could be as much as the am call.

Even with previous design, you couldn't connect with non-termux users. With this design, if uid 0 were to be allowed, you could just drop to root shell once and then run all commands and socket access likely shouldn't be an issue since termux-api app would he creating it, instead of the other way around. And with ssh way, you don't need to recreate ssh connections every time, you can use ControlMaster. It works pretty well. I used to have a ssh based file manager in Tasker and even that worked just fine over remote networks. Of course, here we are talking about must shorter delays.

I am also against other non root and termux users to connect, specially by default. If a separate key pair based authorization can be implemented that is secure, then that could be acceptable, but should be disabled by default.

tareksander commented 2 years ago

I now allow root to access sockets. Users that don't have the uid of Termux or 0 can't access the socket, I tested that.
I also added support for abstract namespace sockets by prepending a "\0" to the socket name, although I don't recommend it, since if a socket with the same name is already there, the socket can't be registered. That could be the case i.e. with Termux forks that forget to change the socket path.

I finished the C socket client. I made it with CMake, so it should build as a termux package.
I'll open PRs for termux-app and termux-packages.
Should I close this PR? The integration of am into Termux should also make the api programs faster, I just need to test them with the new am, but that can be a separate PR.