Skip to content

ext/gmp: Add GMP ECC test #18363

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

Merged
merged 5 commits into from
May 5, 2025
Merged

ext/gmp: Add GMP ECC test #18363

merged 5 commits into from
May 5, 2025

Conversation

famoser
Copy link
Contributor

@famoser famoser commented Apr 20, 2025

Add a test that executes GMP operations in the context of elliptic curve.

This work is motivated by a bug from a couple of months ago (see #16870), which showed this use-case is uncovered by tests in PHP. The bug was fixed quickly, and in #16896 a unit test already landed for finite field cryptography. The test for elliptic curve cryptography is more complicated (as seen in this PR), so this was decided to be done later: #16896 (comment)

This unit test is written based on code developed for https://github.com/famoser/elliptic, which has been tested extensively. However, the code is not ready to use for security-related applications, as it has not been hardened nor tested against possible side channels. Side channels could leak the secret values, and therefore compromise the security of the algorithms.

@Girgias
Copy link
Member

Girgias commented Apr 22, 2025

Can you maybe rebase and force push on the latest master to see if it fixes the FreeBSD pipeline? As that is unrelated to this PR

@famoser famoser changed the base branch from PHP-8.1 to master April 23, 2025 06:32
@famoser famoser requested a review from Girgias as a code owner April 23, 2025 06:32
@famoser famoser changed the title test: Add GMP ECC test ext/gmp: Add GMP ECC test Apr 26, 2025
@famoser
Copy link
Contributor Author

famoser commented Apr 26, 2025

@Girgias The tests pass now (apparently just flaky). I've also added some documentation for the point decompression math, and a new output check to ensure the test does not pass for trivial calculation (e.g. all 0s). Anything should now either be documented or reference the spec now. If this is not the case, please mention it in your review.

While asking around whether I've covered all fundamental operations of ECC, I was made aware of that Parings (as in Weil-Pairing) could be seen as another fundamental operation. For me, this would be another possible PR in the future; but out of scope for now (as I first have to get used to the exact math there).

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

MSMT, added two small comments

@famoser
Copy link
Contributor Author

famoser commented May 1, 2025

From my point of view, all TODOs around testing crypto using GMP are now done.

One doubt I still have: The gmp tests have unfortunately quite a cost (1s for the full ECC, 0.4s for each of the two kitchensink tests). I do not think I can meaningfully improve this performance; given that operations on large numbers are simply costly. While I do not think this is the case; is there some kind of "secondary" testset for PHP that is only run in critical circumstances (e.g. before a release is built), and we could move the tests there?

@Girgias
Copy link
Member

Girgias commented May 5, 2025

From my point of view, all TODOs around testing crypto using GMP are now done.

One doubt I still have: The gmp tests have unfortunately quite a cost (1s for the full ECC, 0.4s for each of the two kitchensink tests). I do not think I can meaningfully improve this performance; given that operations on large numbers are simply costly. While I do not think this is the case; is there some kind of "secondary" testset for PHP that is only run in critical circumstances (e.g. before a release is built), and we could move the tests there?

No, but frankly the better way to gain a speed-up on our CI pipeline would be to finish parallelizing all the ext/mysqli tests (probably still 300+ tests that must run sequentially)

@Girgias Girgias merged commit 90da482 into php:master May 5, 2025
9 checks passed
@famoser famoser deleted the test-gmp-ecc branch May 5, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants