From 7456e2c8f565272c9f4c793606bd20a2af3171f8 Mon Sep 17 00:00:00 2001 From: J-Jamet Date: Wed, 22 Apr 2020 20:23:23 +0200 Subject: [PATCH] Refactor key encryption methods and add unit tests --- .../crypto/{FinalKeyTest.kt => AESKeyTest.kt} | 29 +++++++++---------- .../kunzisoft/keepass/tests/crypto/AESTest.kt | 2 -- .../{HashedBlock.kt => HashedBlockTest.kt} | 2 +- .../keepass/tests/utils/UnsignedIntTest.kt | 28 ++++++++++++++++++ .../ValuesTest.kt} | 7 ++--- .../com/kunzisoft/keepass/crypto/NativeLib.kt | 2 +- ...ctory.java => AESKeyTransformerFactory.kt} | 21 +++++++------- ...{KeyTransformer.java => KeyTransformer.kt} | 13 +++++---- .../keepass/crypto/keyDerivation/AesKdf.kt | 4 +-- .../database/element/database/DatabaseKDB.kt | 4 +-- .../database/file/DatabaseHeaderKDBX.kt | 2 +- .../file/output/DatabaseHeaderOutputKDBX.kt | 2 +- .../kunzisoft/keepass/utils/UnsignedInt.kt | 2 +- .../kunzisoft/keepass/utils/UnsignedLong.kt | 2 +- 14 files changed, 72 insertions(+), 48 deletions(-) rename app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/{FinalKeyTest.kt => AESKeyTest.kt} (71%) rename app/src/androidTest/java/com/kunzisoft/keepass/tests/stream/{HashedBlock.kt => HashedBlockTest.kt} (98%) create mode 100644 app/src/androidTest/java/com/kunzisoft/keepass/tests/utils/UnsignedIntTest.kt rename app/src/androidTest/java/com/kunzisoft/keepass/tests/{StringDatabaseKDBUtilsTest.kt => utils/ValuesTest.kt} (97%) rename app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/{AESFactory.java => AESKeyTransformerFactory.kt} (63%) rename app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/{KeyTransformer.java => KeyTransformer.kt} (71%) diff --git a/app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/FinalKeyTest.kt b/app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/AESKeyTest.kt similarity index 71% rename from app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/FinalKeyTest.kt rename to app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/AESKeyTest.kt index 038790b54..4dfb4b08a 100644 --- a/app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/FinalKeyTest.kt +++ b/app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/AESKeyTest.kt @@ -29,8 +29,8 @@ import junit.framework.TestCase import com.kunzisoft.keepass.crypto.finalkey.AndroidAESKeyTransformer import com.kunzisoft.keepass.crypto.finalkey.NativeAESKeyTransformer -class FinalKeyTest : TestCase() { - private var mRand: Random? = null +class AESKeyTest : TestCase() { + private lateinit var mRand: Random @Throws(Exception::class) override fun setUp() { @@ -40,29 +40,28 @@ class FinalKeyTest : TestCase() { } @Throws(IOException::class) - fun testNativeAndroid() { + fun testAES() { // Test both an old and an even number to test my flip variable - testNativeFinalKey(5) - testNativeFinalKey(6) + testAESFinalKey(5) + testAESFinalKey(6) } @Throws(IOException::class) - private fun testNativeFinalKey(rounds: Int) { + private fun testAESFinalKey(rounds: Long) { val seed = ByteArray(32) val key = ByteArray(32) - val nativeKey: ByteArray - val androidKey: ByteArray + val nativeKey: ByteArray? + val androidKey: ByteArray? - mRand!!.nextBytes(seed) - mRand!!.nextBytes(key) + mRand.nextBytes(seed) + mRand.nextBytes(key) - val aKey = AndroidAESKeyTransformer() - androidKey = aKey.transformMasterKey(seed, key, rounds.toLong()) + val androidAESKey = AndroidAESKeyTransformer() + androidKey = androidAESKey.transformMasterKey(seed, key, rounds) - val nKey = NativeAESKeyTransformer() - nativeKey = nKey.transformMasterKey(seed, key, rounds.toLong()) + val nativeAESKey = NativeAESKeyTransformer() + nativeKey = nativeAESKey.transformMasterKey(seed, key, rounds) assertArrayEquals("Does not match", androidKey, nativeKey) - } } diff --git a/app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/AESTest.kt b/app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/AESTest.kt index 69fdb222c..7e2e17229 100644 --- a/app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/AESTest.kt +++ b/app/src/androidTest/java/com/kunzisoft/keepass/tests/crypto/AESTest.kt @@ -80,6 +80,4 @@ class AESTest : TestCase() { assertArrayEquals("Arrays differ on size: $dataSize", outAndroid, outNative) } - - } diff --git a/app/src/androidTest/java/com/kunzisoft/keepass/tests/stream/HashedBlock.kt b/app/src/androidTest/java/com/kunzisoft/keepass/tests/stream/HashedBlockTest.kt similarity index 98% rename from app/src/androidTest/java/com/kunzisoft/keepass/tests/stream/HashedBlock.kt rename to app/src/androidTest/java/com/kunzisoft/keepass/tests/stream/HashedBlockTest.kt index 62cbe52e9..e66a468cd 100644 --- a/app/src/androidTest/java/com/kunzisoft/keepass/tests/stream/HashedBlock.kt +++ b/app/src/androidTest/java/com/kunzisoft/keepass/tests/stream/HashedBlockTest.kt @@ -33,7 +33,7 @@ import junit.framework.TestCase import com.kunzisoft.keepass.stream.HashedBlockInputStream import com.kunzisoft.keepass.stream.HashedBlockOutputStream -class HashedBlock : TestCase() { +class HashedBlockTest : TestCase() { @Throws(IOException::class) fun testBlockAligned() { diff --git a/app/src/androidTest/java/com/kunzisoft/keepass/tests/utils/UnsignedIntTest.kt b/app/src/androidTest/java/com/kunzisoft/keepass/tests/utils/UnsignedIntTest.kt new file mode 100644 index 000000000..6b8e32560 --- /dev/null +++ b/app/src/androidTest/java/com/kunzisoft/keepass/tests/utils/UnsignedIntTest.kt @@ -0,0 +1,28 @@ +package com.kunzisoft.keepass.tests.utils + +import com.kunzisoft.keepass.utils.UnsignedInt +import junit.framework.TestCase + +class UnsignedIntTest: TestCase() { + + fun testUInt() { + val standardInt = UnsignedInt(15).toInt() + assertEquals(15, standardInt) + val unsignedInt = UnsignedInt(-1).toLong() + assertEquals(4294967295L, unsignedInt) + } + + fun testMaxValue() { + val maxValue = UnsignedInt.MAX_VALUE.toLong() + assertEquals(4294967295L, maxValue) + val longValue = UnsignedInt.fromLong(4294967295L).toLong() + assertEquals(longValue, maxValue) + } + + fun testLong() { + val longValue = UnsignedInt.fromLong(50L).toInt() + assertEquals(50, longValue) + val uIntLongValue = UnsignedInt.fromLong(4294967290).toLong() + assertEquals(4294967290, uIntLongValue) + } +} \ No newline at end of file diff --git a/app/src/androidTest/java/com/kunzisoft/keepass/tests/StringDatabaseKDBUtilsTest.kt b/app/src/androidTest/java/com/kunzisoft/keepass/tests/utils/ValuesTest.kt similarity index 97% rename from app/src/androidTest/java/com/kunzisoft/keepass/tests/StringDatabaseKDBUtilsTest.kt rename to app/src/androidTest/java/com/kunzisoft/keepass/tests/utils/ValuesTest.kt index dab86f23c..fcab0b75d 100644 --- a/app/src/androidTest/java/com/kunzisoft/keepass/tests/StringDatabaseKDBUtilsTest.kt +++ b/app/src/androidTest/java/com/kunzisoft/keepass/tests/utils/ValuesTest.kt @@ -17,7 +17,7 @@ * along with KeePassDX. If not, see . * */ -package com.kunzisoft.keepass.tests +package com.kunzisoft.keepass.tests.utils import com.kunzisoft.keepass.database.element.DateInstant import com.kunzisoft.keepass.stream.* @@ -28,7 +28,7 @@ import org.junit.Assert.assertArrayEquals import java.io.ByteArrayOutputStream import java.util.* -class StringDatabaseKDBUtilsTest : TestCase() { +class ValuesTest : TestCase() { fun testReadWriteLongZero() { testReadWriteLong(0.toByte()) @@ -82,7 +82,6 @@ class StringDatabaseKDBUtilsTest : TestCase() { val dest = uIntTo4Bytes(one) assertArrayEquals(orig, dest) - } private fun setArray(buf: ByteArray, value: Byte, size: Int) { @@ -186,7 +185,7 @@ class StringDatabaseKDBUtilsTest : TestCase() { val bos = ByteArrayOutputStream() val leos = LittleEndianDataOutputStream(bos) - leos.writeLong(UnsignedLong.ULONG_MAX_VALUE) + leos.writeLong(UnsignedLong.MAX_VALUE) leos.close() val uLongMax = bos.toByteArray() diff --git a/app/src/main/java/com/kunzisoft/keepass/crypto/NativeLib.kt b/app/src/main/java/com/kunzisoft/keepass/crypto/NativeLib.kt index 4682692d6..d43cf9152 100644 --- a/app/src/main/java/com/kunzisoft/keepass/crypto/NativeLib.kt +++ b/app/src/main/java/com/kunzisoft/keepass/crypto/NativeLib.kt @@ -30,7 +30,7 @@ object NativeLib { fun init(): Boolean { if (!isLoaded) { try { - System.loadLibrary("final-key") // TODO Rename + System.loadLibrary("final-key") System.loadLibrary("argon2") } catch (e: UnsatisfiedLinkError) { return false diff --git a/app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/AESFactory.java b/app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/AESKeyTransformerFactory.kt similarity index 63% rename from app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/AESFactory.java rename to app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/AESKeyTransformerFactory.kt index 8809a606c..c58364b99 100644 --- a/app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/AESFactory.java +++ b/app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/AESKeyTransformerFactory.kt @@ -17,21 +17,20 @@ * along with KeePassDX. If not, see . * */ -package com.kunzisoft.keepass.crypto.finalkey; +package com.kunzisoft.keepass.crypto.finalkey -import com.kunzisoft.keepass.crypto.CipherFactory; +import com.kunzisoft.keepass.crypto.CipherFactory.deviceBlacklisted -public class AESFactory { - - // TODO Encaspulate - public static KeyTransformer createFinalKey() { +object AESKeyTransformerFactory : KeyTransformer() { + override fun transformMasterKey(seed: ByteArray?, key: ByteArray?, rounds: Long): ByteArray? { // Prefer the native final key implementation - if ( !CipherFactory.INSTANCE.deviceBlacklisted() - && NativeAESKeyTransformer.available() ) { - return new NativeAESKeyTransformer(); + val keyTransformer = if (!deviceBlacklisted() + && NativeAESKeyTransformer.available()) { + NativeAESKeyTransformer() } else { // Fall back on the android crypto implementation - return new AndroidAESKeyTransformer(); + AndroidAESKeyTransformer() } + return keyTransformer.transformMasterKey(seed, key, rounds) } -} +} \ No newline at end of file diff --git a/app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/KeyTransformer.java b/app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/KeyTransformer.kt similarity index 71% rename from app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/KeyTransformer.java rename to app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/KeyTransformer.kt index f3234d44a..419c760f7 100644 --- a/app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/KeyTransformer.java +++ b/app/src/main/java/com/kunzisoft/keepass/crypto/finalkey/KeyTransformer.kt @@ -1,5 +1,5 @@ /* - * Copyright 2017 Brian Pellin, Jeremy Jamet / Kunzisoft. + * Copyright 2020 Jeremy Jamet / Kunzisoft. * * This file is part of KeePassDX. * @@ -17,10 +17,11 @@ * along with KeePassDX. If not, see . * */ -package com.kunzisoft.keepass.crypto.finalkey; +package com.kunzisoft.keepass.crypto.finalkey -import java.io.IOException; +import java.io.IOException -public abstract class KeyTransformer { - public abstract byte[] transformMasterKey(byte[] seed, byte[] key, long rounds) throws IOException; -} +abstract class KeyTransformer { + @Throws(IOException::class) + abstract fun transformMasterKey(seed: ByteArray?, key: ByteArray?, rounds: Long): ByteArray? +} \ No newline at end of file diff --git a/app/src/main/java/com/kunzisoft/keepass/crypto/keyDerivation/AesKdf.kt b/app/src/main/java/com/kunzisoft/keepass/crypto/keyDerivation/AesKdf.kt index cbf052437..5e5ce903c 100644 --- a/app/src/main/java/com/kunzisoft/keepass/crypto/keyDerivation/AesKdf.kt +++ b/app/src/main/java/com/kunzisoft/keepass/crypto/keyDerivation/AesKdf.kt @@ -22,7 +22,7 @@ package com.kunzisoft.keepass.crypto.keyDerivation import android.content.res.Resources import com.kunzisoft.keepass.R import com.kunzisoft.keepass.crypto.CryptoUtil -import com.kunzisoft.keepass.crypto.finalkey.AESFactory +import com.kunzisoft.keepass.crypto.finalkey.AESKeyTransformerFactory import com.kunzisoft.keepass.stream.bytes16ToUuid import com.kunzisoft.keepass.utils.UnsignedInt import java.io.IOException @@ -63,7 +63,7 @@ class AesKdf internal constructor() : KdfEngine() { seed = CryptoUtil.hashSha256(seed) } - return AESFactory.createFinalKey().transformMasterKey(seed, currentMasterKey, rounds) + return AESKeyTransformerFactory.transformMasterKey(seed, currentMasterKey, rounds) ?: ByteArray(0) } override fun randomize(p: KdfParameters) { diff --git a/app/src/main/java/com/kunzisoft/keepass/database/element/database/DatabaseKDB.kt b/app/src/main/java/com/kunzisoft/keepass/database/element/database/DatabaseKDB.kt index 6f44336a6..56141abfd 100644 --- a/app/src/main/java/com/kunzisoft/keepass/database/element/database/DatabaseKDB.kt +++ b/app/src/main/java/com/kunzisoft/keepass/database/element/database/DatabaseKDB.kt @@ -19,7 +19,7 @@ package com.kunzisoft.keepass.database.element.database -import com.kunzisoft.keepass.crypto.finalkey.AESFactory +import com.kunzisoft.keepass.crypto.finalkey.AESKeyTransformerFactory import com.kunzisoft.keepass.crypto.keyDerivation.KdfEngine import com.kunzisoft.keepass.crypto.keyDerivation.KdfFactory import com.kunzisoft.keepass.database.element.entry.EntryKDB @@ -149,7 +149,7 @@ class DatabaseKDB : DatabaseVersioned() { // Encrypt the master key a few times to make brute-force key-search harder dos.write(masterSeed) - dos.write(AESFactory.createFinalKey().transformMasterKey(masterSeed2, masterKey, numRounds)) + dos.write(AESKeyTransformerFactory.transformMasterKey(masterSeed2, masterKey, numRounds) ?: ByteArray(0)) finalKey = messageDigest.digest() } diff --git a/app/src/main/java/com/kunzisoft/keepass/database/file/DatabaseHeaderKDBX.kt b/app/src/main/java/com/kunzisoft/keepass/database/file/DatabaseHeaderKDBX.kt index bdbacb964..a5a5abf26 100644 --- a/app/src/main/java/com/kunzisoft/keepass/database/file/DatabaseHeaderKDBX.kt +++ b/app/src/main/java/com/kunzisoft/keepass/database/file/DatabaseHeaderKDBX.kt @@ -324,7 +324,7 @@ class DatabaseHeaderKDBX(private val databaseV4: DatabaseKDBX) : DatabaseHeader( @Throws(IOException::class) fun computeHeaderHmac(header: ByteArray, key: ByteArray): ByteArray { - val blockKey = HmacBlockStream.getHmacKey64(key, UnsignedLong.ULONG_MAX_VALUE) + val blockKey = HmacBlockStream.getHmacKey64(key, UnsignedLong.MAX_VALUE) val hmac: Mac try { diff --git a/app/src/main/java/com/kunzisoft/keepass/database/file/output/DatabaseHeaderOutputKDBX.kt b/app/src/main/java/com/kunzisoft/keepass/database/file/output/DatabaseHeaderOutputKDBX.kt index 571e7c136..7dec88ab7 100644 --- a/app/src/main/java/com/kunzisoft/keepass/database/file/output/DatabaseHeaderOutputKDBX.kt +++ b/app/src/main/java/com/kunzisoft/keepass/database/file/output/DatabaseHeaderOutputKDBX.kt @@ -66,7 +66,7 @@ constructor(private val databaseKDBX: DatabaseKDBX, val hmac: Mac try { hmac = Mac.getInstance("HmacSHA256") - val signingKey = SecretKeySpec(HmacBlockStream.getHmacKey64(hmacKey, UnsignedLong.ULONG_MAX_VALUE), "HmacSHA256") + val signingKey = SecretKeySpec(HmacBlockStream.getHmacKey64(hmacKey, UnsignedLong.MAX_VALUE), "HmacSHA256") hmac.init(signingKey) } catch (e: NoSuchAlgorithmException) { throw DatabaseOutputException(e) diff --git a/app/src/main/java/com/kunzisoft/keepass/utils/UnsignedInt.kt b/app/src/main/java/com/kunzisoft/keepass/utils/UnsignedInt.kt index 2ee23cab1..777cb2616 100644 --- a/app/src/main/java/com/kunzisoft/keepass/utils/UnsignedInt.kt +++ b/app/src/main/java/com/kunzisoft/keepass/utils/UnsignedInt.kt @@ -65,7 +65,7 @@ class UnsignedInt(private var unsignedValue: Int) { companion object { private const val INT_TO_LONG_MASK: Long = 0xffffffffL - private const val UINT_MAX_VALUE: Long = 4294967296L // 2^32 + private const val UINT_MAX_VALUE: Long = 4294967295L // 2^32 val MAX_VALUE = UnsignedInt(UINT_MAX_VALUE.toInt()) diff --git a/app/src/main/java/com/kunzisoft/keepass/utils/UnsignedLong.kt b/app/src/main/java/com/kunzisoft/keepass/utils/UnsignedLong.kt index 804bab699..b051ed1a3 100644 --- a/app/src/main/java/com/kunzisoft/keepass/utils/UnsignedLong.kt +++ b/app/src/main/java/com/kunzisoft/keepass/utils/UnsignedLong.kt @@ -44,6 +44,6 @@ class UnsignedLong(private var unsignedValue: Long) { } companion object { - const val ULONG_MAX_VALUE: Long = -1 + const val MAX_VALUE: Long = -1 } } \ No newline at end of file