Use XOR instead of subtraction for constant-time equality checks. #119472
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the constant-time equality implementation by replacing subtraction with XOR operations in the FixedTimeEquals method. The change enhances security consistency with industry standards and provides better protection against timing-based side-channel attacks.
Key Changes
- Replaces subtraction operation (
left[i] - right[i]) with XOR operation (left[i] ^ right[i]) - Renames the accumulator variable from
accumtodiffto better reflect its purpose - Updates loop variable declaration to use
varinstead of explicit type
|
|
||
| for (int i = 0; i < length; i++) | ||
| int diff = 0; | ||
| for (var i = 0; i < length; i++) |
There was a problem hiding this comment.
Use explicit type int instead of var for the loop variable to maintain consistency with the existing codebase style. The variable length is already declared as int on line 49.
Copilot generated this review using guidance from repository custom instructions.
dotnet-policy-service
bot
added
the
community-contribution
XOR directly expresses the statement "are these bytes different"; the use of subtraction changes that to "what is the difference between these bytes" The agreed implementation for constant-time security is to avoid arithmetic that may introduce timing variations between platform/processor/compiler. XOR operations `a ^ b` produces a non-zero result only when bits differ. XOR also benefits from being its own inverse without requiring extra information like carry bits or sign bits, and though somewhat irrelevant with built in overflow checks, doesn't carry any risk. XOR is theoretically cheaper at a CPU level; though I mention this more for fun than "this would have a huge impact on perf"; Theoretically that can matter in side-channel-attacks that utilise differential power analysis. * Consistent with: * Windows SymCrypt https://github.com/microsoft/SymCrypt/blob/6d019cefafb3fefe3c53b0de3bba6f8c86e2d48a/lib/equal.c * GoLang std FIPS140 https://cs.opensource.google/go/go/+/master:src/crypto/internal/fips140/subtle/constant_time.go;l=72?q=ConstantTimeEq&ss=go%2Fgo * JDK https://github.com/AdoptOpenJDK/openjdk-jdk11/blob/master/src/java.base/share/classes/java/security/MessageDigest.java#L473C1-L486C6 * Rust https://github.com/cesarb/constant_time_eq/blob/main/src/generic.rs#L158 * OpenBSD https://sources.debian.org/src/openssh/1%3A10.0p1-6/openbsd-compat/timingsafe_bcmp.c/#L30
ChrisMcKee
force-pushed
the
fixed-time-equality
branch
from
34e142b to
4fea913
Compare
|
"Why?" When we originally added this to .NET Framework, we observed that the XOR based approach actually had a timing bias in it (either A XOR A was faster than A XOR (anything not A), or 0 XOR 0 was faster than other things, I don't remember), and we didn't observe the bias with SUB. We don't want a change here "just because". |
bartonjs
added
the
NO-MERGE
|
Didn't think I worded it as 'just because'; xor in time-constant comparison, as per the links, is the standard approach. There was nothing to say why this deviates or why, for instance, this deviates in yet another way. Thanks for your time anyway. |
|
"Because other people do it a different way" is sort of "just because". Maybe a different question would be "what motivated you to make the change?" and/or "what motivated you to look at this code?" |
|
I was working on similar functionality in another library and remembered the extensions had been added to the runtime, which led me to this source, the identity source, and wondering why they all differed from what I considered the standard implementation (ignoring the .net specifics like read-only spans). |
Changes
FixedTimeEqualsto useXORinstead ofSUB.XOR directly expresses the statement "are these bytes different"; the use of subtraction changes that to "what is the difference between these bytes".
The agreed implementation for constant-time security is to avoid arithmetic that may introduce timing variations between platform/processor/compiler. XOR operations
a ^ bproduces a non-zero result, only when bits differ, and benefits from being its own inverse without requiring extra-information like carry bits or sign bits. XOR is cheaper at a CPU level (realitically not a huge factor unless your on certain low speed asics) though I mention this more for fun than "this would have a huge impact on perf"; Theoretically that difference can matter in side-channel-attacks that utilise differential power analysis; but again not the biggest driver for change vs consistency and agreed approach.