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

patched IBE hashToCurve with ietf hashToField impl #12

Closed
wants to merge 2 commits into from

Conversation

CluEleSsUK
Copy link
Contributor

due to using a counter size of 32bits in the stream cipher, only up to
256TB may be encrypted before a nonce reuse would occur.
This solution checks if the counter has reached the limit on every
increment.
This _may_ add overhead or _may_ be free on some engines and I haven't
benchmarked it
- pulled some pieces out of noble
- wrote an implementation of hashToField using https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-16#section-5.2
- created a patched hashToCurve
@CluEleSsUK
Copy link
Contributor Author

CluEleSsUK commented Dec 21, 2022

Took a first crack at this - only 3 tests failing including the go compatibility ones.
I haven't updated the impl for decrypt yet - I'm not sure if I should be updating toField or something else to make it compatible

@nikkolasg
Copy link

nikkolasg commented Dec 21, 2022

What's the reason of having our own implementation of that ?
Noble seems to support the RFC too. is the difference in version the reason ? (12 for Noble vs 16 for this PR). And if so, why keeping at v12 is not an option ?

@CluEleSsUK
Copy link
Contributor Author

CluEleSsUK commented Dec 21, 2022

In retrospect, I should have read the feedback from the audit more clearly - it looks like only the rejection sampling in h3 needs changed, not the whole hashToField x) I took the recommendation at the end to implement the ietf one a bit too literally oops

@AnomalRoil
Copy link
Member

Yeah, in theory Noble should implement what we need already.

@nikkolasg
Copy link

Can someone link then the reason and/or issue raised in the audit (or paste a summary etc), in this PR or issue here ? It'd be nice that this PR contains everything one needs to know to understand the reasons, impacts etc.

@AnomalRoil
Copy link
Member

Our H3 function is vulnerable to collision. But I'd argue it's not a problem since we do key-wrapping and keys have fixed sizes, but TBH, using HashToField and HashToField from the RFC is probably better

Copy link
Member

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

Could we switch to the Noble implementations? If you want to wrap them in your own function to change their API, that's fine.
Also let's keep the tests.

@@ -85,6 +89,10 @@ export class STREAM {
// Increments Big Endian Uint8Array-based counter.
// [0, 0, 0] => [0, 0, 1] ... => [0, 0, 255] => [0, 1, 0]
incrementCounter() {
if (this.counter == COUNTER_MAX) {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't a greater or equal comparison be safer?


// this patched `hashToCurve` uses a different implementation of `hashToField` than noble
// see: https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-16
export async function hashToCurve(input: Uint8Array | string): Promise<PointG2> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Noble's methods instead?
paulmillr/noble-bls12-381@8a18615

But let's keep the tests if these are not the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we were actually using them already internally - I think we can just update the package and then change our h3 to comply

expand: true,
}

export async function hashToField(message: Uint8Array, count: number, options: HashToFieldOptions = defaultsFromNoble): Promise<bigint[][]> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the methods from Noble?
paulmillr/noble-bls12-381@8a18615

@CluEleSsUK
Copy link
Contributor Author

Closing this because I got the wrong end of the stick (was a good learning exercise though ;p)

@CluEleSsUK CluEleSsUK closed this Jan 4, 2023
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.

None yet

3 participants