Fix cipher property NIDs for SSL_get_current_cipher and add PSK kx mapping#10639
Draft
julek-wolfssl wants to merge 2 commits into
Draft
Fix cipher property NIDs for SSL_get_current_cipher and add PSK kx mapping#10639julek-wolfssl wants to merge 2 commits into
julek-wolfssl wants to merge 2 commits into
Conversation
…pping The cipher property helpers (SSL_CIPHER_get_kx_nid / get_auth_nid / get_cipher_nid / get_digest_nid / is_aead) parse the cipher name looked up via cipher->offset in GetCipherSegment(). That offset is only populated when the cipher is obtained through wolfSSL_get_ciphers_compat() (SSL_get_ciphers()). When the cipher comes from SSL_get_current_cipher(), offset is left at 0, so these helpers parsed cipher_names[0] (a TLS 1.3 suite) instead of the negotiated cipher - e.g. returning NID_kx_any for a plain PSK suite while SSL_CIPHER_get_name() (which uses the suite bytes) reported the correct name. Resolve the cipher_names entry from the always-populated suite bytes in GetCipherSegment(), falling back to cipher->offset when no match is found. Also add the missing plain "PSK" -> NID_kx_psk entry to the kx lookup table so PSK suites report NID_kx_psk instead of NID_undef. Add a regression test that drives the SSL_get_current_cipher() path for TLS_PSK_WITH_AES_128_GCM_SHA256 and checks all five property helpers.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes OpenSSL-compat cipher property helpers so they report properties for the negotiated cipher when the cipher object comes from SSL_get_current_cipher() (where cipher->offset is not populated). It also corrects PSK key-exchange NID mapping and adds a regression test to cover the SSL_get_current_cipher() path.
Changes:
- Update
GetCipherSegment()to resolve thecipher_namesentry using the cipher suite bytes (cipherSuite0/cipherSuite) instead of relying solely oncipher->offset. - Add missing
"PSK" -> NID_kx_pskmapping inwolfSSL_CIPHER_get_kx_nid(). - Add an API regression test for
TLS_PSK_WITH_AES_128_GCM_SHA256validating all five cipher property helpers viaSSL_get_current_cipher().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/api.c | Adds a regression test covering cipher property helpers when using SSL_get_current_cipher() for a plain PSK suite. |
| src/ssl.c | Adds missing "PSK" entry to the kx-to-NID mapping table. |
| src/internal.c | Makes GetCipherSegment() resolve cipher name table entries via suite bytes to avoid incorrect parsing when offset is unset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
28750
to
28767
| offset = cipher->offset; | ||
|
|
||
| /* offset is not set via wolfSSL_get_current_cipher(), so resolve it from | ||
| * the always-populated suite bytes. */ | ||
| for (i = 0; i < GetCipherNamesSize(); i++) { | ||
| if (cipher_names[i].cipherSuite0 == cipher->cipherSuite0 && | ||
| cipher_names[i].cipherSuite == cipher->cipherSuite | ||
| #ifndef NO_CIPHER_SUITE_ALIASES | ||
| && (!(cipher_names[i].flags & WOLFSSL_CIPHER_SUITE_FLAG_NAMEALIAS)) | ||
| #endif | ||
| ) { | ||
| offset = (unsigned long)i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (offset >= (unsigned long)GetCipherNamesSize()) | ||
| return NULL; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The cipher property helpers (
SSL_CIPHER_get_kx_nid/get_auth_nid/get_cipher_nid/get_digest_nid/is_aead) parse the cipher name thatGetCipherSegment()looks up viacipher->offset. Thatoffsetis onlypopulated when the cipher is obtained through
wolfSSL_get_ciphers_compat()(
SSL_get_ciphers()). When the cipher comes fromSSL_get_current_cipher(),offsetis left at0, so these helpers parsedcipher_names[0](a TLS 1.3suite) instead of the negotiated cipher — e.g. returning
NID_kx_anyfor aplain PSK suite while
SSL_CIPHER_get_name()(which uses the suite bytes)reported the correct name.
Changes
GetCipherSegment(): resolve thecipher_namesentry from thealways-populated suite bytes (
cipherSuite0/cipherSuite), falling back tocipher->offsetwhen no match is found.wolfSSL_CIPHER_get_kx_nid(): add the missing plain"PSK"->NID_kx_pskentry to the kx lookup table so PSK suites report
NID_kx_pskinstead ofNID_undef.SSL_get_current_cipher()path forTLS_PSK_WITH_AES_128_GCM_SHA256and checks all five property helpers.Testing
Added an API regression test exercising the
SSL_get_current_cipher()path.