Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
0x4f53 opened this issue Nov 8, 2022 · 1 comment
Milestone

Comments

@0x4f53
Copy link

0x4f53 commented Nov 8, 2022

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-
Copy link
Contributor

gurpreet- commented Nov 18, 2022

  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.

@gurpreet- gurpreet- added this to the 5.1.1 milestone Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants