jberkel / sms-backup-plus

Backup Android SMS, MMS and call log to Gmail / Gcal / IMAP
https://play.google.com/store/apps/details?id=com.zegoggles.smssync
Apache License 2.0
1.81k stars 498 forks source link

Encryption of data #105

Closed doits closed 11 years ago

doits commented 13 years ago

I'm currently working on a feature to encrypt the data backed up. See https://github.com/doits/sms-backup-plus

This will use the new APG-AIDL-Interface (I'm working on, too... see https://code.google.com/p/android-privacy-guard/issues/detail?id=71, there's also a working branch apg_service).

If you have any comments on my fork, say so. Remember, it is mostly unfinished/untested and only a proof of concept is working for now (encrypting the body of any msg).

jberkel commented 13 years ago

interesting - i'll take a look soon. however users then won't be able to read their sms online ? it might be important to be able to selectively encrypt messages (only a specific group of people).

doits commented 13 years ago

reading encrypted sms would only be possible if they have an extension for decrypting PGP or the encrypted sms could be piped to gnupg (or similar).

But allowing users to select for which contacts they want to encrypt the sms would be a nice addition, too. Will be implemented after everything else is working.

doits commented 13 years ago

okey, finally encrypting and decrypting is working.

I removed the ability to encrypt just with a passphrase, because maintaining this (different passphrases might be set up for different sms) would be very messy - could be possible, but it is not worth it. Asymmetric encryption is generally better nevertheless.

So you can encrypt and decrypt with your private/public key pairs you have in your apg. On encryption the key is used you have set up in options, on decryption the key corresponding to the public key used on encryption is selected.

If you do not enter your key's passphrase on decryption (cancel the dialog), the whole restore (except the ones restored already) is stopped. As an alternative, you have the choice to just skip the restoring of SMS with that key (so following ones unencrypted/with another key can get restored).

If you do not have the required private key, the decryption of sms with that key is skipped, too (like if the user chooses to skip the key), and a dialog pops up informing him before continuing with the restore.

During my coding I saw throwing an exception in SmsRestoreService::importSms does not show the error in the ui and the restore goes on, so on decryption errors the user is not notified about that something went wrong (unlike in CursorToMessage::messageFromMapSms throwing an exception cancels the backup and displays an error). Both cases should be standardized, I suggest stopping and displaying the exception message (and not continuing silently with errors).

Note you need the branch apg_service of apg for this to work for now. Version 1.09 of apg should incorporate the aidl interface, though.

What do you say?

(edited to adapt to my recent changes)

jberkel commented 13 years ago

i installed apg (APG-1.0.8-release), imported a public key but the PGP encryption checkbox in sms backup+ is still disabled. anything else i need to do?

doits commented 13 years ago

unfortunately APG 1.0.8 does not contain the AIDL interface, so it won't work. The upcoming APG release 1.0.9 will include it.

You have to build the apg_service branch of APG yourself, see the source https://code.google.com/p/android-privacy-guard/source/browse/#svn%2Fbranches%2Fapg_service and https://code.google.com/p/android-privacy-guard/wiki/BuildingAPG

If you do not want to go through all that hassle, I can supply a prebuilt apk. Just tell me.

Btw: I cleaned up my fork from debug code on parts that do not touch en/decryption (doits/sms-backup-plus@95fc6999f78193650bc3) - should make going through the code easier.

jberkel commented 13 years ago

ok, i built apg 1.0.9, added a public key, but i'm not able to select it from the menu (https://skitch.com/jberkel/rmg86/5554-emu-2.2)

during backup i then get:

E/SmsBackup+(  381): Internal failure (class org.thialfihar.android.apg.Apg$GeneralException) in APG when encrypting: no  encryption key(s) or pass phrase given
E/SmsBackup+(  381): error during backup 
E/SmsBackup+(  381): com.fsck.k9.mail.MessagingException: could not encrypt body
doits commented 13 years ago

ahh, okay, missed that point in the logic of selecting a key...

currently you can only select public keys you also have the corresponding private key to. That's because if you encrypt your SMS you should be able to decrypt them, too.

So if a user has no private key, the encryption-checkbox should be disabled, too (with a hint showing it is because of no private key).

So you have to generate a private key in APG. I will update my fork tomorrow disabling the checkbox when no private key is found.

doits commented 13 years ago

there it is, updated my fork. Some other small fixes, too.

jberkel commented 13 years ago

ok, i've just successfully performed a backup+restore w/ encryped emails, nice.

random comments:

shouldn't it be possible to decrypt the email with gpg ? it doesn't work:

$ gpg --decrypt -u test@example.com < email  
gpg: CRC error; DF88E3 - 51F164

email is just the entire email, taken from gmail.

there are some minor code things which need fixing / i don't understand - the android.os.SystemClock.sleep calls look suspicious (should be handled with a callback, not a wait loop). also it doesn't need to use smsSync.showDialog to show the dialog - can be done in the same class (see Donations.java for an example)

also, shouldn't the passphrase handling done by agp, via intents? i imagine the flow like this:

1) user presses restore
2) if encryption is enabled, check if pgp key needs passphrase
3) redirect to agp to unlock private key (intent, etc)
4) agp prompts user, control goes back to sms backup+
5) restore starts

also would be nice to move the en/decryption logic out of the main restore() / backup() methods and in their own method - otherwise the code is really hard to read.

pgp_header != "none" doesn't work in Java: you need !pgp_header.equals("none"). Would actually prefer to leave this header out completely if no encryption is used.

doits commented 13 years ago

shouldn't it be possible to decrypt the email with gpg

it is possible, just checked it. Make sure you have no additional chars in the encrypted text you copy from gmail (even spaces matter, I think).

android.os.SystemClock.sleep calls look suspicious (should be handled with a callback, not a wait loop)

The point of this is to completely halt the restore/backup without having to store the state somewhere. Of course you could write functions, variables, etc. to store the current state to continue later on with a callback, but I thought this to be the quickest and easiest method since the sleep-loop will not last long anyway (<1 minute in normal circumstances). It shouldn't have any impact on performance and can always be easily canceled by canceling the current task if it gets hung somehow (did not have this case on my numerous testing yet, but of course real world testing could reveal this).

So why to write a lot of code when it is solvable that easy (and just works)?

also it doesn't need to use smsSync.showDialog to show the dialog - can be done in the same class (see Donations.java for an example)

I checked and tried to adapted it, but wasn't that easy as with Donate.java. The dialogs used are now put in SmsRestoreService.java, though.

also, shouldn't the passphrase handling done by agp, via intents?

the goal of the AIDL-Interface of APG is to do the interaction without any intent (clean and fast, and let the application handle the ui completely).

There is also a intent-interface of APG, so there might be a hybrid sometime (using AIDL like now but an intent to enter the passphrase in APG), but for now this is not possible because there is no internal structure for "unlocking" keys for a application (afaik).

Of course this is a security flaw for now, because you trust the application you want to encrypt with your private key - but if you do not trust the encrypting application anyway, you should not encrypt with it, either. Nevertheless, there might be some better handling of this some time in the future.

also would be nice to move the en/decryption logic out of the main restore() / backup() methods and in their own method - otherwise the code is really hard to read.

I'll see what I can do - you can be happy there is already a wrapper for the AIDL-connection to APG, or else there would be 1000 more lines (or so ;-) )

pgp_header != "none" doesn't work in Java: you need !pgp_header.equals("none"). Would actually prefer to leave this header out completely if no encryption is used.

silly error, corrected. And adapted to leave out the header if not encrypted, too.

doits commented 13 years ago

2) if encryption is enabled, check if pgp key needs passphrase

Unfortunately this does not work 100% (see below why it works for ~95%), because SmsRestoreService only knows if a sms is encrypted and with which key when it sees it (at the point it wants to restore it). So this has nothing to do with the preferences setting (--> you can restore encrypted sms even if you do not want to backup new sms encrypted).

But we might implement it nevertheless if we assume users will encrypt always with the same key (or check if the user only has one selectable key), and a user who has enabled encryption has at least one sms with that key already uploaded when restoring (which is very assumable, I think). This still might get a false positive (asking for a passphrase which is never needed), but would be a real benefit for a user having a lot of unencrypted sms at the beginning and encrypted ones starting somewhere in the middle. And the dialog at the beginning could be skipped by the user, of course, but would pop up again if the key is encountered on restoring (so sms with that key will not be skipped).

jberkel commented 13 years ago

ok, i've integrated your changes - see jberkel/sms-backup-plus@271d87117a88f872ff23. If you need to make more changes please base them off this branch. i've tested it and everything works ok - i've tried to isolate all crypto related changes to prevent problems for the normal use case (i.e. encryption not enabled).

when will the service based AGP be available in the market ? it doesn't make sense to release this if nobody can use it.

doits commented 13 years ago

nice, I'm glad encryption will make it into the project. I've still some changes to do (some better dialogs and about what keys to select), so it might take some time until I think it is completely finished. No changes to encryption/decryption logic though, this should be finished. (Except I did not test it yet with bunch backup/restore feature of smsbackup+ which might break encryption/decryption?)

when will the service based AGP be available in the market ? it doesn't make sense to release this if nobody can use it.

you're right, I'm finishing the support for AIDL at the APG-side, too, but it will also take some time. For now I will try to stay in touch of changes of the master branch to smsbackup+ in the encryption branch - and then it can be released some time after APG 1.09 i released. But don't expect it to happen tomorrow, because APG development (besides of my AIDL work) is really slow these days...

doits commented 13 years ago

I just pushed encryption into the latest version of sms-backup+ again. Did this in a new branch based on your latest master, because merging was really cumbersome. The commit history of my old changes isn't very helpful anyway, so it shouldn't be a hard loss.

Some movement started again in the AIDL-branch of APG, too, so there might be an official version with AIDL-support soon.