Validate DH peer public key subgroup before key agreement - #460
aidangarske wants to merge 5 commits into
Conversation
aidangarske
commented
Aug 6, 2026
aidangarske
commented
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #460
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #460
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| } | ||
| if (err == 0) { | ||
| rc = EVP_PKEY_derive_set_peer(ctx, small); | ||
| if (rc == 1) { |
There was a problem hiding this comment.
Why do we only test if set_peer succeeds?
There was a problem hiding this comment.
I think it wasn't guaranteed to reach derive the validating set_peer runs the keymgmt public check first so a build where that rejects the key would pass without touching the derive-time check. It now uses EVP_PKEY_derive_set_peer_ex with validation off, requires that to succeed, and then requires derive to fail, same as the q-less cases below it.
| }; | ||
|
|
||
| #endif /* WP_HAVE_DH */ | ||
|
|
There was a problem hiding this comment.
Nit: the empty trailing line is dropped. Can we keep it?
aidangarske
added
ci:all
77c6338 to
10e75ea
Compare
|
@aidangarske , can you fix the conflicts? |
