2
\$\begingroup\$

Your opinion interests me regarding this program.

This program encrypts a text message using the AES256 algorithm and CBC. It allows the creation of an encrypted message that contains:

  • The salt used for the creation of the AES256 key
  • The salt used for the creation of the derived key used for calculating the HMAC for message integrity verification
  • The initialization vector
  • The size of the message
  • The encrypted message
  • The HMAC for integrity verification

Do you have any security remarks about this code? Does it seem secure to you?

If not, why?

package security;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.DataInputStream;
import java.io.DataOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Arrays;

import javax.crypto.Cipher;
import javax.crypto.Mac;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;

public class Sample {
    

    public static void main(String[] args) throws Exception {

        String password = "12341234@xxxx!'9876";
        
        String msg = "This is the message";
        
        System.out.println("Encrypt '" + msg + "'");

        
        byte[] encrypted = encrypt(msg, password);
        System.out.println(bytesToHex(encrypted));

        String msg2 = decrypt(encrypted, password);
        System.out.println("restultat='" + msg2 + "'");

    }

    public static byte[] encrypt(String message, String password) throws Exception {
        SecureRandom rand = new SecureRandom();
        
        try (ByteArrayOutputStream out = new ByteArrayOutputStream()) {
            try (DataOutputStream dout = new DataOutputStream(out)) {
                try (InputStream in = new ByteArrayInputStream(
                        message.getBytes(StandardCharsets.UTF_8))) {

                    byte[] salt = new byte[8];
                    rand.nextBytes(salt);
                    out.write(salt);

                    byte[] derivatedSalt = new byte[8];
                    rand.nextBytes(derivatedSalt);
                    out.write(derivatedSalt);

                    byte[] iv = new byte[16];
                    rand.nextBytes(iv);
                    out.write(iv);

                    dout.writeLong(message.getBytes().length);
                    
                    byte[] aesKey = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256")
                            .generateSecret(new PBEKeySpec(password.toCharArray(), salt, 60000, 256))
                            .getEncoded();
                    Cipher ci = Cipher.getInstance("AES/CBC/PKCS5Padding");
                    ci.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(aesKey, "AES"), 
                            new IvParameterSpec(iv));

                    Mac hmac = Mac.getInstance("HmacSHA256");
                    byte[] key1 = saltedSHA256(saltedSHA256(aesKey, derivatedSalt), derivatedSalt);
                    hmac.init(new SecretKeySpec(key1, "HmacSHA256"));
                    hmac.update(iv);
                    hmac.update(salt);

                    byte[] ibuf = new byte[8192];
                    int len;

                    while ((len = in.read(ibuf)) != -1) {
                        byte[] obuf = ci.update(ibuf, 0, len);
                        if (obuf != null) {
                            out.write(obuf);
                            hmac.update(ibuf, 0, len);
                        }
                    }

                    byte[] obuf = ci.doFinal();
                    if (obuf != null) {
                        out.write(obuf);
                    }

                    byte[] bmac = hmac.doFinal();
                    out.write(bmac);
                    return out.toByteArray();
                }
            }
        }
    }

    public static String decrypt(byte[] xx, String password) throws Exception {

        try (InputStream in = new ByteArrayInputStream(xx)) {
            try (DataInputStream din = new DataInputStream(in)) {
                try (ByteArrayOutputStream out = new ByteArrayOutputStream()) {

                    byte[] salt = new byte[8];
                    read(in, salt);

                    byte[] derivatedSalt = new byte[8];
                    read(in, derivatedSalt);

                    byte[] iv = new byte[16];
                    read(in, iv);

                    
                    long taille = din.readLong();

                    byte[] aesKey = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256")
                            .generateSecret(new PBEKeySpec(password.toCharArray(), salt, 60000, 256))
                            .getEncoded();

                    Cipher ci = Cipher.getInstance("AES/CBC/PKCS5Padding");
                    ci.init(Cipher.DECRYPT_MODE, new SecretKeySpec(aesKey, "AES"), new IvParameterSpec(iv));

                    byte[] key1 = saltedSHA256(saltedSHA256(aesKey, derivatedSalt), derivatedSalt);
                    Mac hmac = Mac.getInstance("HmacSHA256");
                    hmac.init(new SecretKeySpec(key1, "HmacSHA256"));
                    hmac.update(iv);
                    hmac.update(salt);

                    long tailleToread = (taille / 16 + 1) * 16; 
                    
                    long resteALire = tailleToread;

                    int bufsize = 8192;

                    while (resteALire > 0) {

                        int blockToRead = Math.min(
                                (resteALire < bufsize ? (int) resteALire : bufsize + 500), bufsize);
                        // astuce pour gérer une taille de message long avec une taille de buffer en in

                        

                        byte[] ibuf = in.readNBytes(blockToRead);

                        int len = ibuf.length;

                        resteALire -= len;

                        byte[] obuf = ci.update(ibuf, 0, len);
                        if (obuf != null) {
                            out.write(obuf);
                            hmac.update(obuf);
                        }
                    }

                    byte[] obuf = ci.doFinal();
                    if (obuf != null) {
                        out.write(obuf);
                        hmac.update(obuf);
                    }

                    // recalcul
                    byte[] bmac = hmac.doFinal();

                    byte[] readsha = in.readAllBytes();
                    if (!Arrays.equals(bmac, readsha)) {
                        throw new Exception("HMAC error");
                    }

                    return out.toString(StandardCharsets.UTF_8);
                }
            }
        }

    }

    

    private static void read(InputStream inputStream, byte[] buffer) throws IOException {
        int bufferLength = buffer.length;
        int totalBytesRead = 0;
        int bytesRead;
        while ((bytesRead = inputStream.read(buffer, totalBytesRead, bufferLength - totalBytesRead)) != -1) {
            totalBytesRead += bytesRead;
            if (totalBytesRead == bufferLength) {
                break;
            }
        }
    }

    private static byte[] saltedSHA256(byte[] data, byte[] salt) throws NoSuchAlgorithmException {
        byte[] bytes = new byte[data.length + salt.length];

        System.arraycopy(data, 0, bytes, 0, data.length);
        System.arraycopy(salt, 0, bytes, data.length, salt.length);

        MessageDigest digest = MessageDigest.getInstance("SHA-256");
        return digest.digest(bytes);
    }

    public static String bytesToHex(byte[] bytes) {
        StringBuilder hexStringBuilder = new StringBuilder(2 * bytes.length);
        for (byte b : bytes) {
            hexStringBuilder.append(String.format("%02x", b));
        }
        return hexStringBuilder.toString();
    }
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Next time you're asked for the full code, make sure you add it to the question and not to answers. Now it's a puzzle. \$\endgroup\$
    – Mast
    Commented Jan 19 at 16:43
  • 2
    \$\begingroup\$ I have removed your answers. 1 was the full code belonging in the question, the other was a modified version without telling us why you changed what. As a review. If you do not want to post it as a review but as a new question to be reviewed again, it belongs in a new question and you can ask it by clicking the Ask Question button. If you are uncertain, take a look at How to Ask and How to post a follow-up question. \$\endgroup\$
    – Mast
    Commented Jan 19 at 16:57

2 Answers 2

5
\$\begingroup\$

The main thing to be wary of is that this kind of ciphertext can be cracked "offline", i.e. at the convenience of the adversary using as many machines as possible. That means that bad passwords are immediately vulnerable even though PBKDF2 is used to derive the data encryption & MAC keys. If possible it should be avoided.

Security / Cryptography

When it comes to the cryptography, the following can be noted:

  • Instead of inventing your own AEAD cipher, it is probably better and less error prone to use AES in GCM mode. There are reasons to prefer HMAC in some cases, but generally those are rather specific.
  • The number of iterations is an important security parameter, which should be clearly documented. It should definitely be higher than 60000 in most cases, more towards a million.
  • If there is a random salt for each encryption then you will get a different key for each encryption; using a different IV isn't really required (it can remain as an all-zero IV).
  • It may be a better idea to use public key encryption and signing instead of using secret keys. That would allow you to protect the plaintext with a real key instead of a password (a password can be used to protect the private keys).
  • There is no reason to call saltedSHA256 twice, it has no security consequences.
  • HMAC forgets to include the plaintext size in the calculation; as it is an adversary could alter the size after which decryption fails (currently they probably need to remain within the block though).

Note that saltedSHA256 basically implements a Key Derivation Function. There are official key derivation functions such as HKDF.

Conciseness

Clearly the code can be more concise, for instance:

  • A single try can be used to control multiple streams at once, e.g.
    try (var s1 = new Stream(); var s2 = new Stream(s1)) {
        // code goes here
    }
    
  • Instead of performing the cipher and HMAC update yourself you could have used CipherOutputStream and DigestOutputStream.
  • In modern Java versions you can use the var keyword, e.g. var ba = new ByteArrayOutputStream().
  • Instead of creating a buffer etc. it is possible to use InputStream.transferTo(OutputStream out)
  • But while we are at it, we don't need any InputStream, if we have CipherOutputStream we can just write the encoded message directly.
  • Instead of using ByteArrayOutputStream simply use DataOutputStream#write, then it is possible to use var out = new DataOutputStream(new ByteArrayOutputStream()) (far out, eh?).
  • The size of the ciphertext can easily be predicted. As the result is being returned in a byte array, it makes more sense to use ByteBuffer or byte[]. The good thing about ByteBuffer is that Cipher can use it as output buffer, but that it also has put(byte[]) and putLong(long) methods defined for it. So the encode method doesn't require streaming anyway.

Code

  • Some variable names could be improved, e.g. cipher instead of ci.
  • The exception handling basically lets the user handle all (checked) exceptions; it is better to turn most of these into RuntimeException instances, see here how to handle the various exceptions.
  • Java can only destroy strings by using garbage collection, and normally Java doesn't destroy string instances. A password is often shown as using char[] because the char values in the array can be overwritten by methods such as Arrays.fill, which would usually destroy the contents. If the method takes a String that using a char[] becomes pointless.

Recommendations

These are recommendations for creating a streaming implementation, as mentioned in the comments as goal (next time include that information in the question text):

  • If the code needs to use GCM then it does limit to 2 GiB of data due to the array size. For file encryption HMAC can be used, but it makes more sense to e.g. copy the ideas within DigestOutputStream and DigestInputStream to allow for streaming in that case.

  • At a very minimum make the password handling as robust as possible (e.g. indicate weak passwords and/or recommend strong ones).

  • Somehow register the iteration count with the ciphertext so it can be upgraded later.

  • Include as much information in the HMAC calculation as possible.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you for all your feedback. The ultimate goal of this program is to replace the 'String' with a file. That's why I am using AES (file encryption). I'm not using GCM mode because it seems to not work in Java beyond 2GB (I will retry). What would you recommend as a replacement for PBKDF2 ? The HMAC is calculated to compute the hash in MAC_THEN_ENCRYPT mode. An enhancement will be made to also allow the use of a key. I will try HKDF. I will soon post an improved version of this program for file encryption. I hope I can count on your very insightful comments. Thanks \$\endgroup\$
    – b210205
    Commented Jan 19 at 22:21
  • \$\begingroup\$ When you're replacing String with a file you could still use buffers: memory mapped IO is supported in Java, but it is not so common to use it. I'm not really proposing PBKDF2, but to make the iteration count configurable - this may require storing the iteration count or at least a version number with the data. But the remark is more that password encryption does inherently come with problematic security as the password is often rather weak. HMAC is slow compared to GCM but you are using it correctly. \$\endgroup\$ Commented Jan 20 at 11:09
  • \$\begingroup\$ Strike that, there is an error as you forgot to include the size of the plaintext in the calculation. Added the issue to the cryptographic section. \$\endgroup\$ Commented Jan 20 at 11:15
3
\$\begingroup\$
dout.writeLong(message.getBytes().length);

Is buggy, giving a possibly wrong length. Forgotten is the Charset used previously. Also dout is only used for this conversion, jumping into the out.

So use a variable for the message bytes. Clear the bytes when you do not want those bytes to linger around on the heap.

Convert the length separately without DataOutputStream. Or alternatively only use dout. (My personal opinion about the mixed usage.)

You may change try (A) { try (B) { to try (A; B) {.

I think someone else may be a better judge of the security aspect. As message is a UTF-8 formatted textual String with an int length, but with writing a long, there is an unnecessary zero byte padding. Or a longer non-String message was intended.

I personally share your hesitancy w.r.t. to security using this encryption.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Additional code was posted which may or may not influence your answer. I hope there is no invalidation going on, a judgement call was made. \$\endgroup\$
    – Mast
    Commented Jan 19 at 16:54
  • \$\begingroup\$ Happy with the given other answer, also picking up ByteBuffer and transferTo. \$\endgroup\$
    – Joop Eggen
    Commented Jan 23 at 22:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.