terl / lazysodium-android

An Android implementation of the Libsodium cryptography library. For the lazy dev.
https://github.com/terl/lazysodium-java/wiki
Mozilla Public License 2.0
106 stars 24 forks source link

LazySodiumAndroid.cryptoKdfDeriveFromKey() throws an exception if key length exceeds 32 bytes #57

Open 0x4f53 opened 1 year ago

0x4f53 commented 1 year ago

We are trying to use the cryptoKdfDeriveFromKey(subkey: ByteArray?, subkeyLen: Int, subkeyId: Long, context: ByteArray?, masterKey: ByteArray?): Int function on Android to generate keys. For the masterKey parameter, we supply a 64B ByteArray seed output from a bip39 library we use, but there seems to be a 32B check of some kind. We were thrown the following exception:

 E  FATAL EXCEPTION: main
                                                                                                    Process: cloud.keyspace.android, PID: 6710
                                                                                                    java.lang.IllegalArgumentException: Master key length is wrong: 64
                                                                                                        at com.goterl.lazysodium.LazySodium.cryptoKdfDeriveFromKey(LazySodium.java:266)
                                                                                                        at cloud.keyspace.android.DeveloperOptions.onCreate$lambda-3(DeveloperOptions.kt:138)
                                                                                                        at cloud.keyspace.android.DeveloperOptions.$r8$lambda$0j6JCNDxJawYhzlPWAHhcVQ--qI(Unknown Source:0)
                                                                                                        at cloud.keyspace.android.DeveloperOptions$$ExternalSyntheticLambda2.onClick(Unknown Source:11)
                                                                                                        at android.view.View.performClick(View.java:7506)
                                                                                                        at com.google.android.material.button.MaterialButton.performClick(MaterialButton.java:1219)
                                                                                                        at android.view.View.performClickInternal(View.java:7483)
                                                                                                        at android.view.View.-$$Nest$mperformClickInternal(Unknown Source:0)
                                                                                                        at android.view.View$PerformClick.run(View.java:29335)
                                                                                                        at android.os.Handler.handleCallback(Handler.java:942)
                                                                                                        at android.os.Handler.dispatchMessage(Handler.java:99)
                                                                                                        at android.os.Looper.loopOnce(Looper.java:201)
                                                                                                        at android.os.Looper.loop(Looper.java:288)
                                                                                                        at android.app.ActivityThread.main(ActivityThread.java:7898)
                                                                                                        at java.lang.reflect.Method.invoke(Native Method)
                                                                                                        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
                                                                                                        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)

Upon further investigation, the root cause of the issue seems to be at LazySodium.java#258, where the function appears to have a length check for the masterKey ByteArray:

@Override
    public int cryptoKdfDeriveFromKey(byte[] subKey, int subKeyLen, long subKeyId, byte[] context, byte[] masterKey) {
        if (subKeyLen < KeyDerivation.BYTES_MIN || KeyDerivation.BYTES_MAX < subKeyLen) {
            throw new IllegalArgumentException("Sub Key Length is out of bounds: " + subKeyLen);
        }
        if (subKey.length < subKeyLen) {
            throw new IllegalArgumentException("Sub Key array is less than specified size");
        }
        if (masterKey.length != KeyDerivation.MASTER_KEY_BYTES) {
            throw new IllegalArgumentException("Master key length is wrong: " + masterKey.length);
        }
        if (context.length != KeyDerivation.CONTEXT_BYTES) {
            throw new IllegalArgumentException("Context length is wrong: " + context.length);
        }
        return getSodium().crypto_kdf_derive_from_key(subKey, subKeyLen, subKeyId, context, masterKey);
    }

The JavaScript implementation we use (called libsodium.js), doesn't seem to have it either.

To solve our issue for now, we decided to override the function and remove the line ourselves, as such:

class CustomSodium(sodium: SodiumAndroid, charset: Charset) : LazySodiumAndroid (sodium, charset) {
                override fun cryptoKdfDeriveFromKey(subkey: ByteArray?, subkeyLen: Int, subkeyId: Long, context: ByteArray?, masterKey: ByteArray?): Int {
                    require(!(subkeyLen < KeyDerivation.BYTES_MIN || KeyDerivation.BYTES_MAX < subkeyLen)) { "Sub Key Length is out of bounds: $subkeyLen" }
                    require(subkey!!.size >= subkeyLen) { "Sub Key array is less than specified size" }
                    require(context!!.size == KeyDerivation.CONTEXT_BYTES) { "Context length is wrong: " + context.size }
                    return sodium.crypto_kdf_derive_from_key(subkey, subkeyLen, subkeyId, context, masterKey)
                }
            }

            val sodium = CustomSodium(SodiumAndroid(), StandardCharsets.UTF_8)

            val masterKey = bip39.seed!!
            val hardcodedContext = "KEYSPACE".toByteArray(StandardCharsets.UTF_8)
            val outputKeySize = KeyDerivation.MASTER_KEY_BYTES
            val outputKey = ByteArray(outputKeySize)

            val kdfStatusCode = sodium.cryptoKdfDeriveFromKey(outputKey, outputKeySize, 1, hardcodedContext, masterKey)
           ...

This has us asking the following questions:

  1. Why is there a check in the first place?
  2. What are the consequences of removing the check via an override (exceptions? performance?).
gurpreet- commented 1 year ago
  1. The check was added so that library consumers could use this library with ease and because lazysodium must have enforced a check at some point, but I can't see it anymore.
  2. I have removed the check because there is no equivalent check in lazysodium. You can test this out in 5.1.5-SNAPSHOT of lazysodium-java and 5.1.1-SNAPSHOT release in lazysodium-android. The removed check will be available in the next release of those libs.