From 488a2a9fa58bc9a14cd0791dc0c7c20c81ede31b Mon Sep 17 00:00:00 2001 From: Adam Velebil Date: Tue, 21 Nov 2023 10:33:53 +0100 Subject: [PATCH 1/4] clear sensitive data from memory after use --- .../com/yubico/yubikit/fido/ctap/Hkdf.java | 19 +++++++++---- .../fido/ctap/PinUvAuthProtocolV2.java | 27 ++++++++++++++++--- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/fido/src/main/java/com/yubico/yubikit/fido/ctap/Hkdf.java b/fido/src/main/java/com/yubico/yubikit/fido/ctap/Hkdf.java index 6e33e6b7..b75f2ff7 100644 --- a/fido/src/main/java/com/yubico/yubikit/fido/ctap/Hkdf.java +++ b/fido/src/main/java/com/yubico/yubikit/fido/ctap/Hkdf.java @@ -61,20 +61,29 @@ byte[] expand(byte[] prk, byte[] info, int length) throws InvalidKeyException { .put(info) .put(i) .array(); - t = hmacDigest(prk, data); + Arrays.fill(t, (byte) 0); + byte[] digest = hmacDigest(prk, data); - okm = ByteBuffer.allocate(okm.length + t.length) + byte[] result = ByteBuffer.allocate(okm.length + digest.length) .put(okm) - .put(t) + .put(digest) .array(); + Arrays.fill(okm, (byte) 0); + Arrays.fill(data, (byte) 0); + okm = result; + t = digest; } - return Arrays.copyOf(okm, length); + byte[] result = Arrays.copyOf(okm, length); + Arrays.fill(okm, (byte) 0); + return result; } byte[] digest(byte[] ikm, byte[] salt, byte[] info, int length) throws NoSuchAlgorithmException, InvalidKeyException { byte[] prk = extract(salt, ikm); - return expand(prk, info, length); + byte[] result = expand(prk, info, length); + Arrays.fill(prk, (byte) 0); + return result; } } diff --git a/fido/src/main/java/com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java b/fido/src/main/java/com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java index a6cd0b3a..00e17f4f 100644 --- a/fido/src/main/java/com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java +++ b/fido/src/main/java/com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java @@ -56,14 +56,16 @@ public int getVersion() { @Override public byte[] kdf(byte[] z) { + byte[] hmacKey = null; + byte[] aesKey = null; try { - byte[] hmacKey = new Hkdf(HKDF_ALG).digest( + hmacKey = new Hkdf(HKDF_ALG).digest( z, HKDF_SALT, HKDF_INFO_HMAC, HKDF_LENGTH); - byte[] aesKey = new Hkdf(HKDF_ALG).digest( + aesKey = new Hkdf(HKDF_ALG).digest( z, HKDF_SALT, HKDF_INFO_AES, @@ -75,13 +77,21 @@ public byte[] kdf(byte[] z) { .array(); } catch (NoSuchAlgorithmException | InvalidKeyException e) { throw new IllegalStateException(e); + } finally { + if (hmacKey != null) { + Arrays.fill(hmacKey, (byte) 0); + } + if (aesKey != null) { + Arrays.fill(aesKey, (byte) 0); + } } } @Override public byte[] encrypt(byte[] key, byte[] plaintext) { + byte[] aesKey = null; try { - byte[] aesKey = Arrays.copyOfRange(key, 32, key.length); + aesKey = Arrays.copyOfRange(key, 32, key.length); byte[] iv = RandomUtils.getRandomBytes(16); final byte[] ciphertext = @@ -93,19 +103,28 @@ public byte[] encrypt(byte[] key, byte[] plaintext) { .array(); } catch (IllegalBlockSizeException | BadPaddingException e) { throw new IllegalStateException(e); + } finally { + if (aesKey != null) { + Arrays.fill(aesKey, (byte) 0); + } } } @Override public byte[] decrypt(byte[] key, byte[] ciphertext) { + byte[] aesKey = null; try { - byte[] aesKey = Arrays.copyOfRange(key, 32, key.length); + aesKey = Arrays.copyOfRange(key, 32, key.length); byte[] iv = Arrays.copyOf(ciphertext, 16); byte[] ct = Arrays.copyOfRange(ciphertext, 16, ciphertext.length); byte[] plaintext = getCipher(Cipher.DECRYPT_MODE, aesKey, iv).doFinal(ct); return Arrays.copyOf(plaintext, plaintext.length); } catch (BadPaddingException | IllegalBlockSizeException e) { throw new IllegalStateException(e); + } finally { + if (aesKey != null) { + Arrays.fill(aesKey, (byte) 0); + } } } From 94b28c3d012a8258e7253c6086b186a94d1bafc0 Mon Sep 17 00:00:00 2001 From: Adam Velebil Date: Tue, 21 Nov 2023 10:34:27 +0100 Subject: [PATCH 2/4] warn about incompatible/invalid parameters --- .../fido/client/BasicWebAuthnClient.java | 21 ++++++++++++++----- .../yubico/yubikit/fido/ctap/ClientPin.java | 5 +++++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/fido/src/main/java/com/yubico/yubikit/fido/client/BasicWebAuthnClient.java b/fido/src/main/java/com/yubico/yubikit/fido/client/BasicWebAuthnClient.java index d776a2b7..83a3424b 100644 --- a/fido/src/main/java/com/yubico/yubikit/fido/client/BasicWebAuthnClient.java +++ b/fido/src/main/java/com/yubico/yubikit/fido/client/BasicWebAuthnClient.java @@ -19,6 +19,7 @@ import com.yubico.yubikit.core.application.CommandException; import com.yubico.yubikit.core.application.CommandState; import com.yubico.yubikit.core.fido.CtapException; +import com.yubico.yubikit.core.internal.Logger; import com.yubico.yubikit.fido.ctap.ClientPin; import com.yubico.yubikit.fido.ctap.CredentialManagement; import com.yubico.yubikit.fido.ctap.Ctap2Session; @@ -449,12 +450,22 @@ protected Ctap2Session.CredentialData ctapMakeCredential( } @Nullable Integer validatedEnterpriseAttestation = null; - if (isEnterpriseAttestationSupported() && + boolean enterpriseAttestationSupported = isEnterpriseAttestationSupported(); + + if (!enterpriseAttestationSupported && enterpriseAttestation != null) { + Logger.warn(logger, "Ignoring provided enterpriseAttestation parameter because" + + " the authenticator does not support enterprise attestation."); + } + + if (enterpriseAttestationSupported && AttestationConveyancePreference.ENTERPRISE.equals(options.getAttestation()) && - userAgentConfiguration.supportsEpForRpId(rpId) && - enterpriseAttestation != null && - (enterpriseAttestation == 1 || enterpriseAttestation == 2)) { - validatedEnterpriseAttestation = enterpriseAttestation; + userAgentConfiguration.supportsEpForRpId(rpId)) { + if (enterpriseAttestation == null || + (enterpriseAttestation != 1 && enterpriseAttestation != 2)) { + Logger.warn(logger, "Invalid value for enterpriseAttestation parameter."); + } else { + validatedEnterpriseAttestation = enterpriseAttestation; + } } return ctap.makeCredential( diff --git a/fido/src/main/java/com/yubico/yubikit/fido/ctap/ClientPin.java b/fido/src/main/java/com/yubico/yubikit/fido/ctap/ClientPin.java index 36976ea5..94fa95ac 100755 --- a/fido/src/main/java/com/yubico/yubikit/fido/ctap/ClientPin.java +++ b/fido/src/main/java/com/yubico/yubikit/fido/ctap/ClientPin.java @@ -175,6 +175,11 @@ public byte[] getPinToken(char[] pin, ? CMD_GET_PIN_TOKEN_USING_PIN_WITH_PERMISSIONS : CMD_GET_PIN_TOKEN; + if (!tokenSupported && (permissions != null || permissionsRpId != null)) { + Logger.warn(logger, "Ignoring permissions/permissionsRpId parameters as the " + + " authenticator does not support PIN U/V Token"); + } + Map result = ctap.clientPin( pinUvAuth.getVersion(), subCommand, From bc27e72efd8100d525c8487bcbcda9be9f726316 Mon Sep 17 00:00:00 2001 From: Adam Velebil Date: Tue, 21 Nov 2023 11:31:35 +0100 Subject: [PATCH 3/4] Revert "warn about incompatible/invalid parameters" This reverts commit 94b28c3d012a8258e7253c6086b186a94d1bafc0. --- .../fido/client/BasicWebAuthnClient.java | 21 +++++-------------- .../yubico/yubikit/fido/ctap/ClientPin.java | 5 ----- 2 files changed, 5 insertions(+), 21 deletions(-) diff --git a/fido/src/main/java/com/yubico/yubikit/fido/client/BasicWebAuthnClient.java b/fido/src/main/java/com/yubico/yubikit/fido/client/BasicWebAuthnClient.java index 83a3424b..d776a2b7 100644 --- a/fido/src/main/java/com/yubico/yubikit/fido/client/BasicWebAuthnClient.java +++ b/fido/src/main/java/com/yubico/yubikit/fido/client/BasicWebAuthnClient.java @@ -19,7 +19,6 @@ import com.yubico.yubikit.core.application.CommandException; import com.yubico.yubikit.core.application.CommandState; import com.yubico.yubikit.core.fido.CtapException; -import com.yubico.yubikit.core.internal.Logger; import com.yubico.yubikit.fido.ctap.ClientPin; import com.yubico.yubikit.fido.ctap.CredentialManagement; import com.yubico.yubikit.fido.ctap.Ctap2Session; @@ -450,22 +449,12 @@ protected Ctap2Session.CredentialData ctapMakeCredential( } @Nullable Integer validatedEnterpriseAttestation = null; - boolean enterpriseAttestationSupported = isEnterpriseAttestationSupported(); - - if (!enterpriseAttestationSupported && enterpriseAttestation != null) { - Logger.warn(logger, "Ignoring provided enterpriseAttestation parameter because" + - " the authenticator does not support enterprise attestation."); - } - - if (enterpriseAttestationSupported && + if (isEnterpriseAttestationSupported() && AttestationConveyancePreference.ENTERPRISE.equals(options.getAttestation()) && - userAgentConfiguration.supportsEpForRpId(rpId)) { - if (enterpriseAttestation == null || - (enterpriseAttestation != 1 && enterpriseAttestation != 2)) { - Logger.warn(logger, "Invalid value for enterpriseAttestation parameter."); - } else { - validatedEnterpriseAttestation = enterpriseAttestation; - } + userAgentConfiguration.supportsEpForRpId(rpId) && + enterpriseAttestation != null && + (enterpriseAttestation == 1 || enterpriseAttestation == 2)) { + validatedEnterpriseAttestation = enterpriseAttestation; } return ctap.makeCredential( diff --git a/fido/src/main/java/com/yubico/yubikit/fido/ctap/ClientPin.java b/fido/src/main/java/com/yubico/yubikit/fido/ctap/ClientPin.java index 94fa95ac..36976ea5 100755 --- a/fido/src/main/java/com/yubico/yubikit/fido/ctap/ClientPin.java +++ b/fido/src/main/java/com/yubico/yubikit/fido/ctap/ClientPin.java @@ -175,11 +175,6 @@ public byte[] getPinToken(char[] pin, ? CMD_GET_PIN_TOKEN_USING_PIN_WITH_PERMISSIONS : CMD_GET_PIN_TOKEN; - if (!tokenSupported && (permissions != null || permissionsRpId != null)) { - Logger.warn(logger, "Ignoring permissions/permissionsRpId parameters as the " + - " authenticator does not support PIN U/V Token"); - } - Map result = ctap.clientPin( pinUvAuth.getVersion(), subCommand, From 5525a7af7ed686a83d316688ca3cad44e9698524 Mon Sep 17 00:00:00 2001 From: Adam Velebil Date: Tue, 21 Nov 2023 11:39:47 +0100 Subject: [PATCH 4/4] don't unnecessarily copy memory --- .../com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fido/src/main/java/com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java b/fido/src/main/java/com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java index 00e17f4f..d35b3ac9 100644 --- a/fido/src/main/java/com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java +++ b/fido/src/main/java/com/yubico/yubikit/fido/ctap/PinUvAuthProtocolV2.java @@ -117,8 +117,7 @@ public byte[] decrypt(byte[] key, byte[] ciphertext) { aesKey = Arrays.copyOfRange(key, 32, key.length); byte[] iv = Arrays.copyOf(ciphertext, 16); byte[] ct = Arrays.copyOfRange(ciphertext, 16, ciphertext.length); - byte[] plaintext = getCipher(Cipher.DECRYPT_MODE, aesKey, iv).doFinal(ct); - return Arrays.copyOf(plaintext, plaintext.length); + return getCipher(Cipher.DECRYPT_MODE, aesKey, iv).doFinal(ct); } catch (BadPaddingException | IllegalBlockSizeException e) { throw new IllegalStateException(e); } finally { @@ -139,8 +138,7 @@ public byte[] authenticate(byte[] key, byte[] message) { } catch (NoSuchAlgorithmException | InvalidKeyException e) { throw new RuntimeException(e); } - byte[] result = mac.doFinal(message); - return Arrays.copyOf(result, result.length); + return mac.doFinal(message); } private Cipher getCipher(int mode, byte[] secret, byte[] iv) {