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

tkn20,kyber,x25519,x448: plug constant-time leaks #411

Merged
merged 2 commits into from Mar 14, 2023
Merged

tkn20,kyber,x25519,x448: plug constant-time leaks #411

merged 2 commits into from Mar 14, 2023

Conversation

tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Mar 6, 2023

In particular leaking z in kyber could be quite damaging: https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/SJ31w0QSmIM/m/XgdBgh3wAwAJ

The changes to x25519 and x448 are unlikely to be needed, but it's more idiomatic at least.

In particular leaking z in kyber could be quite damaging:
https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/SJ31w0QSmIM/m/XgdBgh3wAwAJ

The changes to x25519 and x448 are unlikely to be needed, but it's more
idiomatic at least.
@bwesterb bwesterb self-requested a review March 6, 2023 11:10
Copy link
Member

@bwesterb bwesterb left a comment

Choose a reason for hiding this comment

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

Thanks.

One of your changes doesn't do anything.

About the Kyber one: when can you get someone to compare two private keys where you know the z of one of them? So I'm not convinced this is "damaging". Anyway, it's good cryptographic hygiene to always compare secrets in constant-time, so it's a good one to take.

The curve ones are comparing public keys... but it already uses a ct-compare. Well, I'll leave that to @armfazh .

I'll leave the tkn to @tanyav2 — I don't know whether the pdfKey is sensitive from the top of my head.

pke/kyber/kyber768/internal/cpapke.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tanyav2 tanyav2 left a comment

Choose a reason for hiding this comment

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

re: tkn, the matrix comparisons in that same line are also not constant time. I'll create an issue to assess the need and make all necessary changes together.

@tanyav2
Copy link
Contributor

tanyav2 commented Mar 6, 2023

re: tkn, the matrix comparisons in that same line are also not constant time. I'll create an issue to assess the need and make all necessary changes together.

This is also bottlenecked by #286 (@armfazh) somewhat. Overall, I don't think it's worthwhile to fix one isolated instance without fixing them underlying bottleneck.

Copy link
Contributor

@armfazh armfazh left a comment

Choose a reason for hiding this comment

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

it looks good, and doesn't add a significant overhead.

pke/kyber/kyber768/internal/cpapke.go Outdated Show resolved Hide resolved
@armfazh armfazh merged commit 9d4f8c8 into cloudflare:main Mar 14, 2023
10 checks passed
@armfazh
Copy link
Contributor

armfazh commented Mar 14, 2023

Thanks @tmthrgd

@tmthrgd tmthrgd deleted the const_time_leak branch March 15, 2023 00:38
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

4 participants