Encrypt and decrypt a serializable object

Question

I want to make sure that the code I have for encrypting and decrypting a serializable object makes sense and is proper. Does it look right too?

Here's what I have so far:

public static void encrypt(Serializable object, String path) throws IOException, NoSuchAlgorithmException,
            NoSuchPaddingException, InvalidKeyException {

          try {

            // Length is 16 byte
            SecretKeySpec sks = new SecretKeySpec("MyDifficultPassw".getBytes(),
                    "AES/ECB/PKCS5Padding");


            // Create cipher
            Cipher cipher = Cipher.getInstance("AES/ECB/PKCS5Padding");
            cipher.init(Cipher.ENCRYPT_MODE, sks);
            SealedObject sealedObject = new SealedObject(object, cipher);
            // Wrap the output stream
            CipherOutputStream cos = new CipherOutputStream(new BufferedOutputStream(new FileOutputStream(path)), cipher);
            ObjectOutputStream outputStream = new ObjectOutputStream(cos);
            outputStream.writeObject(sealedObject);
            outputStream.close();
        }
        catch(IllegalBlockSizeException e){

            e.printStackTrace();

        }
        }

public static void decrypt(Serializable object, String path) throws IOException, NoSuchAlgorithmException,
            NoSuchPaddingException, InvalidKeyException {

        SecretKeySpec sks = new SecretKeySpec("MyDifficultPassw".getBytes(),
                "AES");

        Cipher cipher = Cipher.getInstance("AES");
        cipher.init(Cipher.DECRYPT_MODE, sks);

        CipherInputStream cipherInputStream = new CipherInputStream( new BufferedInputStream( new FileInputStream(path) ), cipher );
        ObjectInputStream inputStream = new ObjectInputStream( cipherInputStream );
        SealedObject sealedObject = null;
        try {
            sealedObject = (SealedObject) inputStream.readObject();
            TransferData td = (TransferData) sealedObject.getObject( cipher );

        }
          catch (ClassNotFoundException e) {
            e.printStackTrace();
        } catch (IllegalBlockSizeException e) {
            e.printStackTrace();
        } catch (BadPaddingException e) {
            e.printStackTrace();
        }

    }

Answer 1

Some obvious problems that jump into the eye:

• Don't use duplicate string literals, like "MyDifficultPassw" and "AES/ECB/PKCS5Padding". Put them into constants and define them near the top.

• Don't e.printStackTrace(). It's considered bad practice.

• The encrypt and decrypt methods violate the single responsibility principle, because they encrypt / decrypt and at the same time do file I/O. Instead of writing to / reading from a filesystem path, it would be better to work with streams. That would open make them testable too (see below).

• The decrypt method takes a Serializable object that's not used at all. Also, the initialization SealedObject sealedObject = null; is pointless, as the variable is always assigned before use anyway.

It would be slightly better this way:

private static final byte[] key = "MyDifficultPassw".getBytes();
private static final String transformation = "AES/ECB/PKCS5Padding";

public static void encrypt(Serializable object, OutputStream ostream) throws IOException, NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException {
    try {
        // Length is 16 byte
        SecretKeySpec sks = new SecretKeySpec(key, transformation);

        // Create cipher
        Cipher cipher = Cipher.getInstance(transformation);
        cipher.init(Cipher.ENCRYPT_MODE, sks);
        SealedObject sealedObject = new SealedObject(object, cipher);

        // Wrap the output stream
        CipherOutputStream cos = new CipherOutputStream(ostream, cipher);
        ObjectOutputStream outputStream = new ObjectOutputStream(cos);
        outputStream.writeObject(sealedObject);
        outputStream.close();
    } catch (IllegalBlockSizeException e) {
        e.printStackTrace();
    }
}

public static Object decrypt(InputStream istream) throws IOException, NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException {
    SecretKeySpec sks = new SecretKeySpec(key, transformation);
    Cipher cipher = Cipher.getInstance(transformation);
    cipher.init(Cipher.DECRYPT_MODE, sks);

    CipherInputStream cipherInputStream = new CipherInputStream(istream, cipher);
    ObjectInputStream inputStream = new ObjectInputStream(cipherInputStream);
    SealedObject sealedObject;
    try {
        sealedObject = (SealedObject) inputStream.readObject();
        return sealedObject.getObject(cipher);
    } catch (ClassNotFoundException | IllegalBlockSizeException | BadPaddingException e) {
        e.printStackTrace();
        return null;
    }
}

This is only slightly better, it still looks quite messy. But it has the big advantage that now this is testable, for example:

@Test
public void testEncryptDecryptString() throws InvalidKeyException, NoSuchAlgorithmException, NoSuchPaddingException, IOException {
    String orig = "hello";
    ByteArrayOutputStream baos = new ByteArrayOutputStream();
    encrypt(orig, baos);
    ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
    assertEquals(orig, decrypt(bais));
}

@Test
public void testEncryptDecryptPerson() throws InvalidKeyException, NoSuchAlgorithmException, NoSuchPaddingException, IOException {
    Person orig = new Person("Jack", 21);
    ByteArrayOutputStream baos = new ByteArrayOutputStream();
    encrypt(orig, baos);
    ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
    assertEquals(orig, decrypt(bais));
}

static class Person implements Serializable {
    private static final long serialVersionUID = 0;
    private final String name;
    private final int age;

    Person(String name, int age) {
        this.name = name;
        this.age = age;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }
        if (o == null || getClass() != o.getClass()) {
            return false;
        }

        Person person = (Person) o;

        if (age != person.age) {
            return false;
        }
        if (!name.equals(person.name)) {
            return false;
        }

        return true;
    }
    @Override
    public int hashCode() {
        int result = name.hashCode();
        result = 31 * result + age;
        return result;
    }
}

Answer 2

You'd need a cryptographer to tell you if it's really correct. But they just don't care about practical questions. So it's my turn to tell you, you're wrong.

The most important thing: Anyone with an access to both the plaintext and the ciphertext will get an oracle cheaply verifying their password guesses. Normally, all they need to do is encrypt a single block using a tentative password and repeat it until the time runs out or they get lucky. The SealedObject might change the situation or not, I don't know.

What you need is a salt and password strengthening, something like here. It makes it much slower for both you and the adversary to do anything with the password. You don't need to care much as you pay the cost just once, the attacker needs it for every guess.

The other problem is what you're doing with the exceptions. These e.printStackTrace(); are of no use if your program runs in a place you'll never come by and writes happily to /dev/null. Just don't do it.

It looks like you somehow divided the exception into two classes, but your criterion is unclear. Maybe you just got bored enumerating them?

If you believe that an exception kind can't occur, then simply wrap it into a RuntimeException. For example, NoSuchAlgorithmException would be a good candidate as AES must be present.

Haven't you forgotten to return something from decrypt? Creating TransferData just for the fun of creation doesn't look exactly right.