[Fix] bchec: Constant time field normalization by zquestz · Pull Request #78 · gcash/bchd

zquestz · 2018-10-26T04:28:26Z

Port of the work done by @bmperrea in btcsuite/btcd#1084

The current implementation of field.Normalize() has a timing vulnerability. This makes it constant time.

cpacia · 2018-10-30T01:28:21Z

I'm not really qualified to evaluate this one

bmperrea · 2018-10-30T16:26:25Z

Obviously I approve 👍

zquestz · 2018-10-31T00:25:50Z

@bmperrea any other individuals from btcd have context on these types of changes that you can recommend for review? @Roasbeef perhaps?

zquestz · 2018-11-09T00:15:03Z

Any chance @davecgh or @stevenroose can review this? I like the change but would love another set of eyes on this!

zquestz · 2018-12-15T22:21:33Z

@jimpo would love if you checked this out.

jimpo

Nice work! Looks correct to me.

However, given that the constant_time routines have the same requirements as crypto/subtle (that all values are at most 31 bits) and the casts don't matter, I see no reason not to use crypto/subtle directly. I'd recommend something more like jimpo@c13c877.

jimpo · 2018-12-28T22:02:20Z

bchec/bench_test.go

+// BenchmarkFieldNormalize2 requires the evaluation of more involved logic in
+// field.Normalize(). We should ensure the runtime is the same as in
+// BenchmarkFieldNormalize
+func BenchmarkFieldNormalize2(b *testing.B) {


nit: BenchmarkFieldNormalizeWithCarry seems like a better name.

Also, I'm not sure this provides much benefit since it doesn't automatically compare the timing against regular BenchmarkFieldNormalize.

jimpo · 2018-12-28T22:03:00Z

bchec/bench_test.go

+// BenchmarkFieldNormalize
+func BenchmarkFieldNormalize2(b *testing.B) {
+	f := new(fieldVal)
+	f.n = [10]uint32{0x148f6, 0x3ffffc0, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x000007}


Might as well just construct the fieldVal directly: f := &fieldVal{ [10]uint32{...} }

jimpo · 2018-12-28T22:04:39Z

bchec/constant_time.go

+// It works by checking the most significant bit, and then testing the
+// rest of the bits by casting to int32
+func lessThanUint32(x, y uint32) uint32 {
+	diff := int32(x) - int32(y)


All of the casts to/from int32 are useless. uint32s overflow/underflow in the exact same way.

great point!

I will try to update my branch in btcd accordingly

jimpo · 2018-12-29T22:56:31Z

bchec/field_test.go

 			[10]uint32{0x03fffc30, 0x03ffffc0, 0x03ffffff, 0x03ffffff, 0x03ffffff, 0x03ffffff, 0x03ffffff, 0x03ffffff, 0x07ffffff, 0x003fffff},
 			[10]uint32{0x00000001, 0x00000001, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000001},
 		},
+		// A more difficult branch for the Normalize logic


Better comment:

// Number less than P that satisfies two of the three overflow // conditions for a field value of magnitude of 1 with value greater // than P in all but the last word.

jimpo · 2018-12-29T22:57:03Z

bchec/field_test.go

+			[10]uint32{0x148f6, 0x3ffffc0, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x000007},
+			[10]uint32{0x148f6, 0x3ffffc0, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x3ffffff, 0x000007},
+		},
+		// A test that would fail without the check t2&t3&t4&t5&t6&t7&t8 == fieldBaseMask before normalization


Better comment:

// Number less than P that satisfies two of the three overflow // conditions for a field value of magnitude of 1 with value greater // than P in all but the second to last word.

bmperrea · 2018-12-30T21:57:12Z

@jimpo I suspect re-writing the crypto/subtle functions in uint32 provides a non-negligible speedup, especially given that the casts I was doing were unnecessary. Also having a strict equality checker (LessThan) makes things more readable in this case I think, so I would still support writing the functions for this specific case here. I'll try to do a benchmark and update my btcd branch.

bmperrea · 2018-12-30T22:04:42Z

Also thanks for the great reveiw @jimpo!

cpacia · 2019-01-01T01:44:36Z

Thanks for reviewing @jimpo !

zquestz · 2019-01-07T19:17:28Z

@bmperrea let me know when the btcd branch is updated and I will port the changes here. =)

Thanks again for the review @jimpo =)

markblundeberg

Hiya, @zquestz suggested I do a drive-by review of this languishing PR.
(bear in mind: I'm more of a C++ programmer and have never used go)

markblundeberg · 2019-11-17T16:03:41Z

bchec/constant_time.go

+	return uint32((diff >> 31) & 1)
+}
+
+// isZeroUint32 returns 1 if x == y and 0 otherwise.


What's "y"?

hmm mistaken comment should be like // isZeroUint32 returns 1 if x == 0 and 0 otherwise.

Therefore y is zero (kidding).

markblundeberg · 2019-11-17T16:08:18Z

bchec/constant_time.go

+// isZeroUint32 returns 1 if x == y and 0 otherwise.
+func isZeroUint32(x uint32) uint32 {
+	x32 := int32(x)
+	return uint32((((x32 - 1) ^ x32) >> 31) & 1)


I agree, the cast to int32 seems unnecessary and IMO technically "worse" since it assumes two's complement representation.

OK yes two's complement is basically universal nowadays. :-) But isn't uint32 specified to be a modulo arithmetic type in go? If so then you don't need to bother about sign bits, this is just good clean mod-2^32 arithmetic. Also btw the final &1 seems to be unneeded in each case.

(also, these are neat bit tricks, I like :-) This trick works because there are only two values for which, upon subtracting 1, the high bit is flipped: 0x80000000 and 0x00000000. Since you assume everything must be less than the first value, then it's good.)

markblundeberg · 2019-11-17T16:30:17Z

bchec/constant_time.go

+// notZeroUint32 returns 1 if x != y and 0 otherwise.
+func notZeroUint32(x uint32) uint32 {
+	x32 := int32(x)
+	return uint32((((-x32) | x32) >> 31) & 1)


(this trick works because there is only one value which has a high bit of 0, both before and after negation: 0x00000000.)

markblundeberg · 2019-11-17T16:53:45Z

bchec/field.go

+	// are true and the value will not be changed when 'm' is zero.
+	m = isZeroUint32((t9 - fieldMSBMask) | (t2&t3&t4&t5&t6&t7&t8 - fieldBaseMask))
+	m &= lessThanUint32(fieldBaseMask, (t0+977)>>fieldBase+t1+64)
+	m |= notZeroUint32(t9 >> fieldMSBBits)


For reference, this seems to get done more simply in libsecp which also has an implementation of the same 10x26 field limbing: https://github.com/bitcoin-core/secp256k1/blob/0c774d89e61808d883b8f2df74462421491d59bb/src/field_10x26_impl.h#L66-L67

I have to admit I don't understand this deeply enough to know how either works.

(for what it's worth, this PR is at least clearly identical to the code being modified, so there is nothing to worry about in terms of changed behaviour)

OK I see now this is identical with libsecp... the two different meanings of 'm' was confusing me.

The libsecp expression is still simpler, for example it takes advantage of the fact that t9 >> fieldMSBBits is surely either 0 or 1 at this point. You could do the same by making the final line to be m |= t9 >> fieldMSBBits.

And libsecp uses equality comparisons along with implicit bool -> int cast (i.e., in the assembly it calls cmp which just does a subtraction anyway but only uses the result to set flags, then sete casts the flag to an int). Unfortunately it looks like in go you might not have a simple bool->int cast so this is about as good as it gets?

Yes, the lack of bool->int cast is an unfortunate choice in golang when you want constant time math. This is as good as it gets and follows the standard patterns in crypto/subtle.

It would be great to incorporate simplifications and improvements based on the libsecp implementation, modulo lack of above casts and different language standards for constants-as-variables.

markblundeberg · 2019-11-17T19:42:46Z

@bmperrea ping ^

bmperrea

Happy to get any other help to update the PR based on the excellent feedback here. Unfortunately I'm not sure when I will have the chance to do so myself.

bmperrea · 2019-11-18T01:20:50Z

bchec/field.go

+	// are true and the value will not be changed when 'm' is zero.
+	m = isZeroUint32((t9 - fieldMSBMask) | (t2&t3&t4&t5&t6&t7&t8 - fieldBaseMask))
+	m &= lessThanUint32(fieldBaseMask, (t0+977)>>fieldBase+t1+64)
+	m |= notZeroUint32(t9 >> fieldMSBBits)


Yes, the lack of bool->int cast is an unfortunate choice in golang when you want constant time math. This is as good as it gets and follows the standard patterns in crypto/subtle.

It would be great to incorporate simplifications and improvements based on the libsecp implementation, modulo lack of above casts and different language standards for constants-as-variables.

bmperrea · 2019-11-18T01:25:04Z

bchec/constant_time.go

+	return uint32((diff >> 31) & 1)
+}
+
+// isZeroUint32 returns 1 if x == y and 0 otherwise.


hmm mistaken comment should be like // isZeroUint32 returns 1 if x == 0 and 0 otherwise.

Therefore y is zero (kidding).

bmperrea · 2019-11-18T04:58:37Z

Updated @markblundeberg ! @jimpo after benchmarking a bit using benchstat with -count=5 (on this commit) I found no statistically significant performance hit for casting and using crypto/subtle directly, even after the optimizations discussed here. Therefore I switched to just using those, making the PR much simpler without the constant math helpers and tests.

bmperrea · 2019-11-18T13:58:46Z

Ping @zquestz !

markblundeberg · 2019-11-19T03:42:59Z

Yeah new one seems OK also.

btcec: Constant time field normalization

a190c2e

zquestz requested review from a user and cpacia October 26, 2018 04:28

Add constant time files

f23b33a

zquestz changed the title ~~btcec: Constant time field normalization~~ [Fix] btcec: Constant time field normalization Oct 26, 2018

bmperrea approved these changes Oct 30, 2018

View reviewed changes

cpacia added the needs review Needs to be reviewed by the maintainers label Dec 5, 2018

jimpo suggested changes Dec 29, 2018

View reviewed changes

cpacia force-pushed the master branch 3 times, most recently from 59fd420 to 25c2c60 Compare August 21, 2019 04:54

markblundeberg reviewed Nov 17, 2019

View reviewed changes

bmperrea reviewed Nov 18, 2019

View reviewed changes

bmperrea mentioned this pull request Nov 18, 2019

btcec: Constant time field normalization. btcsuite/btcd#1084

Open

zquestz changed the title ~~[Fix] btcec: Constant time field normalization~~ [Fix] bchec: Constant time field normalization Jul 20, 2020

cpacia force-pushed the master branch from 1caf4f8 to bd11e95 Compare July 25, 2020 03:18

Conversation

zquestz commented Oct 26, 2018

Uh oh!

cpacia commented Oct 30, 2018

Uh oh!

bmperrea commented Oct 30, 2018

Uh oh!

zquestz commented Oct 31, 2018

Uh oh!

zquestz commented Nov 9, 2018

Uh oh!

zquestz commented Dec 15, 2018

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmperrea commented Dec 30, 2018

Uh oh!

bmperrea commented Dec 30, 2018

Uh oh!

cpacia commented Jan 1, 2019

Uh oh!

zquestz commented Jan 7, 2019

Uh oh!

markblundeberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmperrea Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markblundeberg commented Nov 17, 2019

Uh oh!

bmperrea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmperrea Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmperrea commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmperrea commented Nov 18, 2019

Uh oh!

markblundeberg commented Nov 19, 2019

Uh oh!

Reviewers

Assignees

Labels

bmperrea Nov 18, 2019 •

edited

Loading

bmperrea Nov 18, 2019 •

edited

Loading

bmperrea commented Nov 18, 2019 •

edited

Loading