From bd07154242664e9d25ba60333e1a497562aa7c27 Mon Sep 17 00:00:00 2001 From: Frank Denis Date: Sun, 23 Aug 2020 01:36:37 +0200 Subject: [PATCH 1/2] Add mem.timingSafeEql() for constant-time array comparison This is a trivial implementation that just does a or[xor] loop. However, this pattern is used by virtually all crypto libraries and in practice, even without assembly barriers, LLVM never turns it into code with conditional jumps, even if one of the parameters is constant. This has been verified to still be the case with LLVM 11.0.0. --- lib/std/crypto.zig | 2 + lib/std/crypto/bcrypt.zig | 3 +- lib/std/crypto/ghash.zig | 3 +- lib/std/crypto/poly1305.zig | 3 +- lib/std/crypto/salsa20.zig | 5 ++- lib/std/crypto/utils.zig | 86 +++++++++++++++++++++++++++++++++++++ lib/std/mem.zig | 20 --------- 7 files changed, 97 insertions(+), 25 deletions(-) create mode 100644 lib/std/crypto/utils.zig diff --git a/lib/std/crypto.zig b/lib/std/crypto.zig index c2a6fb6730..a30ec9728e 100644 --- a/lib/std/crypto.zig +++ b/lib/std/crypto.zig @@ -130,6 +130,8 @@ pub const nacl = struct { pub const SealedBox = salsa20.SealedBox; }; +pub const utils = @import("crypto/utils.zig"); + const std = @import("std.zig"); pub const randomBytes = std.os.getrandom; diff --git a/lib/std/crypto/bcrypt.zig b/lib/std/crypto/bcrypt.zig index 179d904494..4cec59961b 100644 --- a/lib/std/crypto/bcrypt.zig +++ b/lib/std/crypto/bcrypt.zig @@ -11,6 +11,7 @@ const math = std.math; const mem = std.mem; const debug = std.debug; const testing = std.testing; +const utils = std.crypto.utils; const salt_length: usize = 16; const salt_str_length: usize = 22; @@ -226,7 +227,7 @@ fn strHashInternal(password: []const u8, rounds_log: u6, salt: [salt_length]u8) state.expand0(passwordZ); state.expand0(salt[0..]); } - mem.secureZero(u8, &password_buf); + utils.secureZero(u8, &password_buf); var cdata = [6]u32{ 0x4f727068, 0x65616e42, 0x65686f6c, 0x64657253, 0x63727944, 0x6f756274 }; // "OrpheanBeholderScryDoubt" k = 0; diff --git a/lib/std/crypto/ghash.zig b/lib/std/crypto/ghash.zig index 36ce7b2c88..d5d4ae98ea 100644 --- a/lib/std/crypto/ghash.zig +++ b/lib/std/crypto/ghash.zig @@ -10,6 +10,7 @@ const std = @import("../std.zig"); const assert = std.debug.assert; const math = std.math; const mem = std.mem; +const utils = std.crypto.utils; /// GHASH is a universal hash function that features multiplication /// by a fixed parameter within a Galois field. @@ -305,7 +306,7 @@ pub const Ghash = struct { mem.writeIntBig(u64, out[0..8], st.y1); mem.writeIntBig(u64, out[8..16], st.y0); - mem.secureZero(u8, @ptrCast([*]u8, st)[0..@sizeOf(Ghash)]); + utils.secureZero(u8, @ptrCast([*]u8, st)[0..@sizeOf(Ghash)]); } pub fn create(out: *[mac_length]u8, msg: []const u8, key: *const [key_length]u8) void { diff --git a/lib/std/crypto/poly1305.zig b/lib/std/crypto/poly1305.zig index 5b1554b113..0b7b4cd64a 100644 --- a/lib/std/crypto/poly1305.zig +++ b/lib/std/crypto/poly1305.zig @@ -4,6 +4,7 @@ // The MIT license requires this copyright notice to be included in all copies // and substantial portions of the software. const std = @import("../std.zig"); +const utils = std.crypto.utils; const mem = std.mem; pub const Poly1305 = struct { @@ -195,7 +196,7 @@ pub const Poly1305 = struct { mem.writeIntLittle(u64, out[0..8], st.h[0]); mem.writeIntLittle(u64, out[8..16], st.h[1]); - std.mem.secureZero(u8, @ptrCast([*]u8, st)[0..@sizeOf(Poly1305)]); + utils.secureZero(u8, @ptrCast([*]u8, st)[0..@sizeOf(Poly1305)]); } pub fn create(out: *[mac_length]u8, msg: []const u8, key: *const [key_length]u8) void { diff --git a/lib/std/crypto/salsa20.zig b/lib/std/crypto/salsa20.zig index f12a4039dc..dd3e4fe99b 100644 --- a/lib/std/crypto/salsa20.zig +++ b/lib/std/crypto/salsa20.zig @@ -9,6 +9,7 @@ const crypto = std.crypto; const debug = std.debug; const math = std.math; const mem = std.mem; +const utils = std.crypto.utils; const Vector = std.meta.Vector; const Poly1305 = crypto.onetimeauth.Poly1305; @@ -414,7 +415,7 @@ pub const XSalsa20Poly1305 = struct { acc |= computedTag[i] ^ tag[i]; } if (acc != 0) { - mem.secureZero(u8, &computedTag); + utils.secureZero(u8, &computedTag); return error.AuthenticationFailed; } mem.copy(u8, m[0..mlen0], block0[32..][0..mlen0]); @@ -532,7 +533,7 @@ pub const SealedBox = struct { const nonce = createNonce(ekp.public_key, public_key); mem.copy(u8, c[0..public_length], ekp.public_key[0..]); try Box.seal(c[Box.public_length..], m, nonce, public_key, ekp.secret_key); - mem.secureZero(u8, ekp.secret_key[0..]); + utils.secureZero(u8, ekp.secret_key[0..]); } /// Decrypt a message using a key pair. diff --git a/lib/std/crypto/utils.zig b/lib/std/crypto/utils.zig new file mode 100644 index 0000000000..f6dc44e7c6 --- /dev/null +++ b/lib/std/crypto/utils.zig @@ -0,0 +1,86 @@ +const std = @import("../std.zig"); +const mem = std.mem; +const testing = std.testing; + +/// Compares two arrays in constant time (for a given length) and returns whether they are equal. +/// This function was designed to compare short cryptographic secrets (MACs, signatures). +/// For all other applications, use mem.eql() instead. +pub fn timingSafeEql(comptime T: type, a: T, b: T) bool { + switch (@typeInfo(T)) { + .Array => |info| { + const C = info.child; + if (@typeInfo(C) != .Int) { + @compileError("Elements to be compared must be integers"); + } + var acc = @as(C, 0); + for (a) |x, i| { + acc |= x ^ b[i]; + } + comptime const s = @typeInfo(C).Int.bits; + comptime const Cu = std.meta.Int(.unsigned, s); + comptime const Cext = std.meta.Int(.unsigned, s + 1); + return @bitCast(bool, @truncate(u1, (@as(Cext, @bitCast(Cu, acc)) -% 1) >> s)); + }, + .Vector => |info| { + const C = info.child; + if (@typeInfo(C) != .Int) { + @compileError("Elements to be compared must be integers"); + } + const z = a ^ b; + var acc = @as(C, 0); + var i: usize = 0; + while (i < info.len) : (i += 1) { + acc |= z[i]; + } + comptime const s = @typeInfo(C).Int.bits; + comptime const Cu = std.meta.Int(.unsigned, s); + comptime const Cext = std.meta.Int(.unsigned, s + 1); + return @bitCast(bool, @truncate(u1, (@as(Cext, @bitCast(Cu, acc)) -% 1) >> s)); + }, + else => { + @compileError("Only arrays and vectors can be compared"); + }, + } +} + +/// Sets a slice to zeroes. +/// Prevents the store from being optimized out. +pub fn secureZero(comptime T: type, s: []T) void { + // NOTE: We do not use a volatile slice cast here since LLVM cannot + // see that it can be replaced by a memset. + const ptr = @ptrCast([*]volatile u8, s.ptr); + const length = s.len * @sizeOf(T); + @memset(ptr, 0, length); +} + +test "crypto.utils.timingSafeEql" { + var a: [100]u8 = undefined; + var b: [100]u8 = undefined; + try std.crypto.randomBytes(a[0..]); + try std.crypto.randomBytes(b[0..]); + testing.expect(!timingSafeEql([100]u8, a, b)); + mem.copy(u8, a[0..], b[0..]); + testing.expect(timingSafeEql([100]u8, a, b)); +} + +test "crypto.utils.timingSafeEql (vectors)" { + var a: [100]u8 = undefined; + var b: [100]u8 = undefined; + try std.crypto.randomBytes(a[0..]); + try std.crypto.randomBytes(b[0..]); + const v1: std.meta.Vector(100, u8) = a; + const v2: std.meta.Vector(100, u8) = b; + testing.expect(!timingSafeEql(std.meta.Vector(100, u8), v1, v2)); + const v3: std.meta.Vector(100, u8) = a; + testing.expect(timingSafeEql(std.meta.Vector(100, u8), v1, v3)); +} + +test "crypto.utils.secureZero" { + var a = [_]u8{0xfe} ** 8; + var b = [_]u8{0xfe} ** 8; + + mem.set(u8, a[0..], 0); + secureZero(u8, b[0..]); + + testing.expectEqualSlices(u8, a[0..], b[0..]); +} diff --git a/lib/std/mem.zig b/lib/std/mem.zig index 532c34e41c..e4d46dbf29 100644 --- a/lib/std/mem.zig +++ b/lib/std/mem.zig @@ -342,26 +342,6 @@ test "mem.zeroes" { testing.expectEqual(@as(u8, 0), c.a); } -/// Sets a slice to zeroes. -/// Prevents the store from being optimized out. -pub fn secureZero(comptime T: type, s: []T) void { - // NOTE: We do not use a volatile slice cast here since LLVM cannot - // see that it can be replaced by a memset. - const ptr = @ptrCast([*]volatile u8, s.ptr); - const length = s.len * @sizeOf(T); - @memset(ptr, 0, length); -} - -test "mem.secureZero" { - var a = [_]u8{0xfe} ** 8; - var b = [_]u8{0xfe} ** 8; - - set(u8, a[0..], 0); - secureZero(u8, b[0..]); - - testing.expectEqualSlices(u8, a[0..], b[0..]); -} - /// Initializes all fields of the struct with their default value, or zero values if no default value is present. /// If the field is present in the provided initial values, it will have that value instead. /// Structs are initialized recursively. From 7f9e3e419c24ba51e80a4c41bcbefc820f7e0a88 Mon Sep 17 00:00:00 2001 From: Frank Denis Date: Sat, 7 Nov 2020 20:30:13 +0100 Subject: [PATCH 2/2] Use @reduce --- lib/std/crypto/utils.zig | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/std/crypto/utils.zig b/lib/std/crypto/utils.zig index f6dc44e7c6..33ad6360f6 100644 --- a/lib/std/crypto/utils.zig +++ b/lib/std/crypto/utils.zig @@ -26,12 +26,7 @@ pub fn timingSafeEql(comptime T: type, a: T, b: T) bool { if (@typeInfo(C) != .Int) { @compileError("Elements to be compared must be integers"); } - const z = a ^ b; - var acc = @as(C, 0); - var i: usize = 0; - while (i < info.len) : (i += 1) { - acc |= z[i]; - } + const acc = @reduce(.Or, a ^ b); comptime const s = @typeInfo(C).Int.bits; comptime const Cu = std.meta.Int(.unsigned, s); comptime const Cext = std.meta.Int(.unsigned, s + 1);