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

Constant-time EC point multiplication (Montgomery ladder) implementation #325

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
130b977
feat(lean-imt): added `updateMany` method to package
ChinoCribioli Aug 28, 2024
6049ea4
feat(lean-imt): implemented some tests on lean-imt
ChinoCribioli Aug 28, 2024
e00a891
feat(lean-imt): added more precondition checks
ChinoCribioli Aug 29, 2024
3e9e54b
feat(lean-imt): finished testing on `updateMany` method
ChinoCribioli Aug 29, 2024
cd3156e
feat(lean-imt): added test to the case when passing repeated indices
ChinoCribioli Aug 31, 2024
06e941c
feat(lean-imt): added complexity documentation for `updateMany` method
ChinoCribioli Aug 31, 2024
4b51e44
feat(lean-imt): added test of several updates
ChinoCribioli Sep 1, 2024
29638d5
feat(lean-imt): added repeated indices check
ChinoCribioli Sep 1, 2024
880812c
feat(lean-imt): changed error message to be more accurate
ChinoCribioli Sep 1, 2024
681239e
feat(lean-imt): added complexity in terms only of n
ChinoCribioli Sep 1, 2024
1bfffd4
feat(lean-imt): changed documentation to add discussion in another issue
ChinoCribioli Sep 2, 2024
bd67b35
feat(lean-imt): fixed typo on documentation
ChinoCribioli Sep 2, 2024
c5e836d
Update packages/lean-imt/src/lean-imt.ts
ChinoCribioli Sep 9, 2024
6270ad5
Merge branch 'privacy-scaling-explorations:main' into main
ChinoCribioli Sep 9, 2024
fe22dac
perf(baby-jubjub): implemented montgomery ladder
ChinoCribioli Sep 10, 2024
6665e36
test(baby-jubjub): added more tests to curve basic operations
ChinoCribioli Sep 11, 2024
fc0f3b8
docs(baby-jubjub): documented montogmery ladder
ChinoCribioli Sep 11, 2024
8127d10
docs(baby-jubjub): fixed documentation
ChinoCribioli Sep 11, 2024
3c122c5
docs(baby-jubjub): added resource of Montgomery Ladder
ChinoCribioli Sep 12, 2024
56fcd9f
fix(baby-jubjub): replaced undetermined while with hardcoded for loop
ChinoCribioli Sep 12, 2024
b7dec5b
refactor(baby-jubjub): created and exported identity point
ChinoCribioli Sep 12, 2024
f0ad8e1
Update packages/baby-jubjub/src/baby-jubjub.ts
ChinoCribioli Sep 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions packages/baby-jubjub/src/baby-jubjub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,25 +65,43 @@ export function addPoint(p1: Point<bigint>, p2: Point<bigint>): Point<bigint> {
/**
* Performs a scalar multiplication by starting from the 'base' point and 'adding'
* it to itself 'e' times.
* This works given the following invariant: At each step, R0 will be r_0*base where r_0 is the prefix of e
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest additionally referring to the named algorithm being used here (Montgomery Ladder) perhaps with a link to a description of the algorithm (Wikipedia or some other source).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Will do.

* written in binary and R1 will be (r_0+1)*base. In other words: at iteration i of the loop, r_0's binary
* representation will be the first i+1 most significant bits of e. If the upcoming bit is a 0, we just have to
* double R0 and add R0 to R1 to maintain the invariant. If it is a 1, we have to double R0 and add 1*base
* (or add R1, which is the same as (r_0+1)*base), and double R1 to maintain the invariant.
* @param base The base point used as a starting point.
* @param e A secret number representing the private key.
* @returns The resulting point representing the public key.
*/
export function mulPointEscalar(base: Point<bigint>, e: bigint): Point<bigint> {
let res: Point<bigint> = [Fr.e(BigInt(0)), Fr.e(BigInt(1))]
let rem: bigint = e
let exp: Point<bigint> = base

while (!scalar.isZero(rem)) {
if (scalar.isOdd(rem)) {
res = addPoint(res, exp)
if (scalar.isZero(e)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't constant time if e is zero. Is that okay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think although the exponent is marked as secret, it may be considered public if it's set to 0. The rationale in my mind is "no private key would ever be selected as the 0 value". That said, I don't think it's a good idea to short circuit here because it breaks the comment above of e being secret in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also thought initially that we shouldn't worry too much about the zero case since it is irrelevant cryptographically. But I agree to change this to make the method more consistent.

return [BigInt(0), BigInt(1)]
}
const eBits: Array<boolean> = []
while (!scalar.isZero(e)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the goal constant time? This loop doesn't seem constant time to me, since it'll exit early if high bits of the input are zero. As a result the length of eBits is also variable. If you want constant time, it seems like there has to be a hard-coded max bit length (254 I think, but someone should double-check me) somewhere.

If you wish, I think this loop can probably be skipped entirely with some bigint bitwise operations below. Something like this:

  for (let bitMask = 1n, bitmask < 1n<<254n, bitmask <<= 1n) {
    if (e & bitMask != 0) {
      // ...

I'm not sure of the relative efficiency, but I'd guess that the bitmasks are cheaper than allocating memory for an array.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this sounds correct to me. If e is 7 for example this will loop 3 times, whereas if it's like 4182498124124891489144 it will loop significantly more times.

Hardcoding an iteration count is a good approach, but we'll still have timing vulnerabilities due to the use of the javascript BigInt type which is not safe against timing attacks.

It's also unclear if the scalar.isZero function is constant time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with this point. I changed the loop with a hardcoded for.

if (scalar.isOdd(e)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unclear if isOdd is constant time

eBits.push(true)
} else {
eBits.push(false)
}
e = scalar.shiftRight(e, BigInt(1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

A right shift on a BigInt is almost certainly not constant time.

}

exp = addPoint(exp, exp)
rem = scalar.shiftRight(rem, BigInt(1))
let R0: Point<bigint> = base
let R1: Point<bigint> = addPoint(base, base)

for (const bit of eBits.slice(0, -1).reverse()) {
Copy link
Collaborator

@chancehudson chancehudson Sep 12, 2024

Choose a reason for hiding this comment

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

This slice is a no-op, i think. It may be necessary to invoke the reverse function though.

nvm, it returns all elements but the last one.

if (bit) {
R0 = addPoint(R0, R1)
R1 = addPoint(R1, R1)
} else {
R1 = addPoint(R0, R1)
R0 = addPoint(R0, R0)
}
}

return res
return R0
}

/**
Expand Down
61 changes: 60 additions & 1 deletion packages/baby-jubjub/tests/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,72 @@
import { babyjub } from "circomlibjs"
import { utils } from "ffjavascript"
import { Base8, Point, addPoint, inCurve, mulPointEscalar, packPoint, r, unpackPoint } from "../src"
import * as scalar from "@zk-kit/utils/scalar"
import { Base8, Point, addPoint, inCurve, mulPointEscalar, packPoint, r, unpackPoint, order, subOrder } from "../src"
import { tonelliShanks } from "../src/sqrt"

describe("BabyJubjub", () => {
const secretScalar = BigInt(324)
const id: Point<bigint> = [BigInt(0), BigInt(1)]

let publicKey: Point<bigint>

it("Test point addition and inCurve", async () => {
expect(inCurve(id)).toBeTruthy()
expect(inCurve([BigInt(1), BigInt(0)])).toBeFalsy()
expect(addPoint(id, id).toString()).toBe(id.toString())

const p1: Point<bigint> = [
BigInt("17777552123799933955779906779655732241715742912184938656739573121738514868268"),
BigInt("2626589144620713026669568689430873010625803728049924121243784502389097019475")
]
const p2: Point<bigint> = [
BigInt("16540640123574156134436876038791482806971768689494387082833631921987005038935"),
BigInt("20819045374670962167435360035096875258406992893633759881276124905556507972311")
]
const p3: Point<bigint> = [
BigInt("7916061937171219682591368294088513039687205273691143098332585753343424131937"),
BigInt("14035240266687799601661095864649209771790948434046947201833777492504781204499")
]

expect(inCurve(p1)).toBeTruthy()
expect(inCurve(p2)).toBeTruthy()
expect(inCurve(p3)).toBeTruthy()

expect(addPoint(p1, p2).toString()).toBe(p3.toString())
})

it("Test point multiplication with small values", async () => {
const P: Point<bigint> = [
BigInt("17777552123799933955779906779655732241715742912184938656739573121738514868268"),
BigInt("2626589144620713026669568689430873010625803728049924121243784502389097019475")
]

expect(mulPointEscalar(P, BigInt(0)).toString()).toBe(id.toString())
expect(mulPointEscalar(P, BigInt(1)).toString()).toBe(P.toString())
expect(mulPointEscalar(P, BigInt(2)).toString()).toBe(addPoint(P, P).toString())
expect(mulPointEscalar(P, BigInt(3)).toString()).toBe(addPoint(addPoint(P, P), P).toString())

expect(mulPointEscalar(id, BigInt(1)).toString()).toBe(id.toString())
expect(mulPointEscalar(id, BigInt(14134324)).toString()).toBe(id.toString())
})

it("Test base point order", async () => {
expect(scalar.shiftRight(order, BigInt(3))).toBe(subOrder)
const G: Point<bigint> = [
BigInt("995203441582195749578291179787384436505546430278305826713579947235728471134"),
BigInt("5472060717959818805561601436314318772137091100104008585924551046643952123905")
]
const p1: Point<bigint> = mulPointEscalar(G, BigInt(8) * subOrder)
expect(p1.toString()).toBe(id.toString())
const p2 = mulPointEscalar(Base8, subOrder)
expect(p2.toString()).toBe(id.toString())

const random = BigInt("38275423985628165")
expect(mulPointEscalar(Base8, random).toString()).toBe(
mulPointEscalar(Base8, random + BigInt(543523) * subOrder).toString()
)
})

it("Should add 1 point to the curve", async () => {
const p1: Point<bigint> = [BigInt(0), BigInt(1)]

Expand Down
Loading