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

LibCrypto+LibWeb: Support P-521 with OpenSSL #3374

Merged
merged 7 commits into from
Jan 27, 2025

Conversation

devgianlu
Copy link
Contributor

This PR introduces P-521 support by rewriting the SECPxxxr1 implementation using OpenSSL, this should be the last remaining piece of implementation for WebCryptoAPI. After this I expect us to pass all WPT tests except ~100.

The code was printing one error message only, but multiple can be
generated in one call. Additionally, using this builtin produces
a much more descriptive output.
@devgianlu devgianlu requested a review from alimpfard as a code owner January 27, 2025 09:29
@devgianlu devgianlu force-pushed the p521 branch 2 times, most recently from 4be180b to 2581924 Compare January 27, 2025 09:47
@alimpfard
Copy link
Contributor

I took a brief look, the only issue I noticed was addressed in the last commit so 👍
Will do a more thorough review in a bit.

@alimpfard
Copy link
Contributor

Ah yes,

102/161 Test #102: TestCurves ...........................***Failed    0.09 sec
Running test 'test_x25519'.
Completed test 'test_x25519' in 3ms
Running test 'test_x448'.
Completed test 'test_x448' in 14ms
Running test 'test_secp256r1'.
Completed test 'test_secp256r1' in 0ms
Running test 'test_secp384r1'.
Completed test 'test_secp384r1' in 7ms
Finished 4 tests and 0 benchmarks in 26ms (24ms tests, 0ms benchmarks, 2ms other).
All 4 tests passed.

@devgianlu
Copy link
Contributor Author

devgianlu commented Jan 27, 2025

Ah yes,

102/161 Test #102: TestCurves ...........................***Failed    0.09 sec
Running test 'test_x25519'.
Completed test 'test_x25519' in 3ms
Running test 'test_x448'.
Completed test 'test_x448' in 14ms
Running test 'test_secp256r1'.
Completed test 'test_secp256r1' in 0ms
Running test 'test_secp384r1'.
Completed test 'test_secp384r1' in 7ms
Finished 4 tests and 0 benchmarks in 26ms (24ms tests, 0ms benchmarks, 2ms other).
All 4 tests passed.

I love it when it's so descriptive lol. Anyway, I think it's a memory leak problem again, I am building with sanitizers locally.

Copy link
Contributor

@alimpfard alimpfard left a comment

Choose a reason for hiding this comment

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

Code looks largely OK, there's some signed len functions that could maybe have better names for the return values though:

Libraries/LibCrypto/Curves/SECPxxxr1.h Outdated Show resolved Hide resolved
@devgianlu
Copy link
Contributor Author

devgianlu commented Jan 27, 2025

I love it when it's so descriptive lol. Anyway, I think it's a memory leak problem again, I am building with sanitizers locally.

Indeed, I was not freeing instances of EC_group.

@alimpfard alimpfard enabled auto-merge (rebase) January 27, 2025 10:52
@alimpfard alimpfard merged commit 1d207aa into LadybirdBrowser:master Jan 27, 2025
7 checks passed
@devgianlu devgianlu deleted the p521 branch January 27, 2025 11:25
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.

2 participants