From a546246fc11e560fb0cce648e07919bc3b858e79 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 29 Nov 2016 20:11:03 +0100 Subject: [PATCH 1/4] fix wrong key usage during encoding in DefaultCipherHandler, see issue #887 --- .../scm/security/DefaultCipherHandler.java | 2 +- .../security/DefaultCipherHandlerTest.java | 63 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 scm-core/src/test/java/sonia/scm/security/DefaultCipherHandlerTest.java diff --git a/scm-core/src/main/java/sonia/scm/security/DefaultCipherHandler.java b/scm-core/src/main/java/sonia/scm/security/DefaultCipherHandler.java index 4b82563687..0edc77009c 100644 --- a/scm-core/src/main/java/sonia/scm/security/DefaultCipherHandler.java +++ b/scm-core/src/main/java/sonia/scm/security/DefaultCipherHandler.java @@ -251,7 +251,7 @@ public class DefaultCipherHandler implements CipherHandler random.nextBytes(salt); IvParameterSpec iv = new IvParameterSpec(salt); - SecretKey secretKey = buildSecretKey(key); + SecretKey secretKey = buildSecretKey(plainKey); javax.crypto.Cipher cipher = javax.crypto.Cipher.getInstance(CIPHER_TYPE); cipher.init(javax.crypto.Cipher.ENCRYPT_MODE, secretKey, iv); diff --git a/scm-core/src/test/java/sonia/scm/security/DefaultCipherHandlerTest.java b/scm-core/src/test/java/sonia/scm/security/DefaultCipherHandlerTest.java new file mode 100644 index 0000000000..95414d5f71 --- /dev/null +++ b/scm-core/src/test/java/sonia/scm/security/DefaultCipherHandlerTest.java @@ -0,0 +1,63 @@ +/** + * Copyright (c) 2014, Sebastian Sdorra + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * 3. Neither the name of SCM-Manager; nor the names of its + * contributors may be used to endorse or promote products derived from this + * software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * http://bitbucket.org/sdorra/scm-manager + * + */ + +package sonia.scm.security; + +import org.junit.Test; +import static org.junit.Assert.*; + +/** + * Unit tests for {@link DefaultCipherHandler}. + * + * @author Sebastian Sdorra + */ +public class DefaultCipherHandlerTest { + + /** + * Test encode and decode method with a separate key. + */ + @Test + public void testEncodeDecodeWithSeparateKey(){ + char[] key = "testkey".toCharArray(); + DefaultCipherHandler cipher = new DefaultCipherHandler("somekey"); + assertEquals("hallo123", cipher.decode(key, cipher.encode(key, "hallo123"))); + } + + /** + * Test encode and decode method with the default key. + */ + @Test + public void testEncodeDecodeWithDefaultKey() { + DefaultCipherHandler cipher = new DefaultCipherHandler("testkey"); + assertEquals("hallo123", cipher.decode(cipher.encode("hallo123"))); + } + +} \ No newline at end of file From 634061a91db5376aeb290dd83bc52340af3fb383 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 29 Nov 2016 20:36:11 +0100 Subject: [PATCH 2/4] refactoring DefaultCipherHandler and added javadoc to CipherHandler --- .../sonia/scm/security/CipherHandler.java | 17 +- .../scm/security/DefaultCipherHandler.java | 212 +++++------------- 2 files changed, 64 insertions(+), 165 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/security/CipherHandler.java b/scm-core/src/main/java/sonia/scm/security/CipherHandler.java index 8463f4a4e3..a27c4b4e28 100644 --- a/scm-core/src/main/java/sonia/scm/security/CipherHandler.java +++ b/scm-core/src/main/java/sonia/scm/security/CipherHandler.java @@ -35,7 +35,8 @@ package sonia.scm.security; /** - * + * Encrypts and decrypts string values. + * * @author Sebastian Sdorra * @since 1.7 */ @@ -43,22 +44,20 @@ public interface CipherHandler { /** - * Method description + * Decrypts the given value. * + * @param value encrypted value * - * @param value - * - * @return + * @return decrypted value */ public String decode(String value); /** - * Method description + * Encrypts the given value. * + * @param value plain text value to encrypt. * - * @param value - * - * @return + * @return encrypted value */ public String encode(String value); } diff --git a/scm-core/src/main/java/sonia/scm/security/DefaultCipherHandler.java b/scm-core/src/main/java/sonia/scm/security/DefaultCipherHandler.java index 0edc77009c..ddb8a699a7 100644 --- a/scm-core/src/main/java/sonia/scm/security/DefaultCipherHandler.java +++ b/scm-core/src/main/java/sonia/scm/security/DefaultCipherHandler.java @@ -53,7 +53,7 @@ import java.io.FileNotFoundException; import java.io.FileReader; import java.io.IOException; import java.io.PrintWriter; -import java.io.UnsupportedEncodingException; +import java.security.GeneralSecurityException; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; @@ -66,32 +66,32 @@ import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; /** - * + * Default implementation of the {@link CipherHandler}, which uses AES for + * encryption and decryption. + * * @author Sebastian Sdorra * @since 1.7 */ -public class DefaultCipherHandler implements CipherHandler -{ +public class DefaultCipherHandler implements CipherHandler { - /** Field description */ + /** used cipher type */ public static final String CIPHER_TYPE = "AES/CTR/PKCS5PADDING"; - /** Field description */ + /** digest type for key generation */ public static final String DIGEST_TYPE = "SHA-512"; - /** Field description */ + /** string encoding */ public static final String ENCODING = "UTF-8"; - /** Field description */ + /** default key length */ public static final int KEY_LENGTH = 16; - /** Field description */ + /** default salt length */ public static final int SALT_LENGTH = 16; - /** Field description */ - private static final String CIPHERKEY_FILENAME = ".cipherkey"; + @VisibleForTesting + static final String CIPHERKEY_FILENAME = ".cipherkey"; - /** Field description */ private static final char[] KEY_BASE = new char[] { '1', '4', '7', '3', 'F', '2', '1', 'E', '-', 'C', '4', 'C', '4', '-', '4', @@ -99,96 +99,72 @@ public class DefaultCipherHandler implements CipherHandler 'E', 'C', '7', '7', '2', 'E' }; - /** Field description */ private static final String KEY_TYPE = "AES"; /** the logger for DefaultCipherHandler */ - private static final Logger logger = - LoggerFactory.getLogger(DefaultCipherHandler.class); - - //~--- constructors --------------------------------------------------------- - + private static final Logger logger = LoggerFactory.getLogger(DefaultCipherHandler.class); + + private final SecureRandom random = new SecureRandom(); + + private final char[] key; + /** * Constructs a new DefaultCipherHandler. Note this constructor is only for * unit tests. * - * - * @param key + * @param key default encryption key * * @since 1.38 */ @VisibleForTesting - protected DefaultCipherHandler(String key) - { + protected DefaultCipherHandler(String key) { this.key = key.toCharArray(); } /** - * Constructs ... - * - * - * @param context - * @param keyGenerator - * + * Constructs a new instance and reads the default key from the scm home directory, + * if the key file does not exists it will be generated with the {@link KeyGenerator}. * + * @param context SCM-Manager context provider + * @param keyGenerator key generator for default key generation */ - public DefaultCipherHandler(SCMContextProvider context, - KeyGenerator keyGenerator) - { + public DefaultCipherHandler(SCMContextProvider context, KeyGenerator keyGenerator) { File configDirectory = new File(context.getBaseDirectory(), "config"); IOUtil.mkdirs(configDirectory); - cipherKeyFile = new File(configDirectory, CIPHERKEY_FILENAME); + File cipherKeyFile = new File(configDirectory, CIPHERKEY_FILENAME); - try - { - if (cipherKeyFile.exists()) - { - loadKey(); - } - else - { + try { + if (cipherKeyFile.exists()) { + key = loadKey(cipherKeyFile); + } else { key = keyGenerator.createKey().toCharArray(); - storeKey(); + storeKey(cipherKeyFile); } - } - catch (IOException ex) - { + } catch (IOException ex) { throw new CipherException("could not create CipherHandler", ex); } } //~--- methods -------------------------------------------------------------- - /** - * Method description - * - * - * @param value - * - * @return - */ @Override - public String decode(String value) - { + public String decode(String value) { return decode(key, value); } /** - * Method description + * Decodes the given value with the provided key. * + * @param plainKey key which is used for decoding + * @param value encrypted value * - * @param plainKey - * @param value - * - * @return + * @return decrypted value */ - public String decode(char[] plainKey, String value) - { + public String decode(char[] plainKey, String value) { String result = null; - try - { + try { byte[] encodedInput = Base64.decode(value); byte[] salt = new byte[SALT_LENGTH]; byte[] encoded = new byte[encodedInput.length - SALT_LENGTH]; @@ -206,46 +182,29 @@ public class DefaultCipherHandler implements CipherHandler byte[] decoded = cipher.doFinal(encoded); result = new String(decoded, ENCODING); - } - catch (Exception ex) - { - logger.error("could not decode string", ex); - - throw new CipherException(ex); + } catch (IOException | GeneralSecurityException ex) { + throw new CipherException("could not decode string", ex); } return result; } - /** - * Method description - * - * - * @param value - * - * @return - */ @Override - public String encode(String value) - { + public String encode(String value) { return encode(key, value); } /** - * Method description + * Encrypts the given value with the provided key. * + * @param plainKey key which is used for encoding + * @param value plain text value to encrypt * - * @param plainKey - * @param value - * - * @return + * @return encrypted value */ - public String encode(char[] plainKey, String value) - { + public String encode(char[] plainKey, String value) { String res = null; - - try - { + try { byte[] salt = new byte[SALT_LENGTH]; random.nextBytes(salt); @@ -264,31 +223,14 @@ public class DefaultCipherHandler implements CipherHandler System.arraycopy(encodedInput, 0, result, SALT_LENGTH, result.length - SALT_LENGTH); res = new String(Base64.encode(result), ENCODING); - } - catch (Exception ex) - { - logger.error("could not encode string", ex); - - throw new CipherException(ex); + } catch (IOException | GeneralSecurityException ex) { + throw new CipherException("could not encode string", ex); } return res; } - /** - * Method description - * - * - * @param plainKey - * - * @return - * - * @throws NoSuchAlgorithmException - * @throws UnsupportedEncodingException - */ - private SecretKey buildSecretKey(char[] plainKey) - throws UnsupportedEncodingException, NoSuchAlgorithmException - { + private SecretKey buildSecretKey(char[] plainKey) throws IOException, NoSuchAlgorithmException { byte[] raw = new String(plainKey).getBytes(ENCODING); MessageDigest digest = MessageDigest.getInstance(DIGEST_TYPE); @@ -298,60 +240,18 @@ public class DefaultCipherHandler implements CipherHandler return new SecretKeySpec(raw, KEY_TYPE); } - /** - * Method description - * - * - * @throws IOException - */ - private void loadKey() throws IOException - { - BufferedReader reader = null; - - try - { - reader = new BufferedReader(new FileReader(cipherKeyFile)); - + private char[] loadKey(File cipherKeyFile) throws IOException { + try (BufferedReader reader = new BufferedReader(new FileReader(cipherKeyFile))) { String line = reader.readLine(); - key = decode(KEY_BASE, line).toCharArray(); - } - finally - { - IOUtil.close(reader); + return decode(KEY_BASE, line).toCharArray(); } } - /** - * Method description - * - * - * @throws FileNotFoundException - */ - private void storeKey() throws FileNotFoundException - { + private void storeKey(File cipherKeyFile) throws FileNotFoundException { String storeKey = encode(KEY_BASE, new String(key)); - PrintWriter output = null; - - try - { - output = new PrintWriter(cipherKeyFile); + try (PrintWriter output = new PrintWriter(cipherKeyFile)) { output.write(storeKey); } - finally - { - IOUtil.close(output); - } } - - //~--- fields --------------------------------------------------------------- - - /** Field description */ - private File cipherKeyFile; - - /** Field description */ - private char[] key = null; - - /** Field description */ - private SecureRandom random = new SecureRandom(); } From 0a47d5946a4bc72b6536d56f062ccf580e1b39a6 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 29 Nov 2016 20:46:30 +0100 Subject: [PATCH 3/4] added unit test for storing and loading of default cipher key --- .../security/DefaultCipherHandlerTest.java | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/scm-core/src/test/java/sonia/scm/security/DefaultCipherHandlerTest.java b/scm-core/src/test/java/sonia/scm/security/DefaultCipherHandlerTest.java index 95414d5f71..1fe78191de 100644 --- a/scm-core/src/test/java/sonia/scm/security/DefaultCipherHandlerTest.java +++ b/scm-core/src/test/java/sonia/scm/security/DefaultCipherHandlerTest.java @@ -31,15 +31,62 @@ package sonia.scm.security; +import java.io.File; +import java.io.IOException; import org.junit.Test; import static org.junit.Assert.*; +import org.junit.Rule; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import static org.mockito.Mockito.*; +import org.mockito.runners.MockitoJUnitRunner; +import sonia.scm.SCMContextProvider; /** * Unit tests for {@link DefaultCipherHandler}. * * @author Sebastian Sdorra */ +@RunWith(MockitoJUnitRunner.class) public class DefaultCipherHandlerTest { + + @Mock + private SCMContextProvider context; + + @Mock + private KeyGenerator keyGenerator; + + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + + /** + * Tests loading and storing default key. + * + * @throws IOException + */ + @Test + public void testLoadingAndStoringDefaultKey() throws IOException { + File baseDirectory = tempFolder.newFolder(); + when(context.getBaseDirectory()).thenReturn(baseDirectory); + when(keyGenerator.createKey()).thenReturn("secret"); + + DefaultCipherHandler cipher = new DefaultCipherHandler(context, keyGenerator); + File configDirectory = new File(baseDirectory, "config"); + assertTrue(new File(configDirectory, DefaultCipherHandler.CIPHERKEY_FILENAME).exists()); + + // plain text for assertion + String plain = "hallo123"; + + // encrypt value with new generated key + String encrypted = cipher.encode(plain); + + // load key from disk + cipher = new DefaultCipherHandler(context, keyGenerator); + + // decrypt with loaded key + assertEquals(plain, cipher.decode(encrypted)); + } /** * Test encode and decode method with a separate key. From 70bd1ea2272bd1f43117e05cf87135fe299e9e69 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 6 Dec 2016 19:24:47 +0100 Subject: [PATCH 4/4] close branch issue-887