Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugins without shared UID #2921

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

tareksander
Copy link
Member

@tareksander tareksander commented Aug 5, 2022

This adds a bound service plugins without a shared UID can use.
closes #2654

How it works:

  • Plugins can connect to the service
  • After connecting, they have to register themselves as a client by providing a Binder for callbacks (and for Termux to watch when the plugin process dies, so resources can be released)
  • After that, the plugin can call the other methods of the service

As an alternative to providing a Binder, clients can also specify a bound service Termux will then bind to to call the callback methods. Plugins can use this prevent themselves from getting killed in the background without having to clutter the notification area with another foreground service notification.

Security Model:

  • From the Termux side:
    • The bound service is protected with a new dangerous permission for plugins, so only apps the user granted the permission can use the service
    • All file methods check if the path is in the plugin directory, so no arbitrary file read/write can occur
    • The runTask method is additionally secured with the RUN_COMMAND permission
    • The service keeps track of which plugin started which task, and plugins can only send signals to their own tasks
  • From the plugin side:
    • I made a library that abstracts away the lower level communication with the service a bit and adds security checks
      • Before connecting to the service, it checks if the signature for the Termux app matches one of the google play, github or fdroid signatures.
      • The callback binder checks if the calling app is Termux on the first transaction, and also checks the Termux app signature
      • Callback services can be secured with a new signature permission Termux defines. This ties the signature into the Android permission system. After getting called, the plugin also has to verify the signature of the Termux app, to tie the permission to the actual signature. This is done in the callback binder by default.

I tested the security aspects, paths outside the plugin directory are rejected, absolute paths are interpreted with the plugin directory as the root. Limiting runTask to apps that have the RUN_COMMAND permission also works. Plugins refuse to connect to Termux if the signature doesn't match.

Features:

  • Each plugin gets a directory with its package name under /data/data/com.termux/files/apps/plugins and some methods to manage it
    • Opening files for reading and/or writing
    • Creating and listening on socket files
  • If the plugin has the RUN_COMMAND permission: A new way to run tasks, using pipes for arbitrarily long input and output
    • A way to send signals to the task
    • A callback for when the task exits/is killed

The features are deliberately minimal and designed to work with programs in Termux to use the sockets and files.

Client Perspective

  • The client checks if the filesystem socket exists and is connectable.
    • If not, activate the plugin with am or termux-am broadcast and wait until connection succeeds.
  • Communicate with the plugin however you want through the socket and files in the plugin dir.

Plugin Perspective

  • Plugin is activated through any means, possibly a broadcast by a client package in Termux.
  • Plugin connects to the plugin service in Termux.
    • Plugin specifies a callback binder: The plugin has to manage not getting killed on its own, likely with a foreground service.
    • Plugin specifies callback service: The plugin can specify the binding priority Termux will use to control how likely it it to be killed by Android.
  • The plugin can now set up filesystem sockets to listen to and manipulate files in its plugin directory.
  • If the plugin also has the RUN_COMMAND permission, it can also run commands through the plugin service, with piped std* streams.

Termux Perspective

  • Termux acts as a broker between the plugins and client packages.
  • It manages the filesystem sockets for the plugin (because you can't pass a filesystem server socket to other apps) and forwards connected clients to the plugin.
  • It also provides a new task type for plugins that gets displayed as a task in the UI, with streams forwarded to the plugin, only if the RUN_COMMAND permission is held by a plugin.
  • Care is taken to keep the list of active plugins valid, plugins are removed when the callback binder/service dies, and filesystem sockets are stopped.

@CruelCritic
Copy link

Why hasn't this been merged already?
@agnostic-apollo isn't even needed.

An image showing that Apollo's review is not required.

@agnostic-apollo
Copy link
Member

@agnostic-apollo isn't even needed.

Things specially like this with security issues don't get merged without proper review and testing.

I did review it couple of weeks back and even with a quick look I did, it had quite a few issues, which I need to fix before this can be merged. I also have to push my local changes first, some of which are required by this pull, which I will do in next few days, mostly done with what I had to do, but lot of testing is required due to tonne of changes and some docs updates.

@CruelCritic
Copy link

bruh 💀
ig that's my bad on not knowing how PR properly work.

I thought if it's bad, then it just gets rejected.

@agnostic-apollo
Copy link
Member

I thought if it's bad, then it just gets rejected.

This pull request is not bad that it gets rejected/closed. The design should likely work and overall good work by @tareksander, but needs a deeper look. There are just issues that need to be fixed, initial implementations usually do have issues.

@cgarz
Copy link

cgarz commented Dec 8, 2023

Is this still being considered?

It would be very nice to be able to use termux-gui for my scripts without having to switch from F-Droid. Especially since then I wouldn't need to worry about termux-dialog and the termux/termux-api#297 issue anymore.

@agnostic-apollo
Copy link
Member

This is planned to be available in next major app release.

@khimaros
Copy link

@agnostic-apollo @tareksander is this still happening?

was this fix committed in another PR?

according to termux/termux-gui#4 (comment) this should fix termux/termux-gui#4

@agnostic-apollo
Copy link
Member

Read my comment above. And no, not merged elsewhere.

…o send and receive file descriptors over UNIX sockets as ancillary data.
…signature permission com.termux.permission.TERMUX_SIGNATURE, PluginService bound service for plugins.

The methods for PluginService are defined in AIDL in the new module plugin-aidl. The module can be imported by plugins to get the method definitions. The TERMUX_PLUGIN permission is needed to bind to the PluginService, and external apps have to be enabled in the config.

AIDL methods:
setCallbackBinder and setCallbackService: Used to get a Binder for the PluginService to identify each plugin and get notified when it dies.
runTask: Runs a command as a Termux task in the background if the caller also has the RUN_COMMAND permission.
listenOnSocketFile: Creates a server socket in the app directory for the plugins (TermuxConstants#TERMUX_APPS_DIR_PATH/<package name>) and returns the file descriptor, which can be used for a LocalServerSocket.
openFile: Opens a file in the app directory for the plugin and returns the file descriptor.
…ission error. listenOnSocketFile() now doesn't send the server socket, but sends the client sockets via new socketConnection() callback.
…n not updating for NativeShell tasks. Added taskFinished callback for plugins and signalTask method for plugins to call.
…lback services, fixed callback services, fixed plugin UID accidentally being set to the PID.
if (packageName == null) {
return null;
}
return TermuxConstants.TERMUX_PLUGINS_DIR_PATH + "/" + packageName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cross UNIX_PATH_MAX (108) for long package names of termux forks or external apps. Check TERMUX__APPS_DIR_MAX_LEN and TERMUX__ROOTFS_DIR_MAX_LEN at following links. The sub directory hierarchy needs to change too. I haven't given much thought into it, but it could be handled by using package name itself and seeing if limit will cross for socket file, and if it does, then generate a shorter random identifier. Whatever is chosen on first connection will be saved in shared preferences, and the selected app data directory can be queried by the plugin app.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a major problem: /data/data/com.termux/files/apps/plugins/very.long.termux.plugin.name.with.a.filesystem.socket/socket.sock would be the limit, but maybe we could drop the plugins from the path to get a few more characters, package names are unique anyways. Or move the while thing up and even drop the files. And if it gets extreme you could only use one letter for the socket name instead of something more descriptive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you generate short randomized names instead of the package names, one problem would be that the client binaries also need to get the name. You'd have to put a mapping file somewhere for them to read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TERMUX__APPS_DIR_MAX_LEN is chosen as 84 based on the 12, 13, 14, 16, 17 and 18 examples.

Check them, it will easily cross. And termux forks matter too, so /data/data/very.long.termux.fork.name/ matters too. And /data/user/xx for secondary users or /mnt/expand for adoptable sd are far more easy to cross. I need to think about every possible use case and for any future changes, not just current termux one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd have to put a mapping file somewhere for them to read.

You mean like /data/data/com.termux/shared_prefs/plugin-apps-mappings.xml? :p

I hadn't thought it from that case, but will have to think on it too, lots to think about and do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to know the package name? Don't third party apps already know their package names? They can always export an environment variable for it too.

However, termux app will write to an apps/i/termux/termux-apps.env file for all the variables of the other termux apps whenever a package is installed/uninstalled/updated. Current way of exporting them when creating shell doesn't work for variables for version names, etc as app could get installed/uninstalled/updated after shell was created, so variables are invalid. So shell scripts will source the file when they want to read an app specific variable. I was thinking of adding variables for currently bound third apps too, so one could find the right variable for the uid of the app by comparing against id -u, and then get package name by generating a variable name with the same prefix as uid variable and reading it. A singular variable could also be provided with info for all the apps in some specific format.

Using socket servers to read app variables has a lot of latency, so cannot be used for time sensitive scripts like termux-api ones, lot of the time gained from using termux-am-socket would be lost if two servers are connected to, but sourcing a file is instant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to know the package name? Don't third party apps already know their package names?

So programs inside Termux that use the plugin know where to look. Third party apps running commands is just one use case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then they can do the reverse, find the variable for package name and then its uid and then generate the path, we can provide a helper script for it too. Or a single json variable with package names as keys and nested dict with values like path.

Even a json file could work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of a JSON file, it's more useful in case you don't use bash, so you can't just source the env file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be fine for plugins, not for termux apps itself, as parsing json with jq in an external call and is heavy and a dependency on the jq package, termux-api scripts and others require ms level efficiency, so sourcing is significantly faster. Could add info to both an env file and json file and clients can decide themselves.

@agnostic-apollo
Copy link
Member

There are quite a few other issues, but I do not have time to get into them now, and cannot get distracted with this anyways. Will look into it all after the pre release, this should be there for the main release, I think this design should work, but have to give a deeper look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Plugins without shared UID
6 participants