From f22bfc7c3d6747ca68255dd4935de4d9a5d5475c Mon Sep 17 00:00:00 2001 From: Daniel Bleichenbacher Date: Tue, 3 Jul 2018 01:33:32 -0700 Subject: [PATCH] This CL is mainly documenting that Oracle is not fixing CipherInputStream and CipherOutputStream. The test code is slightly refactored. PiperOrigin-RevId: 203082697 GitOrigin-RevId: aa26a3d92f94b5f79780ebc2d0674f47ca0d223d --- .../testcases/CipherInputStreamTest.java | 72 +++++++++--------- .../testcases/CipherOutputStreamTest.java | 74 ++++++++++--------- 2 files changed, 76 insertions(+), 70 deletions(-) diff --git a/java/com/google/security/wycheproof/testcases/CipherInputStreamTest.java b/java/com/google/security/wycheproof/testcases/CipherInputStreamTest.java index 1da4c48..3698e4e 100644 --- a/java/com/google/security/wycheproof/testcases/CipherInputStreamTest.java +++ b/java/com/google/security/wycheproof/testcases/CipherInputStreamTest.java @@ -32,7 +32,25 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** CipherInputStream tests */ +/** + * CipherInputStream tests + * + *

CipherInputStream is a class that is basically unsuitable for authenticated encryption + * and hence should be avoided whenever possible. The class is unsuitable, because the interface + * does not provide a method to tell the caller when decryption failed. I.e. the specification + * now explicitly claims that it catches exceptions thrown by the Cipher class such as + * BadPaddingException and that it does not rethrow them. + * http://www.oracle.com/technetwork/java/javase/8u171-relnotes-4308888.html + * + *

The Jdk implementation still has the property that no unauthenticated plaintext is released. + * In the case of an authentication failure the implementation simply returns an empty plaintext. + * This allows a trivial attack where the attacker substitutes any message with an empty message. + * + *

The tests in this class have been adapted to this unfortunate situation. testEmptyPlaintext + * checks whether corrupting the tag of an empty message is detected. This test currently fails. + * All other tests run under the assumption that returning an empty plaintext is acceptable + * behaviour, so that the tests are able to catch additional problems. + */ @RunWith(JUnit4.class) public class CipherInputStreamTest { static final SecureRandom rand = new SecureRandom(); @@ -157,9 +175,14 @@ public class CipherInputStreamTest { * BouncyCastle claims that this bug is fixed in version 1.51. However, the test below still fails * with BouncyCastle v 1.52. A possible explanation is that BouncyCastle has its own * implemenatation of CipherInputStream (org.bouncycastle.crypto.io.CipherInputStream). + * + * @param tests an iterable with valid test vectors, that will be corrupted for the test + * @param acceptEmptyPlaintext determines whether an empty plaintext instead of an exception + * is acceptable. */ @SuppressWarnings("InsecureCryptoUsage") - public void testCorruptDecrypt(Iterable tests) throws Exception { + public void testCorruptDecrypt(Iterable tests, boolean acceptEmptyPlaintext) + throws Exception { for (TestVector t : tests) { Cipher cipher = Cipher.getInstance(t.algorithm); cipher.init(Cipher.DECRYPT_MODE, t.key, t.params); @@ -185,6 +208,8 @@ public class CipherInputStreamTest { + TestUtil.bytesToHex(result) + " pt: " + TestUtil.bytesToHex(t.pt)); + } else if (result.length == 0 && !acceptEmptyPlaintext) { + fail("Corrupted ciphertext returns empty plaintext"); } } catch (IOException ex) { // expected @@ -192,34 +217,6 @@ public class CipherInputStreamTest { } } - @SuppressWarnings("InsecureCryptoUsage") - public void testCorruptDecryptEmpty(Iterable tests) throws Exception { - for (TestVector t : tests) { - Cipher cipher = Cipher.getInstance(t.algorithm); - cipher.init(Cipher.DECRYPT_MODE, t.key, t.params); - cipher.updateAAD(t.aad); - byte[] ct = Arrays.copyOf(t.ct, t.ct.length); - ct[ct.length - 1] ^= (byte) 1; - InputStream is = new ByteArrayInputStream(ct); - CipherInputStream cis = new CipherInputStream(is, cipher); - try { - byte[] result = new byte[t.pt.length]; - int totalLength = 0; - int length = 0; - do { - length = cis.read(result, totalLength, result.length - totalLength); - if (length > 0) { - totalLength += length; - } - } while (length >= 0 && totalLength != result.length); - cis.close(); - fail("this should fail"); - } catch (IOException ex) { - // expected - } - } - } - @Test public void testAesGcm() throws Exception { final int[] keySizes = {16, 32}; @@ -242,13 +239,14 @@ public class CipherInputStreamTest { final int[] aadSizes = {0, 8, 24}; Iterable v = getTestVectors("AES/GCM/NoPadding", keySizes, ivSizes, tagSizes, ptSizes, aadSizes); - testCorruptDecrypt(v); + boolean acceptEmptyPlaintext = true; + testCorruptDecrypt(v, acceptEmptyPlaintext); } /** - * Unfortunately Oracle thinks that returning an empty array is valid behaviour for corrupt - * ciphertexts. Because of this we test empty plaintext separately to distinguish behaviour - * considered acceptable by Oracle from other behaviour. + * Tests the behaviour for corrupt plaintext more strictly than in the tests above. + * This test does not accept that an implementation returns an empty plaintext when the + * ciphertext has been corrupted. */ @Test public void testEmptyPlaintext() throws Exception { @@ -259,7 +257,8 @@ public class CipherInputStreamTest { final int[] aadSizes = {0, 8, 24}; Iterable v = getTestVectors("AES/GCM/NoPadding", keySizes, ivSizes, tagSizes, ptSizes, aadSizes); - testCorruptDecryptEmpty(v); + boolean acceptEmptyPlaintext = false; + testCorruptDecrypt(v, acceptEmptyPlaintext); } /** Tests CipherOutputStream with AES-EAX if this algorithm is supported by the provider. */ @@ -281,6 +280,7 @@ public class CipherInputStreamTest { getTestVectors(algorithm, keySizes, ivSizes, tagSizes, ptSizes, aadSizes); testEncrypt(v); testDecrypt(v); - testCorruptDecrypt(v); + boolean acceptEmptyPlaintext = true; + testCorruptDecrypt(v, acceptEmptyPlaintext); } } diff --git a/java/com/google/security/wycheproof/testcases/CipherOutputStreamTest.java b/java/com/google/security/wycheproof/testcases/CipherOutputStreamTest.java index 06e8d23..d016941 100644 --- a/java/com/google/security/wycheproof/testcases/CipherOutputStreamTest.java +++ b/java/com/google/security/wycheproof/testcases/CipherOutputStreamTest.java @@ -31,7 +31,26 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** CipherOutputStream tests */ +/** + * CipherOutputStream tests + * + *

CipherOutputStream is a class that is basically unsuitable for authenticated encryption + * and hence should be avoided whenever possible. The class is unsuitable, because the interface + * does not provide a method to tell the caller when decryption failed. I.e. the specification + * now explicitly claims that it catches exceptions thrown by the Cipher class such as + * BadPaddingException and that it does not rethrow them. + * http://www.oracle.com/technetwork/java/javase/8u171-relnotes-4308888.html + * + *

The Jdk implementation has the property that no unauthenticated plaintext is released. + * In the case of an authentication failure the implementation simply returns an empty plaintext. + * This allows a trivial attack where the attacker substitutes any message with an empty message. + * + *

The tests in this class have been adapted to this unfortunate situation. testEmptyPlaintext + * checks whether corrupting the tag of an empty message is detected. This test currently fails. + * All other tests run under the assumption that returning an empty plaintext is acceptable + * behaviour, so that the tests are able to catch additional problems. + */ + @RunWith(JUnit4.class) public class CipherOutputStreamTest { static final SecureRandom rand = new SecureRandom(); @@ -130,8 +149,16 @@ public class CipherOutputStreamTest { } } + /** + * Tests decryption of corrupted ciphertext. The test may accept empty plaintext as valid + * result because of the problem with CipherOutputStream described in the header of this file. + * @param tests an iterable with valid test vectors, that will be corrupted for the test + * @param acceptEmptyPlaintext determines whether an empty plaintext instead of an exception + * is acceptable. + */ @SuppressWarnings("InsecureCryptoUsage") - public void testCorruptDecrypt(Iterable tests) throws Exception { + public void testCorruptDecrypt(Iterable tests, boolean acceptEmptyPlaintext) + throws Exception { for (TestVector t : tests) { Cipher cipher = Cipher.getInstance(t.algorithm); cipher.init(Cipher.DECRYPT_MODE, t.key, t.params); @@ -154,6 +181,8 @@ public class CipherOutputStreamTest { + TestUtil.bytesToHex(decrypted) + " pt: " + TestUtil.bytesToHex(t.pt)); + } else if (decrypted.length == 0 && !acceptEmptyPlaintext) { + fail("Corrupted ciphertext returns empty plaintext"); } } catch (IOException ex) { // expected @@ -161,32 +190,6 @@ public class CipherOutputStreamTest { } } - @SuppressWarnings("InsecureCryptoUsage") - public void testCorruptDecryptEmpty(Iterable tests) throws Exception { - for (TestVector t : tests) { - Cipher cipher = Cipher.getInstance(t.algorithm); - cipher.init(Cipher.DECRYPT_MODE, t.key, t.params); - cipher.updateAAD(t.aad); - byte[] ct = Arrays.copyOf(t.ct, t.ct.length); - ct[ct.length - 1] ^= (byte) 1; - ByteArrayOutputStream os = new ByteArrayOutputStream(); - CipherOutputStream cos = new CipherOutputStream(os, cipher); - cos.write(ct); - try { - // cos.close() should call cipher.doFinal(). - cos.close(); - byte[] decrypted = os.toByteArray(); - fail( - "this should fail; decrypted:" - + TestUtil.bytesToHex(decrypted) - + " pt: " - + TestUtil.bytesToHex(t.pt)); - } catch (IOException ex) { - // expected - } - } - } - @Test public void testAesGcm() throws Exception { final int[] keySizes = {16, 32}; @@ -198,13 +201,14 @@ public class CipherOutputStreamTest { getTestVectors("AES/GCM/NoPadding", keySizes, ivSizes, tagSizes, ptSizes, aadSizes); testEncrypt(v); testDecrypt(v); - testCorruptDecrypt(v); + boolean acceptEmptyPlaintext = true; + testCorruptDecrypt(v, acceptEmptyPlaintext); } /** - * Unfortunately Oracle thinks that returning an empty array is valid behaviour for corrupt - * ciphertexts. Because of this we test empty plaintext separately to distinguish behaviour - * considered acceptable by Oracle from other behaviour. + * Tests the behaviour for corrupt plaintext more strictly than in the tests above. + * This test does not accept that an implementation returns an empty plaintext when the + * ciphertext has been corrupted. */ @Test public void testEmptyPlaintext() throws Exception { @@ -217,7 +221,8 @@ public class CipherOutputStreamTest { getTestVectors("AES/GCM/NoPadding", keySizes, ivSizes, tagSizes, ptSizes, aadSizes); testEncrypt(v); testDecrypt(v); - testCorruptDecryptEmpty(v); + boolean acceptEmptyPlaintext = false; + testCorruptDecrypt(v, acceptEmptyPlaintext); } /** Tests CipherOutputStream with AES-EAX if AES-EAS is supported by the provider. */ @@ -240,6 +245,7 @@ public class CipherOutputStreamTest { getTestVectors(algorithm, keySizes, ivSizes, tagSizes, ptSizes, aadSizes); testEncrypt(v); testDecrypt(v); - testCorruptDecrypt(v); + boolean acceptEmptyPlaintext = true; + testCorruptDecrypt(v, acceptEmptyPlaintext); } }