3

The problem

I'm trying to make a file encryptor for Android(Encryption algorithm is AES). Everything works fine until I try to encrypt/decrypt a large file. For example, when I try to encrypt a 771 MB file, it gives me this error:

E/AndroidRuntime: FATAL EXCEPTION: Thread-6
Process: com.suslanium.encryptor, PID: 27638
java.lang.OutOfMemoryError: Failed to allocate a 536869904 byte allocation with 25165824 free bytes and 254MB until OOM, target footprint 295637424, growth limit 536870912
    at com.android.org.conscrypt.OpenSSLCipher$EVP_AEAD.expand(OpenSSLCipher.java:1219)
    at com.android.org.conscrypt.OpenSSLCipher$EVP_AEAD.updateInternal(OpenSSLCipher.java:1336)
    at com.android.org.conscrypt.OpenSSLCipher.engineUpdate(OpenSSLCipher.java:323)
    at javax.crypto.Cipher.update(Cipher.java:1722)
    at javax.crypto.CipherOutputStream.write(CipherOutputStream.java:158)
    at com.suslanium.encryptor.MainActivity.encryptFileAES_GCM(MainActivity.java:912)
    at com.suslanium.encryptor.MainActivity$18.run(MainActivity.java:794)
    at java.lang.Thread.run(Thread.java:919)

Fragment of my code

public void encryptFileAES_GCM(File file , File fileToSave) throws Exception {
    File keyEncrypted = new File(pathToStorage + "EncryptedKey.enc");
    File IVEncrypted = new File(pathToStorage + "EncryptedKIV.enc");
    if (keyEncrypted.exists() && IVEncrypted.exists()) {
        FileInputStream fis = new FileInputStream(file);
        FileOutputStream fos = new FileOutputStream(fileToSave);
        Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
        SecretKeySpec keySpec = new SecretKeySpec(globalKey, "AES");
        GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(GCM_TAG_LENGTH * 8, globalIV);
        cipher.init(Cipher.ENCRYPT_MODE, keySpec, gcmParameterSpec);
        CipherOutputStream cos = new CipherOutputStream(fos, cipher);
        int b;
        byte[] d = new byte[512];
        while ((b = fis.read(d)) != -1) {
            cos.write(d, 0, b); //Line 912
        }
        cos.flush();
        cos.close();
        fis.close();
    } else {
        throw new Exception("Key or IV does not exist, encryption can't be done. Please create a key first.");
    }
}

I already tried to search for some solutions, but with no luck. I have no idea what's wrong with this code. The buffer size is not too big, but even after decreasing it - nothing happens. Can anyone please help me?

UPDATE

Problem solved. When encrypting large files, I decided to use the following algorithm: split the file into chunks with X size, encrypt these chunks and put them in a zip archive. When decrypting large files, you need to extract the previously encrypted chunks from the archive, decrypt them and merge decrypted chunks into one file.

  • 1
    Indeed strange. Make your buffer bigger. At least 8192 and try with a flush() in the while loop. – blackapps Feb 10 '21 at 09:48
  • Is it the first write() call that causes this? Or after a certain time? – blackapps Feb 10 '21 at 09:52
  • https://stackoverflow.com/questions/57382238/java-cipher-update-does-not-write-to-buffer-when-using-aes-gcm-android-9 – Selvin Feb 10 '21 at 09:57
  • 1. Did you know `CipherOutputStream` uses an internal buffer? 2. I think it is safe to encrypt `16 x N` bytes with the same instance. – Darkman Feb 10 '21 at 10:06
  • @blackapps Already tried 8192, 10*1024*1024, 512, 16 and 8 buffer size. It crashes with any buffer size. flush() in a while loop doesn't help. About write() - no, it crashes only after some time(depending on buffer size). – Michael Zaharov Feb 10 '21 at 10:07
  • You can catch the exception to not let your app crash. Further this is indeed very strange and unexpected. – blackapps Feb 10 '21 at 10:09
  • Try to work with DataInputStream –  Feb 10 '21 at 10:16
  • 1
    Did you check the example by@President James K. Polk in the linked https://stackoverflow.com/a/57384337/8166854 ? He is giving a complete example to avoid the bug in "Conscrypt" and recommends to use Bouncy Castle instead – Michael Fehr Feb 10 '21 at 12:02

1 Answers1

1

NB: Your code is all over the place and looks to be COMPLETELY INSECURE. Even if you do fix this problem. See note below.

You've found a "bug" in conscrypt. Conscrypt is technically fulfilling the requirements as per the stated spec. We can either point fingers at android/google for shipping such a useless excuse for a library, or for the community not to have better libraries, at the spec authors for allowing such a useless excuse to claim they technically don't violate spec, or at conscrypt for writing such a poor implementation. I leave that to you. The upshot is: It doesn't work, it won't work, and anybody you care to complain to, to fix it, will simply point fingers elsewhere.

Specifically, conscrypt will just cache it all and won't perform the encryption until the very end, which is bizarre, but that's how it works. See this SO question/answer for proof and details.

Solutions:

I don't have an android phone to test this for you and I rarely do android dev, which complicates matters - there is no obvious fix here.

  1. Look into using BouncyCastle. android used to ship with it and sort of still does for backwards compatibility, but I'm pretty sure it's not going to be reliably used here, so you'd have to look into shipping the BC library as part of your app. Consider using the BC-specific API (not built on top of javax.crypto), which guarantees that android won't pull a switcheroo on you and pick the broken conscrypt implementation.

  2. Find the code in conscrypt that is written so poorly and send a pull request with a fix. They can point fingers all day long if you file a bug, but an improvement in their code base, all set to go, just press merge - that's harder for them to deny.

  3. Change your code to save in chunks instead. Write a little protocol: files are stored not as an encrypted blob, but as a sequence of cryptounits, where each unit is a length (as a big-endian 32-bit integer), followed by that many bytes of crypto, followed by another unit. A sequence of cryptounits always ends in a 0-length unit, which tells you it's done. To write these, pick an arbitrary size (maybe 10MB: 10*1024*1024), and write data (might as well do it all in memory, I guess - conscrypt will be doing that in nay case), then you know how much data you have which you can then save by writing the length, then writing all the data. Then keep going until your file is done.

NB: Your code is all over the place and looks to be COMPLETELY INSECURE. You have 'keyEncrypted' and 'IVEncrypted', which you then do not use (other than to abort with an exception if these files are not available). Instead, you are doing your crypto with the 'global key', directly.

That means anybody can decrypt anything your app produced, needing nothing except your app apk which is everywhere. Surely that's not what you intended. Perhaps you intended to use the global key to decrypt iv and key, and then use the decrypted iv/key combo to encrypt or decrypt the file? Note that this 'encryptedIV.key' malarkey is security theatre. Even if you fix your bug, it's bad crypto: Somebody using your app sees that you name these files encryptedKey and would then probably assume they are encrypted. That's a misleading name: They are encrypted with public info, and encrypting something with public info is idiotic: What are you trying to accomplish? A 2-bit hacker with 5 seconds to peruse your code (even if you try to obfuscate it) will figure this out, extract the 'globalKey', and decrypt these files just the same.

If the user themselves does not have to enter any passphrase to kickstart some encryption or decryption, and you can't use the android built in security stuff that e.g. asks them to confirm with a fingerprint scan, then accept that anybody with access to the phone's disk contents or the phone itself unlocked CAN read all this stuff, period, and make that clear by not suggesting these key files are encrypted (because, they effectively aren't, given that the only thing they are encrypted with, is publicly known keys, which does not count).

rzwitserloot
  • 85,357
  • 5
  • 51
  • 72
  • Well, I'm not an information security expert, but I have to make a couple of things clear. When the user launches the application for the first time, he must click on the "Generate key" button, then the application asks the user for the password for the keys.After that, a random key and IV are generated, and then they are encrypted with a different algorithm using the user's password and saved to the keyEncrypted and IVEncrypted files.When re-entering the application, the user is prompted for a password to decrypt the key and IV. – Michael Zaharov Feb 10 '21 at 10:55
  • Then the key and IV are decrypted using the password, and the received data is written to globalKey and globalIV.globalKey and globalIV are NOT predefined values ​​in the code. Just for convenience, I do not decrypt the key for each file separately, but only when entering the application. So, to decrypt user data, you need not an apk-file of the application, but two files (one with a key, the other with an IV) and a user's password. – Michael Zaharov Feb 10 '21 at 10:55
  • _then the application asks the user for the password for the keys._ - _the user is prompted for a password to decrypt the key and IV._ this all sounds good. But if you've already done the job of taking the data in these files and decrypting it using the user's entered PW, why are you re-checking that these files exist, and then not use them? – rzwitserloot Feb 10 '21 at 11:05
  • Because the user can click the "Encrypt file" button when the keys have not yet been generated or imported from the storage (when the application is launched first time). In this case, globalKey and globalIV are null and encryption or decryption can't be performed. – Michael Zaharov Feb 10 '21 at 11:16
  • Sounds like it'd be slightly better to check for those fields being `null`, instead of checking for the existence of the file. At any rate, okay - your code was merely a little misleading without that context, but with this context - looks allright at a casual glance. That just leaves the unfortunate case of conscrypt not being able to operate without buffering the entire datastream first. – rzwitserloot Feb 10 '21 at 22:46