Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tentative fix for #345 - constant-time scalar mul with endomorphism acceleration wrong result #346

Merged
merged 3 commits into from
Jan 20, 2024

Conversation

mratsim
Copy link
Owner

@mratsim mratsim commented Jan 18, 2024

This tentatively fixes #345 with the simple approach explained in #345 (comment) to add an extra bit to hold the decomposed miniscalars.

This will be kept as a fix if we can also relax the requirement for exact bit match for scalarMul_vartime when dispatching to endomorphism.

const L = scalBits.ceilDiv_vartime(M) + 1
let usedBits = scalar.limbs.getBits_LE_vartime()
when scalBits == EC.F.C.getCurveOrderBitwidth() and
EC.F.C.hasEndomorphismAcceleration():
if usedBits >= L:
when EC.F is Fp:
P.scalarMulEndo_minHammingWeight_windowed_vartime(scalar, window = 4)
elif EC.F is Fp2:
P.scalarMulEndo_minHammingWeight_windowed_vartime(scalar, window = 3)
else: # Curves defined on Fp^m with m > 2
{.error: "Unreachable".}
return
if 64 < usedBits:
# With a window of 5, we precompute 2^3 = 8 points
P.scalarMul_minHammingWeight_windowed_vartime(scalar, window = 5)
elif 16 < usedBits:
# With a window of 3, we precompute 2^1 = 2 points
P.scalarMul_minHammingWeight_windowed_vartime(scalar, window = 3)
elif 4 < usedBits:
P.scalarMul_doubleAdd_vartime(scalar)
else:
P.scalarMul_addchain_4bit_vartime(scalar)

In that case, we can remove the current way to handle negative mini-scalars by negating the curve point.

when babai(F)[i][1]:
# prod_high_words works like logical right shift
# When negative, we should add 1 to properly round toward -infinity
alphas[i] += One
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is the main change. Papers that introduce the Babai's rounding unfortunately use shifts but do not go over the negative special case.

@mratsim
Copy link
Owner Author

mratsim commented Jan 20, 2024

Change of fix approach,

Adding an extra bit also on scalarMul_vartime fails, which suggest the issue is not in the bit length.

When developing endomorphism years ago, the papers didn't special-case Babai's rounding for negative scalars.

Now what's curious is that it has not trigger any error so far in Google Ossfuzz.

@mratsim
Copy link
Owner Author

mratsim commented Jan 20, 2024

The CI failure is due to some Nim upstream bug when declaring {.noInit.} variables in templates
image

Same issue as #332 (comment)

Besides removing the {.noInit.}, it may be that using inject solves the issue as well.

@mratsim mratsim merged commit bc5faaa into master Jan 20, 2024
32 checks passed
@mratsim mratsim deleted the bug-345 branch January 20, 2024 18:39
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.

wrong result with scalarMul in G2 curve
1 participant