From 6c6ff1888eb7225ef42acbf5442f546ac41b8903 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Fri, 11 Sep 2020 11:11:58 -0500 Subject: [PATCH 01/14] Replace division with a branch with a constant time impl that's x * invert(y) --- shamir/shamir.go | 36 ++++++++++-------------------------- 1 file changed, 10 insertions(+), 26 deletions(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index f2ba820f1fd2..451e1fd914c5 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -80,34 +80,18 @@ func interpolatePolynomial(x_samples, y_samples []uint8, x uint8) uint8 { return result } -// div divides two numbers in GF(2^8) -func div(a, b uint8) uint8 { - if b == 0 { - // leaks some timing information but we don't care anyways as this - // should never happen, hence the panic - panic("divide by zero") - } - - var goodVal, zero uint8 - log_a := logTable[a] - log_b := logTable[b] - diff := (int(log_a) - int(log_b)) % 255 - if diff < 0 { - diff += 255 - } - - ret := expTable[diff] - - // Ensure we return zero if a is zero but aren't subject to timing attacks - goodVal = ret - - if subtle.ConstantTimeByteEq(a, 0) == 1 { - ret = zero - } else { - ret = goodVal +func invert(x uint8) uint8 { + z := x + for i := 0; i < 6; i ++ { + z = mult(z, z) + z = mult(z, x) } + return mult(z, z) +} - return ret +// div divides two numbers in GF(2^8) +func div(x, y uint8) uint8 { + return mult(x, invert(y)) } // mult multiplies two numbers in GF(2^8) From e3eae37cb52943ba1b907f7d91b3fd479c08038e Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Fri, 11 Sep 2020 11:26:13 -0500 Subject: [PATCH 02/14] Use a table based inverse --- shamir/shamir.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index 451e1fd914c5..b676fb6b0f30 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -80,13 +80,18 @@ func interpolatePolynomial(x_samples, y_samples []uint8, x uint8) uint8 { return result } +// Calculate the multiplicative inverse of x in GF(2^8) func invert(x uint8) uint8 { - z := x - for i := 0; i < 6; i ++ { - z = mult(z, z) - z = mult(z, x) + var zero, goodVal uint8 + /* 0 is self inverting */ + ret := expTable[(255 - logTable[x])] + goodVal = ret + if subtle.ConstantTimeByteEq(x, 0) == 1 { + ret = zero + } else { + ret = goodVal } - return mult(z, z) + return ret } // div divides two numbers in GF(2^8) From 7a2d0e24e18586f6daa20365a19579d434263573 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Fri, 11 Sep 2020 11:30:18 -0500 Subject: [PATCH 03/14] Move the comment --- shamir/shamir.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index b676fb6b0f30..491b5876e8c4 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -83,9 +83,10 @@ func interpolatePolynomial(x_samples, y_samples []uint8, x uint8) uint8 { // Calculate the multiplicative inverse of x in GF(2^8) func invert(x uint8) uint8 { var zero, goodVal uint8 - /* 0 is self inverting */ ret := expTable[(255 - logTable[x])] goodVal = ret + + /* 0 is self inverting */ if subtle.ConstantTimeByteEq(x, 0) == 1 { ret = zero } else { From 49959354d793f04617be3c079fe4fa2693fd29a2 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Fri, 11 Sep 2020 17:55:05 -0500 Subject: [PATCH 04/14] Use ConstantTimeSelect to avoid branching --- shamir/shamir.go | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index 491b5876e8c4..adbac3f187c9 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -102,33 +102,19 @@ func div(x, y uint8) uint8 { // mult multiplies two numbers in GF(2^8) func mult(a, b uint8) (out uint8) { - var goodVal, zero uint8 + var z int log_a := logTable[a] log_b := logTable[b] sum := (int(log_a) + int(log_b)) % 255 - ret := expTable[sum] + ret := int(expTable[sum]) - // Ensure we return zero if either a or b are zero but aren't subject to - // timing attacks - goodVal = ret - - if subtle.ConstantTimeByteEq(a, 0) == 1 { - ret = zero - } else { - ret = goodVal - } + // Jump through some hoops for constant time evaluation. + ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(a, 0), z, ret) + ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(b, 0), z, ret) - if subtle.ConstantTimeByteEq(b, 0) == 1 { - ret = zero - } else { - // This operation does not do anything logically useful. It - // only ensures a constant number of assignments to thwart - // timing attacks. - goodVal = zero - } + return uint8(ret) - return ret } // add combines two numbers in GF(2^8) From 9da59382b3df7ca0bbefd4d60f18cb88ffe663d7 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Mon, 14 Sep 2020 08:33:36 -0500 Subject: [PATCH 05/14] simpler invert --- shamir/shamir.go | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index adbac3f187c9..1ef1ff754a64 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -82,17 +82,8 @@ func interpolatePolynomial(x_samples, y_samples []uint8, x uint8) uint8 { // Calculate the multiplicative inverse of x in GF(2^8) func invert(x uint8) uint8 { - var zero, goodVal uint8 ret := expTable[(255 - logTable[x])] - goodVal = ret - - /* 0 is self inverting */ - if subtle.ConstantTimeByteEq(x, 0) == 1 { - ret = zero - } else { - ret = goodVal - } - return ret + return uint8(subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(x, 0), 0, int(ret))) } // div divides two numbers in GF(2^8) From b971c92c995e506531e93d50b7efd6b9a174950a Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Mon, 14 Sep 2020 08:41:40 -0500 Subject: [PATCH 06/14] simplify --- shamir/shamir.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index 1ef1ff754a64..643f88ea149a 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -93,7 +93,6 @@ func div(x, y uint8) uint8 { // mult multiplies two numbers in GF(2^8) func mult(a, b uint8) (out uint8) { - var z int log_a := logTable[a] log_b := logTable[b] sum := (int(log_a) + int(log_b)) % 255 @@ -101,8 +100,8 @@ func mult(a, b uint8) (out uint8) { ret := int(expTable[sum]) // Jump through some hoops for constant time evaluation. - ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(a, 0), z, ret) - ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(b, 0), z, ret) + ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(a, 0), 0, ret) + ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(b, 0), 0, ret) return uint8(ret) From c07300ceac6c1ad753b8806741636e853015f01a Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Mon, 14 Sep 2020 08:42:47 -0500 Subject: [PATCH 07/14] better comment --- shamir/shamir.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index 643f88ea149a..a1f397532901 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -99,7 +99,8 @@ func mult(a, b uint8) (out uint8) { ret := int(expTable[sum]) - // Jump through some hoops for constant time evaluation. + // Ensure we return zero if either a or b are zero but aren't subject to + // timing attacks ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(a, 0), 0, ret) ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(b, 0), 0, ret) From 5c4a89ed237964cdf97127a83a2b5dcd14e4f381 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Mon, 14 Sep 2020 18:17:37 -0500 Subject: [PATCH 08/14] Don\'t allow div by zero --- shamir/shamir.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/shamir/shamir.go b/shamir/shamir.go index a1f397532901..b79c5da1da72 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -88,6 +88,12 @@ func invert(x uint8) uint8 { // div divides two numbers in GF(2^8) func div(x, y uint8) uint8 { + if y == 0 { + // leaks some timing information but we don't care anyways as this + // should never happen, hence the panic + panic("divide by zero") + } + return mult(x, invert(y)) } From fc93290013a1371f57ac1161e1f62188a6f28a0e Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Tue, 15 Sep 2020 07:13:37 -0500 Subject: [PATCH 09/14] Constant time mod --- shamir/shamir.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index b79c5da1da72..a8168ca5e293 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -87,14 +87,21 @@ func invert(x uint8) uint8 { } // div divides two numbers in GF(2^8) -func div(x, y uint8) uint8 { - if y == 0 { +func div(a, b uint8) uint8 { + if b == 0 { // leaks some timing information but we don't care anyways as this // should never happen, hence the panic panic("divide by zero") } - return mult(x, invert(y)) + log_a := logTable[a] + log_b := logTable[b] + diff := ((int(log_a) - int(log_b))+255)%255 + + ret := int(expTable[diff]) + ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(a, 0), 0, ret) + ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(b, 0), 0, ret) + return uint8(ret) } // mult multiplies two numbers in GF(2^8) @@ -111,7 +118,6 @@ func mult(a, b uint8) (out uint8) { ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(b, 0), 0, ret) return uint8(ret) - } // add combines two numbers in GF(2^8) From 21b33bf7c6d7c43d06dd463fb6f2104016f31aef Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Tue, 15 Sep 2020 07:17:03 -0500 Subject: [PATCH 10/14] comment --- shamir/shamir.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shamir/shamir.go b/shamir/shamir.go index a8168ca5e293..7a8f8c6051f4 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -99,6 +99,8 @@ func div(a, b uint8) uint8 { diff := ((int(log_a) - int(log_b))+255)%255 ret := int(expTable[diff]) + + // Ensure we return zero if a is zero but aren't subject to timing attacks ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(a, 0), 0, ret) ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(b, 0), 0, ret) return uint8(ret) From 8bde40d295f4a63a9272ca6c7be7a4fb2d12679e Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Tue, 15 Sep 2020 07:50:57 -0500 Subject: [PATCH 11/14] Unused --- shamir/shamir.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index 7a8f8c6051f4..c279715768fa 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -80,12 +80,6 @@ func interpolatePolynomial(x_samples, y_samples []uint8, x uint8) uint8 { return result } -// Calculate the multiplicative inverse of x in GF(2^8) -func invert(x uint8) uint8 { - ret := expTable[(255 - logTable[x])] - return uint8(subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(x, 0), 0, int(ret))) -} - // div divides two numbers in GF(2^8) func div(a, b uint8) uint8 { if b == 0 { From 38ba86a3f8aebc1d7cfaaf8628891a8655dc2b6c Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 16 Sep 2020 13:52:16 -0500 Subject: [PATCH 12/14] Get rid of unnecessary comparison of b --- shamir/shamir.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/shamir/shamir.go b/shamir/shamir.go index c279715768fa..f3fe4deb8b39 100644 --- a/shamir/shamir.go +++ b/shamir/shamir.go @@ -94,9 +94,8 @@ func div(a, b uint8) uint8 { ret := int(expTable[diff]) - // Ensure we return zero if a is zero but aren't subject to timing attacks + // Ensure we return zero if a is zero but aren't subject to timing attacks ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(a, 0), 0, ret) - ret = subtle.ConstantTimeSelect(subtle.ConstantTimeByteEq(b, 0), 0, ret) return uint8(ret) } From bf1976402a7f73689d642d9f36afc28367973a57 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 16 Sep 2020 17:16:51 -0500 Subject: [PATCH 13/14] changelog++ --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5130ea14704f..38a5b2278fd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ BUG FIXES: * replication (enterprise): Only write failover cluster addresses if they've changed * secrets/database: Fix handling of TLS options in mongodb connection strings [[GH-9519](https://github.com/hashicorp/vault/pull/9519)] * secrets/gcp: Ensure that the IAM policy version is appropriately set after a roleset's bindings have changed. [[GH-93](https://github.com/hashicorp/vault-plugin-secrets-gcp/pull/93)] +* core: implement constant time version of shamir GF(2^8) math [[GH-9932](https://github.com/hashicorp/vault/pull/9932)] ## 1.5.3 ### August 27th, 2020 From 31c012681c2ff9dc6e056e1624a0d50e4fb25df0 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 16 Sep 2020 17:18:11 -0500 Subject: [PATCH 14/14] caps --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a5b2278fd2..96f1c3b6b08b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ BUG FIXES: * replication (enterprise): Only write failover cluster addresses if they've changed * secrets/database: Fix handling of TLS options in mongodb connection strings [[GH-9519](https://github.com/hashicorp/vault/pull/9519)] * secrets/gcp: Ensure that the IAM policy version is appropriately set after a roleset's bindings have changed. [[GH-93](https://github.com/hashicorp/vault-plugin-secrets-gcp/pull/93)] -* core: implement constant time version of shamir GF(2^8) math [[GH-9932](https://github.com/hashicorp/vault/pull/9932)] +* core: Implement constant time version of shamir GF(2^8) math [[GH-9932](https://github.com/hashicorp/vault/pull/9932)] ## 1.5.3 ### August 27th, 2020