Skip to content

Add ByteString.equals(other, constantTime) overload#1812

Open
SAY-5 wants to merge 6 commits into
square:masterfrom
SAY-5:bytestring-constant-time-equals
Open

Add ByteString.equals(other, constantTime) overload#1812
SAY-5 wants to merge 6 commits into
square:masterfrom
SAY-5:bytestring-constant-time-equals

Conversation

@SAY-5

@SAY-5 SAY-5 commented May 24, 2026

Copy link
Copy Markdown

Adds a constant-time ByteString.equals(other, constantTime) overload for timing-safe comparison of secrets like hashes and MACs, as requested in #1797. After a length check it XORs every byte and never short-circuits, so the running time does not depend on where the byte strings differ. Implemented in commonMain so it works on all targets, and tested across the basic and segmented byte string variants.

Closes #1797. Closes #1799

@JakeWharton

Copy link
Copy Markdown
Collaborator

Your test doesn't make any attempt to validate the constant-time-ness of the implementation.

@SAY-5

SAY-5 commented May 24, 2026

Copy link
Copy Markdown
Author

Fair point. A wall-clock timing assertion would be flaky in CI, so the test only covers functional correctness; the constant-time property comes from the implementation never short-circuiting. Happy to drop the constant-time framing and document it purely as the structural guarantee, or take whatever direction you prefer here.

@JakeWharton

Copy link
Copy Markdown
Collaborator

We should be able to use simple statistics to determine that given two very large byte strings:

  • When equal, constant time takes the same amount as normal equal
  • When not equal, constant time takes the same amount as when equal
  • When not equal, normal equal takes statistically significantly less than when equal

Signed-off-by: say <say.apm35@gmail.com>
@SAY-5

SAY-5 commented May 25, 2026

Copy link
Copy Markdown
Author

Added a JVM statistical timing test in ConstantTimeEqualsTimingTest. It measures median nanos over 500 iterations (after 200 warm-up reps) for CT(match), CT(mismatch), normal(match), and normal(mismatch) on 1 MB strings differing at byte 0. Asserts CT(match)/CT(mismatch) are within 3x of each other and normal(mismatch) is <2% of CT(mismatch) (short-circuit at byte 0 vs full scan). Only Java 17+ available in CI so couldn't run locally; let me know if you'd like any adjustments to thresholds or structure.

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@SAY-5

SAY-5 commented May 26, 2026

Copy link
Copy Markdown
Author

Fixed: switched to org.junit.{Test,Assert.assertTrue} (matching the rest of jvmTest) and reformatted the assertTrue calls to multi-line style so spotlessKotlinCheck passes on ktlint 0.48.2.

…gTest

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@SAY-5

SAY-5 commented May 26, 2026

Copy link
Copy Markdown
Author

Fixed: added missing import okio.ByteString.Companion.toByteString in ConstantTimeEqualsTimingTest.kt (4bfb638).

@JakeWharton JakeWharton reopened this May 26, 2026
@JakeWharton

Copy link
Copy Markdown
Collaborator

Trying to kick CI after this morning's incident.

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@SAY-5

SAY-5 commented May 26, 2026

Copy link
Copy Markdown
Author

The CT-vs-normal(match) ratio was flaky on loom/Windows CI (>5x overhead from XOR accumulation on slower runners). Dropped that assertion -- the two that matter are CT(match)~CT(mismatch) (neither short-circuits) and normal(mismatch)<<CT(mismatch) (normal does). Push a59f221.

@JakeWharton JakeWharton changed the title feat: add ByteString.equals(other, constantTime) overload Add ByteString.equals(other, constantTime) overload May 28, 2026

actual override fun equals(other: Any?) = commonEquals(other)

actual fun equals(other: ByteString, constantTime: Boolean) = commonEqualsConstantTime(other)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not handle constantTime=false correctly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a4e8c40. The overload now branches on the flag and falls back to the regular equals for constantTime=false, with the false path covered in ByteStringTest.

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>

@JakeWharton JakeWharton left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one question.

Comment on lines +240 to +242
var result = 0
for (i in 0 until size) {
result = result or (this[i].toInt() xor other[i].toInt())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you chose to use 0 as the true-y value and or the comparisons as opposed to using 1 and and? The result is the same, but this version is unintuitive to me because it's not how I think about the comparisons being done.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the standard constant-time idiom: XOR each byte pair (0 when equal) and OR the results, so result stays 0 only if every pair matched and becomes non-zero the moment any byte differs, with a single comparison at the very end. Going with 1 and AND would need a per-byte equality test (this[i] == other[i]), which reintroduces a branch/comparison inside the loop that the OR-of-XOR form avoids. It mirrors crypto/subtle.ConstantTimeCompare and CRYPTO_memcmp. Happy to add a short comment to that effect if it helps readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ByteString.equals(other: ByteString, constantTime: Boolean) overload

2 participants