search

termux / termux-api

Termux add-on app which exposes device functionality as API to command line programs.
https://f-droid.org/en/packages/com.termux.api/
2.22k stars 443 forks source link

Implement Encrypt and Decrypt in KeyStore [+more KeyStore support] #550

Open EduardDurech opened 2 years ago

EduardDurech commented 2 years ago

Feature description Implement encrypt and decrypt in the termux-keystore (currently it only supports signing and verifying) via Cipher

This would enable passcodes, secrets, et cetera to be stored in the Android KeyStore, an example would be for automatic decryption of an rclone config file without storing the password in a text file (e.g. encrypted by gpg) using rclone's --password-command, and would enable easy integration with the FingerprintAPI/Biometric Authentication, which would resolve #246 and would also be more convenient than a passphrase or using pass (possibly more secure)

Reference implementation

  1. Using the KeyGenParameterSpec.Builder with PURPOSE_ENCRYPT | PURPOSE_DECRYPT, as well as examples of encrypting and decrypting a text with Cipher (this example would need to store the IV)
  2. LokileCrypt is an implemented example of Android KeyStore supporting encryption/decryption, it merges the encrypted data and a random IV header, as already supported with cipher.getIV() which may be preferable so the IV is not stored separately. termux-keystore can also set a constant IV using IVParameterSpec but not preferable or derived from the alias, secret, such as what rclone does
  3. How to get key from keystore on successful fingerprint auth
  4. Android Fingerprint API Encryption and Decryption
EduardDurech commented 2 years ago

I will contribute to this as much as I can; however, I am not an Android Engineer nor do I usually use Java, @agnostic-apollo would you be able to assist/refactor my code? As well so it fits nicely into KeystoreAPI Thanks

agnostic-apollo commented 2 years ago

I am not an Android Engineer nor do I usually use Java

Directly working on encryption seems like a great way to start learning! :D

It's been a while since I looked/used keystore apis, so it will require me to research things as well. Encryption stuff needs to be handled with care and proper knowledge. It would also be good to have an external review as well from a good encryption apps dev as well, since its not my domain currently.

But yes, I can try to assist. Any refactoring will have to be done after next termux-app release, since that's a priority right now.

You can open a pull request and we can see.

Thanks

EduardDurech commented 1 year ago

Directly working on encryption seems like a great way to start learning! :D

Exactly what I've been up to ;)

@agnostic-apollo would you happen to remember if you had issues with using the "AndroidKeyStore" PROVIDER such as in the key .getInstance calls https://github.com/termux/termux-api/blob/6e80b07f66a6b959a8b92e91daaf52ac30b371c0/app/src/main/java/com/termux/api/apis/KeystoreAPI.java#L227

I keep getting java.security.NoSuchProviderException: no such provider: AndroidKeyStore apparently this happens with unit tests (regardless of using Roboletric, such as in my case using termux with ecj and dx).

I have tried so many different ways of circumventing it, that has been the better part of 2 days, and I cannot get a security provider implemented via Termux and ECJ, I've tried using BouncyCastle, custom providers, using GCMParameterSpec or IVParameterSpec directly, and all I can find for solutions to these online is to use the provider as "AndroidKeyStore", looping back to the same problem. I cannot run the KeystoreAPI.java generateKey either (removing Termux calls) for the same reason.

For now, I am just using anonymous keys without an alias using Javax Crypto, as Android KeyStore uses as a backend (analogues to Java Security for symmetric encryption it seems), just to test the encryption/decryption ciphers, but in order to use Android's Keystore API for Termux, Biometric Authentication, et cetera for integration with Android, I need to resolve this Provider issue

What is/was your development environment? If I can get the provider working I'm quite confident in getting this implemented

agnostic-apollo commented 1 year ago

termux-api app doesn't have any unit tests so that can't be the reason.

Apps should be built with gradle. Are you building on mobile? Don't have access to pc? I use Android Studio mainly.

For building in termux, check https://github.com/termux/termux-packages/pull/7227#issuecomment-893022283. Some github release urls are inactive now.

Some reference implementation I used many years ago is at

https://github.com/agnostic-apollo/FTP/blob/master/app/src/main/java/com/allonsy/android/ftp/KeyHelper.java

https://github.com/agnostic-apollo/FTP/blob/master/app/src/main/java/com/allonsy/android/ftp/FTPLab.java

EduardDurech commented 1 year ago

I have a PC, just wanted access to my phone's hardware-backed keystore for testing. Again, not familiar with Android development so not sure if it's possible to do this in Android Studio over ADB, although this isn't mandatory, I just thought it would be easier to test e.g. Biometric Authentication, I don't know how these are handled in Studio. Same reason I've not used gradle as I rarely use it and just used ECJ in Termux, but I assume there's a reason Android developers use the tools they do :-) So will migrate the project over

Your KeyHelper code is similar to the other implementations of e.g. AES that I've been seeing, and in the Documentation, I just have a problem getting the AndroidKeyStore provider working as you have here

KeyGenerator keyGenerator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, AndroidKeyStore);

agnostic-apollo commented 1 year ago

Play button in android studio will build and install the app on your phone, then you can test however you like.

Code will be similar since it would have been copied and modified a bit.

EduardDurech commented 1 year ago

termux-api app doesn't have any unit tests so that can't be the reason.

BTW by "unit testing" I meant a stand-alone Java code that I built in ECJ on Termux, I took the generateKey method, removed the Termux references, et cetera, just to see if I could use the same Java and Android Keystore API calls, as termux-keystore works, but referencing those libraries in a stand-alone code seems to be an issue

Play button in android studio will build and install the app on your phone, then you can test however you like.

Sounds good, but I should rebuild Termux-API and Termux, right? So they are signed by the same source?

agnostic-apollo commented 1 year ago

Without more info on code content and design and how you building and running, can't comment. Any ECJ issues is not something I have time to deal with anyway.

If you are using Github action builds or releases, all apps will have same key as local debug builds sourced from dev_keystore.jks.

EduardDurech commented 1 year ago

I was using the FDroid builds, but I've switched over and got everything running via Github builds in Android Studio, tested and working

Just a quick question, I'd prefer the "Apply Code Changes" in Android Studio to rebuilding every time, but I get Error Changes were not applied. Class not found: com.termux.api.apis.KeystoreAPI$1 Reinstall and restart app

From what I read online, this is a problem with older Gradle versions, but updating to 7.2.2 does not resolve it. Any ideas off the top of your head?

Thank you so much for your help, that should be the last of my questions and now I can implement this

agnostic-apollo commented 1 year ago

Click app drop down next to play button -> Edit Configuration -> Enable Always install with package manager

Welcome.

EduardDurech commented 1 year ago

Have to exit Termux and have Termux:API open while doing it for it to work, assuming it's a conflict

EduardDurech commented 1 year ago

@agnostic-apollo this is nearly done, I am just trying to support an option to store the encrypted data in SharedPreferences but am not familiar with the whole Context part of Android, for example in FTPLab it seems that it is passed to the constructor, but from where?

Let's say given this snippet

public class KeystoreAPI {
    public static void onReceive(TermuxApiReceiver apiReceiver, Intent intent) {
        switch (intent.getStringExtra("command")) {
            ....
            case "encrypt":
                encryptData(apiReceiver, intent);
                break;
        }
    }

    ...

    public static void encryptData(TermuxApiReceiver apiReceiver, final Intent intent) {
        ResultReturner.returnData(apiReceiver, intent, new WithInput() {
            @Override
            public void writeResult(PrintWriter out) throws ...{
                ...
                encryptedData = ...
            }
        }
    }
}

Would it make the most sense to:

or

Then I should be able to ??

                encryptedData = ...
                SharedPreferences sharedPref = context.getPreferences(Context.MODE_PRIVATE);
                SharedPreferences.Editor editor = sharedPref.edit();
                editor.putString("FooPrefName", encryptedData);
                editor.apply();
                ...
agnostic-apollo commented 1 year ago

for example in FTPLab it seems that it is passed to the constructor, but from where?

Different places like https://github.com/agnostic-apollo/FTP/blob/068f9d57247cc70609d612bd6272fcc916b5ffd4/app/src/main/java/com/allonsy/android/ftp/FTPListFragment.java#L170

Yes, you must use it. Rebase against master branch for 9e72088 and then you should be able to do something like following. The last true is for commitToFile immediately, some phones have issues with saving shared preferences so it must be done, otherwise will lose data. The key name would likely vary if clients want to store different data under different keys, but should prefix them with keystore_api__encrypted_data__<client_key>. Make sure client key is not empty, and possibly matches ^[a-zA-Z0-9_-]+$

TermuxAPIAppSharedPreferences preferences = TermuxAPIAppSharedPreferences.build(context);
if (preferences == null) return; // log and return error
SharedPreferenceUtils.setString(preferences.getSharedPreferences(), key, data, true);

Include context in onReceive

Yeah, do that.

EduardDurech commented 1 year ago

What I did was add context in the onReceive API call https://github.com/termux/termux-api/blob/b296a0c29eea7f9f9f88eb560e53b088605ad575/app/src/main/java/com/termux/api/TermuxApiReceiver.java#L148-L149

 case "Keystore": 
     KeystoreAPI.onReceive(this, **context**, intent);

then

public class KeystoreAPI {
    public static void onReceive(TermuxApiReceiver apiReceiver, **Context context**, Intent intent) {
        switch (intent.getStringExtra("command")) {
            ....
            case "encrypt":
                encryptData(apiReceiver, **context**, intent);
                break;
        }
    }

    ...

    public static void encryptData(TermuxApiReceiver apiReceiver, **final Context context**, final Intent intent) {
        ResultReturner.returnData(apiReceiver, intent, new WithInput() {
            @Override
            public void writeResult(PrintWriter out) throws ...{
                ...
                encryptedData = ...
                SharedPreferences.Editor editor = context.getSharedPreferences("foo", Context.MODE_PRIVATE).edit();
                editor.putString("foobar", encryptedData);
                editor.apply()
            }
        }
    }
}

Is this the best way? This is one thing I am not familiar with so feedback would be appreciated

Edit: You responded while I was typing, will check haha

EduardDurech commented 1 year ago

Okay, so given what you posted, all is the same except

                encryptedData = ...
                TermuxAPIAppSharedPreferences preferences = TermuxAPIAppSharedPreferences.build(context);
                SharedPreferenceUtils.setString(preferences.getSharedPreferences(),
                                "keystore_api__encrypted_data__"+userSuppliedFoo,
                                encryptedData,
                                true);

And for decryption

                TermuxAPIAppSharedPreferences preferences = TermuxAPIAppSharedPreferences.build(context);
                toBeDecrypted = SharedPreferenceUtils.getString(preferences.getSharedPreferences(),
                                "keystore_api__encrypted_data__"+userSuppliedFoo,
                                null,
                                true);

It works, so long as this seems proper

agnostic-apollo commented 1 year ago

Looks good, other than static constant. Also make sure to use space between + and items as per convention.

public static final String KEYSTORE_API__ENCRYPTED_DATA_PREFERENCES_SCOPE = "keystore_api__encrypted_data__";

...
KEYSTORE_API__ENCRYPTED_DATA_PREFERENCES_SCOPE + userSuppliedFoo,
EduardDurech commented 1 year ago

Code is available

EduardDurech commented 1 year ago

It would also be good to have an external review as well from a good encryption apps dev as well

Any suggestions? Some of my references were @lokile, @nelenkov, @yakivmospan, and @WithSecureLabs has a Keystore Audit

EduardDurech commented 1 year ago

@agnostic-apollo I'm trying to prompt for authentication when the key is locked by calling FingerprintAPI.onReceive(context, intent); From KeystoreAPI but the subsequent code executes regardless of if FingerprintAPI has been finished (such as initializing cipher)

Any idea how to wait for that receiver to finish before moving on? I've tried creating 

Intent fingerprintIntent = new Intent(context, FingerprintAPI.FingerprintActivity.class);
fingerprintIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
context.startActivity(fingerprintIntent);

with the same problems

Not sure what the easiest way to do this is - create a new receiver? Add activity to manifest? Or do I have to edit FingerprintAPI https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/apis/FingerprintAPI.java#L81

agnostic-apollo commented 1 year ago

You would need to start FingerprintActivity and send it a PendingIntent or special extras so that it sends result back to KeystoreAPI instead of normal behaviour to socket server. Will have to send back result on all error outs and authentication success. Then in KeystoreAPI, do whatever you originally intended to do. That should work.

https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/apis/FingerprintAPI.java#L163

https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/apis/FingerprintAPI.java#L83

EduardDurech commented 1 year ago

I've been trying things for the past 2 days on this and am not able to get the KeystoreAPI to wait, I've been searching a lot online but to no avail, I think I am understanding PendingIntent wrong. Here are some methods I tried:

Not waiting:

Intent fingerprintIntent = new Intent(context, FingerprintAPI.FingerprintActivity.class);
fingerprintIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
PendingIntent pendingIntent = PendingIntent.getActivity(context, 0, fingerprintIntent, 0);
pendingIntent.send();
PendingIntent pendingIntent = PendingIntent.getActivity(context, 0, intent, 0);
pendingIntent.send();

The way I understand Pending Intents is that I initialize the request code and then in the onAuthentication...(...) methods I would do the pendingIntent.send(requestCode), which would start something back in KeystoreAPI. My issue is 1) How to send the Pending Intent, 2) How to "wait" for the request code. of course I could just add in the method arguments of authenticatedWithFingerprint(..., PendingIntent pendingIntent) and then call that from KeystoreAPI but then I have the issue of sending FragmentActivity context https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/apis/FingerprintAPI.java#L163 "Method addObserver must be called on the main thread", checking online this seems to be a whole lot of other issues

I think I am making this far too complicated. But even once I am able to send PendingIntent, how would KeystoreAPI know to wait for the request code? Wouldn't I need to create a new activity within KeystoreAPI, create a new Intent from that class, wrap it with a PendingIntent pendingIntent = ..., send that pendingIntent to FingerprintAPI (previously mentioned problem), then in the onAuthentication...(...) call that pendingIntent.send()? I don't know if this helps what I'm trying to do as right now I try cipher.init(...), if it catches UserNotAuthenticatedException then it tries the fingerprint and retries the block, I need it to wait before retrying:

int count = 0;
do {
    try {
       cipher.init(mode, key);
    } catch (UserNotAuthenticatedException e) {
        if (count < MAX_AUTH_RETRIES) {
            // Fingerprint
        } else {
            Logger.logError(LOG_TAG, String.valueOf(e));
            throw e;
        }
    }
} while (count++ < MAX_AUTH_RETRIES);

So I need it to wait at // Fingerprint, do I need to create a BroadcastReceiver? As I mentioned, biometricPrompt.authenticate(...) seems to create a new thread regardless, which just initiates the continuation on KeystoreAPI before any onAuthentication...(...), so I really do not know how to make KeystoreAPI "wait" or "listen" for the callback

Sorry for the long information, I've been trying numerous things

EduardDurech commented 1 year ago

So the only way so far I can get this to "work" is by adding a new activity to AndroidManifest which is defined in KeystoreAPI as

public static class FooActivity extends FragmentActivity{
  @Override
  public void onCreate(Bundle savedInstanceState) {
      ...
  }
}

where onCreate will call the cipher action

within KeystoreAPI a mutable (so intent extras are inherited, but hopefully you can give more information on this) PendingIntent wraps an Intent created referencing this FooActivity class, which I include as an extra in the original intent sent to FingerprintAPI.onReceive(...)

Intent fooIntent = new Intent(context, FooActivity.class);
PendingIntent pendingIntent = PendingIntent.getActivity(context, 0, fooIntent, 0);

intent.putExtra("pi", pendingIntent);
FingerprintAPI.onReceive(context, intent);

then in the onAuthentication...{...} callback of the BiometricPrompt

PendingIntent pi = (PendingIntent) intent.getParcelableExtra("pi");
pi.send();

which instantiates the FooActivity onCreate

If this seems like the proper/safest way to do it please let me know as it will take a bit of code-editing and before I commit I would like to know if there is a better way, thanks

EduardDurech commented 1 year ago

You would need to start FingerprintActivity and send it a PendingIntent or special extras so that it sends result back to KeystoreAPI instead of normal behaviour to socket server. Will have to send back result on all error outs and authentication success. Then in KeystoreAPI, do whatever you originally intended to do. That should work.

@agnostic-apollo sorry if you are just busy, not sure if my replies pinged you

agnostic-apollo commented 1 year ago

My responses often take days and weeks. I get requests from people everyday and have my own work as well, can't respond immediately to everyone.

In KeystoreAPI to call FingerprintAPI use

public static final String EXTRA_ORIGINAL_INTENT = "original_intent";
public static final String EXTRA_PENDING_INTENT = "pending_intent";
public static final String EXTRA_FINGERPRINT_RESULT = "fingerprint_result";
public static final String EXTRA_RESULT_BUNDLE = "result";

Intent keyStoreAPIIntent = new Intent(context, TermuxApiReceiver.class);
keyStoreAPIIntent.putExtra("api_method", "Keystore");
keyStoreAPIIntent.putExtra(EXTRA_ORIGINAL_INTENT, intent);
PendingIntent pendingIntent = PendingIntent.getBroadcast(context, getLastPendingIntentRequestCode(context), keyStoreAPIIntent, PendingIntent.FLAG_ONE_SHOT);

Intent fingerprintAPIIntent = new Intent(context, TermuxApiReceiver.class);
fingerprintAPIIntent.putExtra("api_method", "Fingerprint");
fingerprintAPIIntent.putExtra(EXTRA_PENDING_INTENT, pendingIntent);
// Put additional extras like title, description, etc
context.sendBroadcast(fingerprintAPIIntent);

In FingerprintAPI to send result to KeystoreAPI use

PendingIntent pendingIntent = intent.getParcelableExtra(KeystoreAPI.EXTRA_PENDING_INTENT);
if(pendingIntent != null) {
    final Bundle resultBundle = new Bundle();
    resultBundle.putSerializable(KeystoreAPI.EXTRA_FINGERPRINT_RESULT, fingerprintResult);

    Intent resultIntent = new Intent();
    resultIntent.putExtra(KeystoreAPI.EXTRA_RESULT_BUNDLE, resultBundle);
    try {
        pendingIntent.send(context, Activity.RESULT_OK, resultIntent);
    } catch (PendingIntent.CanceledException e) {
        // The caller doesn't want the result? That's fine, just ignore
    }
} else {
    postFingerprintResult(context, intent, fingerprintResult);
}

To receive result from FingerprintAPI at start of KeystoreAPI use

final Intent originalIntent = intent.getParcelableExtra(EXTRA_ORIGINAL_INTENT);
if (originalIntent != null) {
   // Must be response from FingerprintAPI
    final Bundle resultBundle = intent.getBundleExtra(EXTRA_RESULT_BUNDLE);
    if (resultBundle == null) {
        Logger.logError(context, "The intent passed to KeystoreAPI() must contain the result bundle at the " + EXTRA_RESULT_BUNDLE + " key.");
        return;
    }
    // Do whatever and then send result back
    ResultReturner.returnData(apiReceiver, originalIntent /* not intent */, out -> {

    }
}

For getLastPendingIntentRequestCode(), check https://github.com/termux/termux-app/commit/ac32fbc53d8e91a5b7e53d08c06b936699d53ca2 and https://github.com/termux/termux-tasker/commit/d9a172d759adf080b6d8e05be0b64df7051699c1

Handle this case and any other potential ones in both apis.

https://github.com/termux/termux-api/blob/master/app/src/main/java/com/termux/api/apis/FingerprintAPI.java#L83

For reference, check https://github.com/termux/termux-tasker/blob/d52f84fa6759ba3efbf6d12c91fc1943b78d33a9/app/src/main/java/com/termux/tasker/PluginResultsService.java

I haven't tested this, but should work.

EduardDurech commented 1 year ago

@agnostic-apollo thank you :) I've been trying this, it does work with slight modifications but it has the issue as my previous suggestion that now I'd have to send the cipher over broadcast. I'm assuming it is safe as it's an explicit intent, but I'd prefer not sending the cipher and encrypted/decrypted data over broadcasts or intent extras when the user specifies the -q quiet flag, but that seems unavoidable using these methods as I'd have to send them from before the pending intent and then to the onReceive call

This is why I wanted to try and create a "lock" that would wait until the FingerprintActivity finished. I have figured out a round-about way but I'd like to know if this follows Java/Android standards

**Did something new, see comment below** from `KeystoreAPI` after the code finds that `cipher.init(...)` returns a `UserNotAuthenticatedException`, I create an `Intent` calling `FingerprintActivity` and start the activity. a `while(fooResult = fooWaiting)` loops until the `onAuthentication...(...)` sets the `KeystoreAPI.fooResult` var to something else. In order to make sure that the while loop doesn't infinitely continue, I call a `timeoutHandler.postDelayed(...)` before Here is the logic ``` public class KeystoreAPI { protected static final String EXTRA_CALLER_FOO = "FOO_CALLED_BY"; protected static final String AUTH_RESULT_WAITING = "AUTH_RESULT_WAITING"; protected static final String AUTH_RESULT_SUCCESS = "AUTH_RESULT_SUCCESS"; protected static string AUTH_RESULT = AUTH_RESULT_WAITING; ... // Also includes AUTH_RESULT_FAILURE, et cetera... ... // The rest as before public static void cipherCall(Context context, ...) { try { cipher.init(...); ... // Returns after cipher successfully initialized } catch {UserNotAuthenticatedException e) { Intent fingerprintIntent = new Intent(context, FingerprintAPI.FingerprintActivity.class); fingerprintIntent.putExtra(EXTRA_CALLER_FOO, "IveBeenCalledByKeystore"); ... // Add other extras fingerprintIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK); context.startActivity(fingerprintIntent); final Handler timeoutHandler = new Handler(Looper.getMainLooper()); timeoutHandler.postDelayed(() -> { if (AUTH_RESULT.equals(AUTH_RESULT_WAITING)) AUTH_RESULT = AUTH_RESULT_FAILURE; }, FingerprintAPI.SENSOR_TIMEOUT); while (AUTH_RESULT.equals(AUTH_RESULT_WAITING) ; ... // Continues after Biometric Prompt or Handler timeout changes AUTH_RESULT // If AUTH_RESULT is successful then retries cipher.init(...) and returns } } } ``` then in `FingerprintActivity` the `BiometricPrompt.AuthenticationCallback() {...}` ``` @Override public void onAuthenticationSucceeded(@NonNull BiometricPrompt.AuthenticationResult result) { if (intent.hasExtra(KeystoreAPI.EXTRA_CALLER_FOO)) KeystoreAPI.AUTH_RESULT = KeystoreAPI.AUTH_RESULT_SUCCESS; else { ... // The rest as before } } ``` And of course handling the other callbacks Is this safe? I think the only potential attack is another application could change KeystoreAPI.AUTH_RESULT, which at worst would either fail the cipher call or try to call the cipher, which would not be successful if fingerprint wasn't anyway. I think it is better than sending ciphers and sensitive data over intent extras, and there is minimal rewriting of existing code My concern is if it is OK programming practice to change global variables of external classes in Java, or if running a busy-wait loop is OK as long as I include a timer (Android would timeout after a few seconds anyway) since it is for a short time Edit: From reading, it seems I should make `protected static string AUTH_RESULT` a `volatile`, if you could confirm

Thanks

EduardDurech commented 1 year ago

Okay, I have found a way to avoid external global variable setting and busy-wait loops

Essentially, I had a context.wait() after calling the fingerprintIntent, which then calls pendingIntent.send(...) and in KeystoreAPI onReceive I call the context.notifyAll()

This seems like a good compromise, but looking for your input ```Java public class KeystoreAPI { ... public static void onReceive(...) { if (intent.hasExtra("FOO_RESULT")) { synchronized(context) { context.notifyAll(); } } ... } ... public static void cipherCall(Context context, ...) { try { cipher.init(...); ... // Returns after cipher successfully initialized } catch {UserNotAuthenticatedException e) { Intent keystoreAPIIntent = new Intent(context, TermuxApiReceiver.class); keystoreAPIIntent.putExtra("api_method", "Keystore"); keystoreAPIIntent.putExtra(EXTRA_ORIGINAL_INTENT, intent); PendingIntent pendingIntent = PendingIntent.getBroadcast(context, getLastPendingIntentRequestCode(context), keystoreAPIIntent, PendingIntent.FLAG_ONE_SHOT); Intent fingerprintAPIIntent = new Intent(context, FingerprintAPI.FingerprintActivity.class); fingerprintAPIIntent.putExtra(EXTRA_PENDING_INTENT, pendingIntent); ... // Add other extras fingerprintIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK); context.startActivity(fingerprintIntent); synchronized (context) { try { context.wait(); } catch (InterruptedException otherE) { // Error handling } } ... // Continues after BiometricPrompt calls KeystoreAPI which notifies .wait() then retries cipher.init } } } ```
Then in `FingerprintActivity` the `BiometricPrompt.AuthenticationCallback() {...}` ```Java @Override public void onAuthenticationSucceeded(@NonNull BiometricPrompt.AuthenticationResult result) { if (intent.hasExtra(KeystoreAPI.EXTRA_PENDING_INTENT)) { PendingIntent pendingIntent = intent.getParcelableExtra(KeystoreAPI.EXTRA_PENDING_INTENT); Intent resultIntent = new Intent(); resultIntent.putExtra("FOO_RESULT", fooResult); try { pendingIntent.send(context, Activity.RESULT_OK, resultIntent); } catch (PendingIntent.CanceledException e) { // The caller doesn't want the result? That's fine, just ignore } } else { postFingerprintResult(...); } } ```
Then, finally, `getLastPendingIntentRequestCode` is ```Java public static int getLastPendingIntentRequestCode(final Context context) { if (context == null) return 0; SharedPreferences preferences = SharedPreferenceUtils.getPrivateSharedPreferences(context, TermuxConstants.TERMUX_API_DEFAULT_PREFERENCES_FILE_BASENAME_WITHOUT_EXTENSION); if (preferences == null) return 0; int lastPendingIntentRequestCode = SharedPreferenceUtils.getInt(preferences, "last_pending_intent_request_code", 0); int nextPendingIntentRequestCode = lastPendingIntentRequestCode + 1; if (nextPendingIntentRequestCode == Integer.MAX_VALUE || nextPendingIntentRequestCode < 0) { nextPendingIntentRequestCode = 0; } SharedPreferenceUtils.setInt(preferences, "last_pending_intent_request_code", nextPendingIntentRequestCode, false); return nextPendingIntentRequestCode; } ```

from PluginUtils and TermuxTaskerAppSharedPreferences, I will add the analogous TermuxTaskerAppSharedPreferences to TermuxAPIAppSharedPreferences instead, with similar logic and adding values to termux-shared

agnostic-apollo commented 1 year ago

Why do you need to wait at all? The shell command will keep on waiting if ResultSender is not called anyways. Once PendingIntent is sent after authentication, and KeystoreApi receives second intent, it will continue on like it were processing the first intent. A postDelayed() itself is a good idea to timeout in case user doesn't authenticate within a certain time or something went wrong and PendingIntent is not received back. The SENSOR_TIMEOUT could default to like 60s but should be configurable from command line where no timeout if value is 0 in case command was started from background and user wasn't active on the device to authenticate in time, although not sure what happens if display is off and fingerprint authentication is started.

EduardDurech commented 1 year ago

Why do you need to wait at all? The shell command will keep on waiting if ResultSender is not called anyways.

Mainly because of the logic that follows before creating and sending the PendingIntent, I have to first create the cipher, try to initialize it, and only if it catches a UserNotAuthenticatedException then it will call FingerprintActivity, wait for it to finish, then retry the cipher initialization loop n times until either a successful cipher initialization (if authorization was successful), or n times did not work

So, if I refactor the code to include everything in the onReceive, it means I would need to send the cipher as an intent extra which onReceive would then call a function to try and cipher.init(...), otherwise I need to re-create the cipher again. The way I sent in the last comment hangs the thread until a BiometricCallback and then continues as normal, so the cipher, sensitive data, et cetera are still in the initial thread, less things having to be sent around between threads and keeps all data and crypto objects in the initial thread

I hope that makes sense, if you agree?

The SENSOR_TIMEOUT could default to like 60s but should be configurable from command line where no timeout if value is 0 in case command was started from background and user wasn't active on the device to authenticate in time

Currently I was using FingerprintAPI.SENSOR_TIMEOUT (=10s), but with the method I sent in the last comment, a postDelayed() is not necessary, although I may want to add a time to the context.wait(fooTimeout) lock

[According to the documentation](https://developer.android.com/reference/androidx/biometric/BiometricPrompt#ERROR_TIMEOUT()) the BiometricPrompt times out on its own ~30s

not sure what happens if display is off and fingerprint authentication is started.

Nor am I :) One of the things I was going to test once I get this all working, but here is a tracker

agnostic-apollo commented 1 year ago

No need to create the cipher again, just send it in PendingIntent and any other required data and skip generation on result intent. Explicit intents are secure and there will be minimal overhead for putting and sending an extra. The intent and arg limits would also apply though.

https://github.com/termux/termux-tasker#arguments-and-result-data-limits

https://www.reddit.com/r/tasker/comments/prro8t/autoshare_crashed_when_i_pasted_the_file_path/

BiometricPrompt times out on its own ~30s

I see but there should be some extra seconds for intents overhead.

There would also be issue of multiple commands requesting Fingerprint authentication at a given time, so some kind of queuing would be required in FingerprintApi, not in KeystoreApi and so timeouts would need to consider that too. Timer could technically be started in queue entry, so that time used by other earlier requests is ignored.

Nor am I :) One of the things I was going to test once I get this all working, but here is a tracker

Cool. I guess that will need to be handled as well as the comment or wait till screen is unlocked.

EduardDurech commented 1 year ago

No need to create the cipher again, just send it in PendingIntent and any other required data and skip generation on result intent. Explicit intents are secure and there will be minimal overhead for putting and sending an extra. The intent and arg limits would also apply though.

Couldn't this be avoided by using the context.wait() and context.notifyAll() though? Or is there a problem with that method? I would also have to send the input data and other input args over intent extras, which is a bit, I'd like to avoid that if unnecessary

agnostic-apollo commented 1 year ago

I don't know if notifyAll() is internally called by android or something for context object. And since you would be passing original intent in PendingIntent anyways, you won't need to do anything else. Your way may work somehow, but you need to start another thread and not keep the TermuxApiReceiver thread on hold, and not really sure what other advantage you would getting.

EduardDurech commented 1 year ago

I think .wait() and .notify() are methods of Object, and context supports it, I've tested that it does indeed work though, it foregoes the need to be sending things in extras is essentially its benefit, I also don't need to handle different calls to cipher in both the onReceive and another cipher call, more consistent logic

you need to start another thread and not keep the TermuxApiReceiver thread on hold

Another thread is started by biometricPrompt.authenticate(...), it is partly why I have to do all this as the Biometric prompt/interface is created by Android in a different thread

One more question in creating getLastPendingIntentRequestCode, since there is no PluginUtils.java for TermuxAPI as there is for TermuxTasker, would you like me to create one? Or include a getLastPendingIntentRequestCode method in one of the existing util classes of TermuxAPI?

agnostic-apollo commented 1 year ago

I think .wait() and .notify() are methods of Object, and context supports it,

I am aware, but since android is passed the same object, there is risk of android calling it. A separate object would be safer.

Another thread is started by biometricPrompt.authenticate(...),

That's in FingerprintApi, not in KeystoreApi, but ResultSender.returnData() does start a thread, so keep logic there.

would you like me to create one?

Yeah, create a util class.

EduardDurech commented 1 year ago

but since android is passed the same object, there is risk of android calling it. A separate object would be safer.

Oh okay, I get what you mean, the documentation suggests to include a while check loop anyway due to spurious wakeup

ResultSender.returnData() does start a thread, so keep logic there.

I am not sure what you mean, right now what I've noticed (not familiar with Android/Java, so just while testing) is that the context.startActivity(fingerprintAPIIntent) in KeystoreAPI starts a thread, as does the bmPrompt.authenticate(...) in FingerprintAPI, are you just suggesting that I wrap the context.startActivity(fingerprintAPIIntent) and stuff preceding, following it in a ResultReturner.returnData(...)? If so, I am currently doing this, or are you suggesting that I call ResultReturner.returnData(...) afterwards? I'm not sure if you're suggesting that I create another thread or if you are just saying that ResultReturner.returnData(...) just needs to return something so it doesn't remain indefinitely on hold (as we do in all the API calls)?

Current logic with `ResultReturner` ```Java private static void encryptData(TermuxApiReceiver apiReceiver, final Context context, final Intent intent) { ResultReturner.returnData(apiReceiver, intent, new WithInput() { @Override public void writeResult(PrintWriter out) throws GeneralSecurityException, IOException, JSONException { String alias = intent.getStringExtra("alias"); String algorithm = intent.getStringExtra("algorithm"); String path = intent.getStringExtra("filepath"); String store = intent.getStringExtra("store"); boolean quiet = (intent.getIntExtra("quiet", 0) == 1); // Get input data Cipher cipher = Cipher.getInstance(algorithm); // cipherCall currently does all the cipher initialization tries, PendingIntent, and FingerprintAPI interactions cipherCall(context, intent, cipher, Cipher.ENCRYPT_MODE, getKey(alias, alg[0], true), null); // encrypting logic // writing to shared preferences, stdout, ... } }); } ```
If you're curious about `cipherCall(...)` ```Java private static void cipherCall(Context context, Intent intent, Cipher cipher, int mode, Key key, AlgorithmParameterSpec spec) throws InvalidKeyException, InvalidAlgorithmParameterException { int count = 0; do { try { if (spec == null) cipher.init(mode, key); else cipher.init(mode, key, spec); break; } catch (UserNotAuthenticatedException e) { if (count < MAX_AUTH_RETRIES) { boolean[] auths = {true, true}; // Temporary Intent keystoreAPIIntent = new Intent(context, TermuxApiReceiver.class); keystoreAPIIntent.putExtra("api_method", "Keystore"); keystoreAPIIntent.putExtra(EXTRA_ORIGINAL_INTENT, intent); PendingIntent pendingIntent = PendingIntent.getBroadcast(context, getLastPendingIntentRequestCode(context), keystoreAPIIntent, PendingIntent.FLAG_ONE_SHOT); Intent fingerprintAPIIntent = new Intent(context, FingerprintAPI.FingerprintActivity.class); fingerprintAPIIntent.putExtra(EXTRA_PENDING_INTENT, pendingIntent); fingerprintAPIIntent.putExtras(...) fingerprintAPIIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK); context.startActivity(fingerprintAPIIntent); synchronized (context) { try { context.wait(); } catch (InterruptedException etmp) { // Error handling } } } else { Logger.logError(LOG_TAG, String.valueOf(e)); throw e; } } } while (count++ < MAX_AUTH_RETRIES); } ```

Also, I think the cleanest thing (would also mean I don't need to add more to KeystoreAPI onReceive(...) would be to do all the PendingIntent .wait() and .notifyall() logic within FingerprintAPI onReceive and FingerprintActivity, this would also make it universal to other calls to it, This would mean that KeystoreAPI.onReceive(...) calls wouldn't return until either a timeout or auth_result (right now that is the same case when TermuxAPIReceiver calls it, due to ReturnData being called, but this would also make it work for non-Broadcast calls)

EduardDurech commented 1 year ago

Cool, I can just do a lock in FingerprintAPI meaning that this can go for any future calls

This way no extra Intents are needed ```diff public class FingerprintAPI { + private static final Object lock = new Object(); + protected static final String EXTRA_LOCK_ACTION = "EXTRA_LOCK_ACTION"; public static void onReceive(final Context context, final Intent intent) { Logger.logDebug(LOG_TAG, "onReceive"); resetFingerprintResult(); FingerprintManagerCompat fingerprintManagerCompat = FingerprintManagerCompat.from(context); // make sure we have a valid fingerprint sensor before attempting to launch Fingerprint activity if (validateFingerprintSensor(context, fingerprintManagerCompat)) { Intent fingerprintIntent = new Intent(context, FingerprintActivity.class); fingerprintIntent.putExtras(intent.getExtras()); fingerprintIntent.setFlags(FLAG_ACTIVITY_NEW_TASK); context.startActivity(fingerprintIntent); + if (intent.getBooleanExtra(EXTRA_LOCK_ACTION, false)) { + synchronized (lock) { + try { + lock.wait(); + } catch (InterruptedException e) { + /// Handle Error + } + } + } } else { postFingerprintResult(context, intent, fingerprintResult); } } protected static void postFingerprintResult(Context context, Intent intent, final FingerprintResult result) { + if (intent.getBooleanExtra(EXTRA_LOCK_ACTION, false)) { + synchronized(lock) { + lock.notifyAll(); + } } else { ResultReturner.returnData(context, intent, new ResultReturner.ResultJsonWriter() { @Override public void writeJson(JsonWriter out) throws Exception { out.beginObject(); out.name("errors"); out.beginArray(); for (String error : result.errors) { out.value(error); } out.endArray(); out.name("failed_attempts").value(result.failedAttempts); out.name("auth_result").value(result.authResult); out.endObject(); out.flush(); out.close(); postedResult = true; } }); } } } ```

And in KeystoreAPI

                    intent.putExtra(FingerprintAPI.EXTRA_LOCK_ACTION, true);
                    FingerprintAPI.onReceive(context, intent);

Doesn't require PluginUtils either, but I sent a PR just so the functionality is there for PendingIntent in case it will be used in the future

EduardDurech commented 1 year ago

@agnostic-apollo all done! I understand it will take some time to review but the interface is all in https://github.com/termux/termux-api-package/pull/161

and thank you so much for all of your help, I learned a lot during this, it is greatly appreciated

EduardDurech commented 1 year ago

@agnostic-apollo actually, one more problem, sorry

When calling the fingerprint fragment it seems to leave an invisible overlay even after finishing, the grey bar seems to be from the Termux:API app, but I am not sure how to "exit" it when it finishes. The screen is unable to be interacted with

I tried calling this.finish() after handleFingerprint() returns to onCreate and this.onBackPressed(), but they don't work. I am not sure what exactly the problem is but I'd like to "close" the Termux:API overlay, calling termux-fingerprint does not do this https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/apis/FingerprintAPI.java#L144-L150 image

Thank you

agnostic-apollo commented 1 year ago

Calling finish() in onCreate() will finish the activity before it or dialog is shown. The finish() call is already made in ResultSender if correct Activity context is passed. Are you saying even master branch shows grey bar or your pull branch?

https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/util/ResultReturner.java#L226

EduardDurech commented 1 year ago

Are you saying even master branch shows grey bar or your pull branch?

My pull branch, I cannot test on master because KeystoreAPI it doesn't call FingerprintAPI, although this is not a problem when calling termux-fingerprint from the CLI, only when calling the new termux-keystore which calls FingerprintAPI.onReceive(...) which then calls the context.startActivity(fingerprintIntent), I think when the FragmentActivity starts is when this starts

https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/apis/FingerprintAPI.java#L81

It actually never goes to activity.finish() but instead https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/util/ResultReturner.java#L224

I assume because the KeystoreAPI is a Receiver and not an Activity

EduardDurech commented 1 year ago
So one thing I can do is add a `ResultReturner` in `postFingerprintResult` ```diff protected static void postFingerprintResult(Context context, Intent intent, final FingerprintResult result) { if (intent.getBooleanExtra(EXTRA_LOCK_ACTION, false)) { lock.lock(); try { condition.signalAll(); } finally { lock.unlock(); + ResultReturner.returnData(context, intent, null); } } else { ResultReturner.returnData(context, intent, new ResultReturner.ResultJsonWriter() { @Override public void writeJson(JsonWriter out) throws Exception { out.beginObject(); out.name("errors"); out.beginArray(); for (String error : result.errors) { out.value(error); } out.endArray(); out.name("failed_attempts").value(result.failedAttempts); out.name("auth_result").value(result.authResult); out.endObject(); out.flush(); out.close(); postedResult = true; } }); } } ```
Or cast `context` to an `Activity` ```diff protected static void postFingerprintResult(Context context, Intent intent, final FingerprintResult result) { if (intent.getBooleanExtra(EXTRA_LOCK_ACTION, false)) { lock.lock(); try { condition.signalAll(); } finally { lock.unlock(); + ((Activity) context).finish(); } } else { ResultReturner.returnData(context, intent, new ResultReturner.ResultJsonWriter() { @Override public void writeJson(JsonWriter out) throws Exception { out.beginObject(); out.name("errors"); out.beginArray(); for (String error : result.errors) { out.value(error); } out.endArray(); out.name("failed_attempts").value(result.failedAttempts); out.name("auth_result").value(result.authResult); out.endObject(); out.flush(); out.close(); postedResult = true; } }); } } ```

Thoughts?

agnostic-apollo commented 1 year ago

Since the intent you are passing to FingerprintAPI does not contain SOCKET_OUTPUT_EXTRA, an exception will be thrown each time by ResultReturner, the second way should work. The ResultReturner started by KeystoreAPI will be a BroadcastReceiver context, hence asyncResult is called, instead of activity, and calling it again in FingerprintAPI would be a nested call and will result in duplicate calls to functions. Although, I currently haven't taken a close look at your design.

Since following will not pass Activity context, add an if (context instanceof Activity) before calling finish().

https://github.com/termux/termux-api/blob/9e72088e8b1112deae67992f1f94029a58a7445d/app/src/main/java/com/termux/api/apis/FingerprintAPI.java#L83

I understand it will take some time to review but the interface is all in termux/termux-api-package#161

Great. Will take a look when I get time to look into termux-api.

and thank you so much for all of your help, I learned a lot during this, it is greatly appreciated

You are very welcome. Good for you. :)

FunctionDJ commented 1 month ago

if i understand correctly, this would allow setups for setting up remote decrypt-root-at-boot (e.g. through dropbear/initramfs) using fingerprint authentication without storing the key (or whatever secret) in regular termux storage. that's what i've been trying to set up and landed here :) what i'd do is set up fingerprint to unlock rbw, a CLI for bitwarden, and then i could do all sorts of unlocks just with fingerprint, which would be awesome.