diff options
author | Bjørn Christian Seime <bjorn.christian@seime.no> | 2023-07-19 17:09:28 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-07-19 17:09:28 +0200 |
commit | 48212ae35bea422c44f09511984fdcdabae23106 (patch) | |
tree | 5bb2ef86b6de1e963f408880abcd00b9764a5fe6 | |
parent | 6f0ad0e494badcc8a6f0dd077d3cd7ef8339a1f8 (diff) | |
parent | 3144d978886984f57344e1580326749b25614cb6 (diff) |
Merge pull request #27834 from vespa-engine/vekterli/defer-side-channel-safe-array-ops-to-bc
Defer side channel-safe array checks to existing BC utils
3 files changed, 13 insertions, 24 deletions
diff --git a/security-utils/src/main/java/com/yahoo/security/SideChannelSafe.java b/security-utils/src/main/java/com/yahoo/security/SideChannelSafe.java index bd085f6f624..3a46891085f 100644 --- a/security-utils/src/main/java/com/yahoo/security/SideChannelSafe.java +++ b/security-utils/src/main/java/com/yahoo/security/SideChannelSafe.java @@ -1,6 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.security; +import org.bouncycastle.util.Arrays; + /** * Utility functions for comparing the contents of arrays without leaking information about the * data contained within them via timing side-channels. This is done by avoiding any branches @@ -13,18 +15,11 @@ package com.yahoo.security; public class SideChannelSafe { /** - * @return true iff all bytes in the array are zero. An empty array always returns false - * since it technically can't contain any zeros at all. + * @return true iff all bytes in the array are zero. An empty array always returns true + * to be in line with BouncyCastle semantics. */ public static boolean allZeros(byte[] buf) { - if (buf.length == 0) { - return false; - } - byte accu = 0; - for (byte b : buf) { - accu |= b; - } - return (accu == 0); + return Arrays.areAllZeroes(buf, 0, buf.length); } /** @@ -32,23 +27,14 @@ public class SideChannelSafe { * about the contents of either of the arrays. * * <strong>Important:</strong> the <em>length</em> of the arrays is not considered secret, and - * will be leaked if arrays of differing sizes are given. + * <em>may</em> be leaked if arrays of differing sizes are given. * * @param lhs first array of bytes to compare * @param rhs second array of bytes to compare * @return true iff both arrays have the same size and are element-wise identical */ public static boolean arraysEqual(byte[] lhs, byte[] rhs) { - if (lhs.length != rhs.length) { - return false; - } - // Only use constant time bitwise ops. `accu` will be non-zero if at least one bit - // differed in any byte compared between the two arrays. - byte accu = 0; - for (int i = 0; i < lhs.length; ++i) { - accu |= (byte)(lhs[i] ^ rhs[i]); - } - return (accu == 0); + return Arrays.constantTimeAreEqual(lhs, rhs); } } diff --git a/security-utils/src/main/java/com/yahoo/security/token/TokenCheckHash.java b/security-utils/src/main/java/com/yahoo/security/token/TokenCheckHash.java index 2ff47081784..b67b120ba7b 100644 --- a/security-utils/src/main/java/com/yahoo/security/token/TokenCheckHash.java +++ b/security-utils/src/main/java/com/yahoo/security/token/TokenCheckHash.java @@ -1,6 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.security.token; +import com.yahoo.security.SideChannelSafe; + import java.util.Arrays; import static com.yahoo.security.ArrayUtils.hex; @@ -18,8 +20,9 @@ public record TokenCheckHash(byte[] hashBytes) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; TokenCheckHash tokenCheckHash = (TokenCheckHash) o; - // We don't consider token hashes secret data, so no harm in data-dependent equals() - return Arrays.equals(hashBytes, tokenCheckHash.hashBytes); + // Although not considered secret information, avoid leaking the contents of + // the check-hashes themselves via timing channels. + return SideChannelSafe.arraysEqual(hashBytes, tokenCheckHash.hashBytes); } @Override diff --git a/security-utils/src/test/java/com/yahoo/security/SideChannelSafeTest.java b/security-utils/src/test/java/com/yahoo/security/SideChannelSafeTest.java index 7a66ed6eb7f..9731bbffc38 100644 --- a/security-utils/src/test/java/com/yahoo/security/SideChannelSafeTest.java +++ b/security-utils/src/test/java/com/yahoo/security/SideChannelSafeTest.java @@ -14,7 +14,7 @@ public class SideChannelSafeTest { @Test void all_zeros_checks_length_and_array_contents() { - assertFalse(SideChannelSafe.allZeros(new byte[0])); + assertTrue(SideChannelSafe.allZeros(new byte[0])); assertFalse(SideChannelSafe.allZeros(new byte[]{ 1 })); assertTrue(SideChannelSafe.allZeros(new byte[]{ 0 })); assertFalse(SideChannelSafe.allZeros(new byte[]{ 0, 0, 127, 0 })); |