BouncyCastle Rijndael 256 Implementation

Question

I modified the implementation from this website to include a salt:

package de.xxx.yyy.main;

import static org.junit.Assert.*;

import org.bouncycastle.crypto.BlockCipher;
import org.bouncycastle.crypto.BufferedBlockCipher;
import org.bouncycastle.crypto.DataLengthException;
import org.bouncycastle.crypto.InvalidCipherTextException;
import org.bouncycastle.crypto.PBEParametersGenerator;
import org.bouncycastle.crypto.digests.SHA3Digest;
import org.bouncycastle.crypto.engines.RijndaelEngine;
import org.bouncycastle.crypto.generators.PKCS12ParametersGenerator;
import org.bouncycastle.crypto.modes.CBCBlockCipher;
import org.bouncycastle.crypto.paddings.PKCS7Padding;
import org.bouncycastle.crypto.paddings.PaddedBufferedBlockCipher;
import org.bouncycastle.crypto.params.ParametersWithIV;
import org.bouncycastle.util.encoders.Base64;
import org.junit.Test;

public class test {

    private char[] password = "0123456789abcdef0123456789abcdef".toCharArray();
    private byte[] salt = "0123456789".getBytes();
    private int iterationCount = 5;

    @Test
    public void testEncryptRijndael() throws DataLengthException, IllegalStateException, InvalidCipherTextException {

        PKCS12ParametersGenerator pGen = new PKCS12ParametersGenerator(new SHA3Digest(256));
        char[] passwordChars = password;

        final byte[] pkcs12PasswordBytes = PBEParametersGenerator.PKCS12PasswordToBytes(passwordChars);
        pGen.init(pkcs12PasswordBytes, salt , iterationCount );     

        BlockCipher engine = new RijndaelEngine(256);
        CBCBlockCipher cbc = new CBCBlockCipher(engine);
        BufferedBlockCipher cipher = new PaddedBufferedBlockCipher(cbc, new PKCS7Padding());

        ParametersWithIV aesCBCParams = (ParametersWithIV) pGen.generateDerivedParameters(256, 256);
        cipher.init(true, aesCBCParams);

        byte[] input = "Hallo ich bin ein Test".getBytes();
        byte[] cipherText = new byte[cipher.getOutputSize(input.length)];

        int cipherLength = cipher.processBytes(input, 0, input.length, cipherText, 0);
        cipher.doFinal(cipherText, cipherLength);

        String result = new String(Base64.encode(cipherText));
        System.out.println("testEncryptRijndael result : " + result);
        assertEquals("cMoMSNMNsikAkLjaheE6iD48Xkfvo7Y6gS8/zroGfHc=",result);
    }

    @Test
    public void testDecryptRijndael() throws DataLengthException, IllegalStateException, InvalidCipherTextException {
        PKCS12ParametersGenerator pGen = new PKCS12ParametersGenerator(new SHA3Digest(256));
        char[] passwordChars = password;

        final byte[] pkcs12PasswordBytes = PBEParametersGenerator.PKCS12PasswordToBytes(passwordChars);
        pGen.init(pkcs12PasswordBytes, salt , iterationCount );     

        BlockCipher engine = new RijndaelEngine(256);
        CBCBlockCipher cbc = new CBCBlockCipher(engine);
        BufferedBlockCipher cipher = new PaddedBufferedBlockCipher(cbc, new PKCS7Padding());

        ParametersWithIV aesCBCParams = (ParametersWithIV) pGen.generateDerivedParameters(256, 256);
        cipher.init(false, aesCBCParams);

        byte[] output = Base64.decode("cMoMSNMNsikAkLjaheE6iD48Xkfvo7Y6gS8/zroGfHc=".getBytes());
        byte[] cipherText = new byte[cipher.getOutputSize(output.length)];

        int cipherLength = cipher.processBytes(output, 0, output.length, cipherText, 0);
        int outputLength = cipher.doFinal(cipherText, cipherLength);
        outputLength += cipherLength;

        byte[] resultBytes = cipherText;
        if (outputLength != output.length) {
            resultBytes = new byte[outputLength];
            System.arraycopy(
                    cipherText, 0,
                    resultBytes, 0,
                    outputLength
                    );
        }

        String result = new String(resultBytes);
        System.out.println("testDecryptRijndael result : " + result);
        assertEquals("Hallo ich bin ein Test", result);
    }
}

Is this a good implementation, or are there some mistakes, logical errors or security risks?

Answer 1

It's good that you used a more interesting test string instead of "value" and that in testEncryptRijndael you replaced assertNotNull with a more strict assertEquals.

Since your tests are different from the originals, it would be better to rename them to testEncryptRijndaelWithSalt and testDecryptRijndaelWithSalt, respectively. And keep the originals too.

In both the original tests and yours I don't see the point of the logging and printing statements. I think you can drop that. They do no harm, but they might invite the bad habit of reading test outputs instead of focusing on adding proper assertions.

There's a lot of duplication in the encryption and decryption cases. Perhaps you could move that logic to a common private helper method. That can reduce possible typing errors.

I don't see the point of this if block:

 if (outputLength != output.length) {
     // ...
 }

The thing is, in a unit test everything should be predictable: you should already know at this point the outcome of that condition. If outputLength should be equal to output.length, then drop the if but keep the block of code. Otherwise delete the whole thing. You can add an assertion for the condition as a sanity check.