From a8018783373ff5e5210225069c9919e071597d5e Mon Sep 17 00:00:00 2001 From: Brian Demers Date: Wed, 6 Nov 2019 15:31:09 -0500 Subject: [PATCH] Updates the default Cipher mode to GCM in AesCipherService Adds tests for each mode Fix issue where GCM needs a different AlgorithmParameterSpec implementation (previously IvParameterSpec was used) --- crypto/cipher/pom.xml | 7 + .../apache/shiro/crypto/AesCipherService.java | 23 ++- .../apache/shiro/crypto/JcaCipherService.java | 9 +- .../shiro/crypto/AesCipherServiceTest.groovy | 158 +++++++++++++++--- .../web/mgt/CookieRememberMeManagerTest.java | 28 ++-- 5 files changed, 179 insertions(+), 46 deletions(-) diff --git a/crypto/cipher/pom.xml b/crypto/cipher/pom.xml index 2390a75135..f88d899c90 100644 --- a/crypto/cipher/pom.xml +++ b/crypto/cipher/pom.xml @@ -60,6 +60,13 @@ org.apache.shiro shiro-crypto-core + + + org.bouncycastle + bcprov-jdk15on + 1.64 + test + diff --git a/crypto/cipher/src/main/java/org/apache/shiro/crypto/AesCipherService.java b/crypto/cipher/src/main/java/org/apache/shiro/crypto/AesCipherService.java index 0c2f5cc7f2..13863ecbb7 100644 --- a/crypto/cipher/src/main/java/org/apache/shiro/crypto/AesCipherService.java +++ b/crypto/cipher/src/main/java/org/apache/shiro/crypto/AesCipherService.java @@ -18,13 +18,16 @@ */ package org.apache.shiro.crypto; +import javax.crypto.spec.GCMParameterSpec; +import java.security.spec.AlgorithmParameterSpec; + /** * {@code CipherService} using the {@code AES} cipher algorithm for all encryption, decryption, and key operations. *

* The AES algorithm can support key sizes of {@code 128}, {@code 192} and {@code 256} bits*. This implementation * defaults to 128 bits. *

- * Note that this class retains the parent class's default {@link OperationMode#CBC CBC} mode of operation + * Note that this class retains changes the parent class's default {@link OperationMode#CBC CBC} mode to {@link OperationMode#GCM GCM} of operation * instead of the typical JDK default of {@link OperationMode#ECB ECB}. {@code ECB} should not be used in * security-sensitive environments because {@code ECB} does not allow for initialization vectors, which are * considered necessary for strong encryption. See the {@link DefaultBlockCipherService parent class}'s JavaDoc and the @@ -59,7 +62,7 @@ public class AesCipherService extends DefaultBlockCipherService { * * * {@link #setMode mode} - * {@link OperationMode#CBC CBC}* + * {@link OperationMode#GCM GCM}* * * * {@link #setPaddingScheme paddingScheme} @@ -75,16 +78,28 @@ public class AesCipherService extends DefaultBlockCipherService { * * *

- * * The {@link OperationMode#CBC CBC} operation mode is used instead of the JDK default {@code ECB} to + * * The {@link OperationMode#GCM GCM} operation mode is used instead of the JDK default {@code ECB} to * ensure strong encryption. {@code ECB} should not be used in security-sensitive environments - see the * {@link DefaultBlockCipherService DefaultBlockCipherService} class JavaDoc's "Operation Mode" section * for more. *

- * **In conjunction with the default {@code CBC} operation mode, initialization vectors are generated by + * **In conjunction with the default {@code GCM} operation mode, initialization vectors are generated by * default to ensure strong encryption. See the {@link JcaCipherService JcaCipherService} class JavaDoc for more. */ public AesCipherService() { super(ALGORITHM_NAME); + setMode(OperationMode.GCM); + setStreamingMode(OperationMode.GCM); } + @Override + protected AlgorithmParameterSpec createParameterSpec(byte[] iv, boolean streaming) { + + if ((streaming && OperationMode.GCM.name().equals(getStreamingModeName())) + || (!streaming && OperationMode.GCM.name().equals(getModeName()))) { + return new GCMParameterSpec(getKeySize(), iv); + } + + return super.createParameterSpec(iv, streaming); + } } diff --git a/crypto/cipher/src/main/java/org/apache/shiro/crypto/JcaCipherService.java b/crypto/cipher/src/main/java/org/apache/shiro/crypto/JcaCipherService.java index bb215568e3..b305b02383 100644 --- a/crypto/cipher/src/main/java/org/apache/shiro/crypto/JcaCipherService.java +++ b/crypto/cipher/src/main/java/org/apache/shiro/crypto/JcaCipherService.java @@ -590,13 +590,18 @@ private javax.crypto.Cipher initNewCipher(int jcaCipherMode, byte[] key, byte[] javax.crypto.Cipher cipher = newCipherInstance(streaming); java.security.Key jdkKey = new SecretKeySpec(key, getAlgorithmName()); - IvParameterSpec ivSpec = null; + AlgorithmParameterSpec ivSpec = null; + if (iv != null && iv.length > 0) { - ivSpec = new IvParameterSpec(iv); + ivSpec = createParameterSpec(iv, streaming); } init(cipher, jcaCipherMode, jdkKey, ivSpec, getSecureRandom()); return cipher; } + + protected AlgorithmParameterSpec createParameterSpec(byte[] iv, boolean streaming) { + return new IvParameterSpec(iv); + } } diff --git a/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/AesCipherServiceTest.groovy b/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/AesCipherServiceTest.groovy index f35751e72d..0385420b05 100644 --- a/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/AesCipherServiceTest.groovy +++ b/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/AesCipherServiceTest.groovy @@ -18,6 +18,12 @@ */ package org.apache.shiro.crypto +import org.bouncycastle.jce.provider.BouncyCastleProvider + +import java.security.Security + +import static org.junit.Assert.*; + import org.apache.shiro.codec.CodecSupport import org.apache.shiro.util.ByteSource import org.junit.Test @@ -29,46 +35,146 @@ import static junit.framework.Assert.* * * @since 1.0 */ -public class AesCipherServiceTest { +class AesCipherServiceTest { private static final String[] PLAINTEXTS = [ "Hello, this is a test.", "Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." - ]; + ] + + AesCipherServiceTest() { + Security.addProvider(new BouncyCastleProvider()) + } @Test - public void testBlockOperations() { - AesCipherService aes = new AesCipherService(); + void testBlockOperations() { + AesCipherService cipher = new AesCipherService() + assertBlock(cipher) + } - byte[] key = aes.generateNewKey().getEncoded(); + @Test + void testStreamingOperations() { + AesCipherService cipher = new AesCipherService() + assertStreaming(cipher) + } - for (String plain : PLAINTEXTS) { - byte[] plaintext = CodecSupport.toBytes(plain); - ByteSource ciphertext = aes.encrypt(plaintext, key); - ByteSource decrypted = aes.decrypt(ciphertext.getBytes(), key); - assertTrue(Arrays.equals(plaintext, decrypted.getBytes())); - } + @Test + void testAesGcm() { + assertBlock(OperationMode.GCM) + assertStreaming(OperationMode.GCM) } @Test - public void testStreamingOperations() { + void testCcm() { + assertBlock(OperationMode.CCM, PaddingScheme.NONE, 13 * 8) // 13 bytes + assertStreaming(OperationMode.CCM) + } - AesCipherService cipher = new AesCipherService(); - byte[] key = cipher.generateNewKey().getEncoded(); + @Test + void testCfb() { + assertBlock(OperationMode.CFB) + assertStreaming(OperationMode.CFB) + } + @Test + void testCtr() { + assertBlock(OperationMode.CTR) + assertStreaming(OperationMode.CTR) + } + + @Test + void testEax() { + assertBlock(OperationMode.EAX) + assertStreaming(OperationMode.EAX) + } + + @Test + void testEcb() { + assertBlock(OperationMode.ECB, PaddingScheme.PKCS5) + } + + @Test + void testNone() { + assertBlock((OperationMode) null, null) + } + + @Test + void testOcb() { + assertBlock(OperationMode.OCB, PaddingScheme.NONE, 15 * 8) // 15 bytes + assertStreaming(OperationMode.OCB, PaddingScheme.NONE, 16 * 8) // 16 bytes + } + + @Test + void testOfb() { + assertBlock(OperationMode.OFB) + assertStreaming(OperationMode.OFB) + } + + @Test + void testPcbc() { + assertBlock(OperationMode.PCBC, PaddingScheme.PKCS5) + assertStreaming(OperationMode.PCBC, PaddingScheme.PKCS5) + } + + private static assertBlock(OperationMode mode, PaddingScheme scheme = PaddingScheme.NONE, int ivSize = JcaCipherService.DEFAULT_KEY_SIZE) { + AesCipherService cipher = new AesCipherService() + cipher.setInitializationVectorSize(ivSize) + + if (mode == null) { + cipher.setModeName(null) + } else { + cipher.setMode(mode) + } + + if (scheme == null) { + cipher.setPaddingSchemeName(null) + } else { + cipher.setPaddingScheme(scheme) + } + assertBlock(cipher) + } + + private static assertStreaming(OperationMode mode, PaddingScheme scheme = PaddingScheme.NONE, int ivSize = JcaCipherService.DEFAULT_KEY_SIZE) { + AesCipherService cipher = new AesCipherService() + cipher.setInitializationVectorSize(ivSize) + + if (mode == null) { + cipher.setStreamingModeName(null) + } else { + cipher.setStreamingMode(mode) + } + + if (scheme == null) { + cipher.setStreamingPaddingScheme(null) + } else { + cipher.setStreamingPaddingScheme(scheme) + } + assertBlock(cipher) + } + + private static assertBlock(AesCipherService cipher, byte[] key = cipher.generateNewKey().getEncoded()) { for (String plain : PLAINTEXTS) { - byte[] plaintext = CodecSupport.toBytes(plain); - InputStream plainIn = new ByteArrayInputStream(plaintext); - ByteArrayOutputStream cipherOut = new ByteArrayOutputStream(); - cipher.encrypt(plainIn, cipherOut, key); - - byte[] ciphertext = cipherOut.toByteArray(); - InputStream cipherIn = new ByteArrayInputStream(ciphertext); - ByteArrayOutputStream plainOut = new ByteArrayOutputStream(); - cipher.decrypt(cipherIn, plainOut, key); - - byte[] decrypted = plainOut.toByteArray(); - assertTrue(Arrays.equals(plaintext, decrypted)); + byte[] plaintext = CodecSupport.toBytes(plain) + ByteSource ciphertext = cipher.encrypt(plaintext, key) + ByteSource decrypted = cipher.decrypt(ciphertext.getBytes(), key) + assertTrue(Arrays.equals(plaintext, decrypted.getBytes())) + } + } + + private static assertStreaming(AesCipherService cipher, byte[] key = cipher.generateNewKey().getEncoded()) { + for (String plain : PLAINTEXTS) { + byte[] plaintext = CodecSupport.toBytes(plain) + InputStream plainIn = new ByteArrayInputStream(plaintext) + ByteArrayOutputStream cipherOut = new ByteArrayOutputStream() + cipher.encrypt(plainIn, cipherOut, key) + + byte[] ciphertext = cipherOut.toByteArray() + InputStream cipherIn = new ByteArrayInputStream(ciphertext) + ByteArrayOutputStream plainOut = new ByteArrayOutputStream() + cipher.decrypt(cipherIn, plainOut, key) + + byte[] decrypted = plainOut.toByteArray() + assertTrue(Arrays.equals(plaintext, decrypted)) } } } diff --git a/web/src/test/java/org/apache/shiro/web/mgt/CookieRememberMeManagerTest.java b/web/src/test/java/org/apache/shiro/web/mgt/CookieRememberMeManagerTest.java index 2f07865ef5..6eeb2785b3 100644 --- a/web/src/test/java/org/apache/shiro/web/mgt/CookieRememberMeManagerTest.java +++ b/web/src/test/java/org/apache/shiro/web/mgt/CookieRememberMeManagerTest.java @@ -125,13 +125,13 @@ public void getRememberedPrincipals() { //The following base64 string was determined from the log output of the above 'onSuccessfulLogin' test. //This will have to change any time the PrincipalCollection implementation changes: - final String userPCAesBase64 = "WlD5MLzzZznN3dQ1lPJO/eScSuY245k29aECNmjUs31o7Yu478hWhaM5Sj" + - "jmoe900/72JNu3hcJaPG6Q17Vuz4F8x0kBjbFnPVx4PqzsZYT6yreeS2jwO6OwfI+efqXOKyB2a5KPtnr" + - "7jt5kZsyH38XJISb81cf6xqTGUru8zC+kNqJFz7E5RpO0kraBofS5jhMm45gDVjDRkjgPJAzocVWMtrza" + - "zy67P8eb+kMSBCqGI251JTNAGboVgQ28KjfaAJ/6LXRJUj7kB7CGia7mgRk+hxzEJGDs81at5VOPqODJr" + - "xb8tcIdemFUFIkiYVP9bGs4dP3ECtmw7aNrCzv+84sx3vRFUrd5DbDYpEuE12hF2Y9owDK9sxStbXoF0y" + - "A32dhfGDIqS+agsass0sWn8WX2TM9i8SxrUjiFbxqyIG49HbqGrZp5QLM9IuIwO+TzGfF1FzumQGdwmWT" + - "xkVapw5UESl34YvA615cb+82ue1I="; + final String userPCAesBase64 = "0o6DCfePYTjK4q579qzUFEfkeGRvbBOdKHp2y8/nGAltt1Vz8uW0Z8igeO" + + "Tq/yBmcw25f3Q0ui/Leg3x0iQZWhw9Bbu0mFHmHsGxEd6mPwtUpSegIjyX5c/kZpqnb7QLdajPWiczX8P" + + "Oc2Eku5+8ye1u38Y8uKlklHxcYCPh0pRiDSBxfjPsLaDfOpGbmPjZd4SVg68i/++TvUjqBNJyb+pDix3f" + + "PeuPvReWGcE50iovezVZrEfDOAQ0cZYW35ShypMWOmE9yZnb+p8++StDyAUegryyuIa4pjuRzfMh9D+sN" + + "F9tm/EnDC1VCer2S/a0AGlWAQiM7jrWt1sNinZcKIrvShaWI21tONJt8WhozNS2H72lk4p92rfLNHeglT" + + "xObxIYxLfTI9KiToSe1nYmpQmbBO8x1wWDkWBG//EqRvhgbIfQVqJp12T0fJC1nFuZuVhw/ZanaAZGDk8" + + "7aLMiw3T6FBZtWaspgvfH+0TJrTD8Ra386ekNXNN8JW8="; Cookie[] cookies = new Cookie[]{ new Cookie(CookieRememberMeManager.DEFAULT_REMEMBER_ME_COOKIE_NAME, userPCAesBase64) @@ -165,13 +165,13 @@ public void getRememberedPrincipalsNoMoreDefaultCipher() { //The following base64 string was determined from the log output of the above 'onSuccessfulLogin' test. //This will have to change any time the PrincipalCollection implementation changes: - final String userPCAesBase64 = "WlD5MLzzZznN3dQ1lPJO/eScSuY245k29aECNmjUs31o7Yu478hWhaM5Sj" + - "jmoe900/72JNu3hcJaPG6Q17Vuz4F8x0kBjbFnPVx4PqzsZYT6yreeS2jwO6OwfI+efqXOKyB2a5KPtnr" + - "7jt5kZsyH38XJISb81cf6xqTGUru8zC+kNqJFz7E5RpO0kraBofS5jhMm45gDVjDRkjgPJAzocVWMtrza" + - "zy67P8eb+kMSBCqGI251JTNAGboVgQ28KjfaAJ/6LXRJUj7kB7CGia7mgRk+hxzEJGDs81at5VOPqODJr" + - "xb8tcIdemFUFIkiYVP9bGs4dP3ECtmw7aNrCzv+84sx3vRFUrd5DbDYpEuE12hF2Y9owDK9sxStbXoF0y" + - "A32dhfGDIqS+agsass0sWn8WX2TM9i8SxrUjiFbxqyIG49HbqGrZp5QLM9IuIwO+TzGfF1FzumQGdwmWT" + - "xkVapw5UESl34YvA615cb+82ue1I="; + final String userPCAesBase64 = "0o6DCfePYTjK4q579qzUFEfkeGRvbBOdKHp2y8/nGAltt1Vz8uW0Z8igeO" + + "Tq/yBmcw25f3Q0ui/Leg3x0iQZWhw9Bbu0mFHmHsGxEd6mPwtUpSegIjyX5c/kZpqnb7QLdajPWiczX8P" + + "Oc2Eku5+8ye1u38Y8uKlklHxcYCPh0pRiDSBxfjPsLaDfOpGbmPjZd4SVg68i/++TvUjqBNJyb+pDix3f" + + "PeuPvReWGcE50iovezVZrEfDOAQ0cZYW35ShypMWOmE9yZnb+p8++StDyAUegryyuIa4pjuRzfMh9D+sN" + + "F9tm/EnDC1VCer2S/a0AGlWAQiM7jrWt1sNinZcKIrvShaWI21tONJt8WhozNS2H72lk4p92rfLNHeglT" + + "xObxIYxLfTI9KiToSe1nYmpQmbBO8x1wWDkWBG//EqRvhgbIfQVqJp12T0fJC1nFuZuVhw/ZanaAZGDk8" + + "7aLMiw3T6FBZtWaspgvfH+0TJrTD8Ra386ekNXNN8JW8="; Cookie[] cookies = new Cookie[]{ new Cookie(CookieRememberMeManager.DEFAULT_REMEMBER_ME_COOKIE_NAME, userPCAesBase64)