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

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

Open
7 tasks done
EduardDurech opened this issue Sep 6, 2022 · 41 comments · May be fixed by #556 or termux/termux-api-package#161
Open
7 tasks done

Comments

@EduardDurech
Copy link

EduardDurech commented Sep 6, 2022

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
Copy link
Author

EduardDurech commented Sep 6, 2022

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
Copy link
Member

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

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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 termux/termux-packages#7227 (comment). 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

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

agnostic-apollo commented Sep 13, 2022

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

Welcome.

@EduardDurech

This comment was marked as resolved.

@EduardDurech

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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

This comment was marked as resolved.

@EduardDurech

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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
Copy link
Author

Code is available

@EduardDurech
Copy link
Author

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

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

agnostic-apollo commented Sep 30, 2022

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.

protected static void authenticateWithFingerprint(final FragmentActivity context, final Intent intent, final Executor executor) {

postFingerprintResult(context, intent, fingerprintResult);

@EduardDurech

This comment was marked as resolved.

@EduardDurech

This comment was marked as resolved.

@EduardDurech

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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 termux/termux-app@ac32fbc and termux/termux-tasker@d9a172d

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

This comment was marked as resolved.

@EduardDurech

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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

This comment was marked as resolved.

@EduardDurech

This comment was marked as resolved.

@EduardDurech
Copy link
Author

EduardDurech commented Oct 18, 2022

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

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

@EduardDurech

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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?

@EduardDurech

This comment was marked as resolved.

@EduardDurech

This comment was marked as resolved.

@agnostic-apollo
Copy link
Member

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().

postFingerprintResult(context, intent, fingerprintResult);

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. :)

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