OpenSSL compatibility for libodbc#10813
Conversation
|
ad3c910 to
b41bbac
Compare
|
Retest please. |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 7 total — 7 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
2 finding(s) not tied to a diff line (full detail below)
Posted findings
- [High] OBJ_find_sigid_algs stub hardcodes SHA-256/RSA and ignores sigid —
wolfssl/openssl/objects.h:81-88 - [Medium] No test coverage for WOLFSSL_VERIFY_NONE_DEFAULT security-relevant default change —
src/internal.c:2645-2649 - [Low] WOLFSSL_ERR_R_BUF_LIB defined as 0 (conflicts with 'no error' and OpenSSL's value) —
wolfssl/openssl/err.h:46 - [Low] Garbled comment on OBJ_find_sigid_algs literal NIDs —
wolfssl/openssl/objects.h:77-79 - [Low] TLS_ST_OK hardcoded to 16, coupled to internal enum position —
wolfssl/openssl/ssl.h:1572
Findings not tied to a diff line
SSL_R_UNKNOWN_PROTOCOL / WRONG_VERSION_NUMBER / UNSUPPORTED_PROTOCOL numeric value changed
File: wolfssl/openssl/ssl.h:1748-1753, wolfssl/openssl/err.h:47-49
Function: N/A (macros)
Severity: Medium
These three pre-existing public compat macros were changed from expanding to VERSION_ERROR (a negative wolfSSL internal error enum) to OpenSSL numeric reason codes (WOLFSSL_SSL_R_UNKNOWN_PROTOCOL=252, WOLFSSL_SSL_R_WRONG_VERSION_NUMBER=267, WOLFSSL_SSL_R_UNSUPPORTED_PROTOCOL=258). This is more OpenSSL-accurate, but it is a behavior change: any existing application that compared these macros against wolfSSL's internally reported error code (VERSION_ERROR, e.g. from wolfSSL_get_error/SSL_get_error) will no longer match, because wolfSSL still reports VERSION_ERROR internally, not 252/267/258. I confirmed there are no internal src/*.c or tests/*.c users of these specific macros (tests compare against VERSION_ERROR directly), so the build and current tests are unaffected, but downstream consumers relying on the old mapping would silently break.
Recommendation: Confirm this value change is intended and note it as a compatibility change in the PR/changelog. If wolfSSL's error surface (ERR_get_error/ERR_GET_REASON) does not translate internal VERSION_ERROR into these OpenSSL reason codes, apps checking reason == SSL_R_WRONG_VERSION_NUMBER will never match - verify the intended consumer (libodbc/libtds) actually reads these as raw constants and not via wolfSSL's error queue.
Referenced code: wolfssl/openssl/ssl.h:1748-1753, wolfssl/openssl/err.h:47-50 (4 lines)
BIO compat stubs have latent multi-use hazards (fixed index, app_data aliasing)
File: wolfssl/openssl/bio.h:196, wolfssl/openssl/ssl.h:1771-1774
Function: wolfSSL_BIO_get_new_index
Severity: Medium
Two BIO compat shims work for a single custom BIO but are unsafe if used more than once: (1) wolfSSL_BIO_get_new_index() always returns the constant 1000 - in OpenSSL each call returns a distinct index, so an app that registers two custom BIO types would get identical type values and collide. (2) BIO_get_app_data/BIO_set_app_data are mapped onto wolfSSL_BIO_get_data/wolfSSL_BIO_set_data, which is the SAME storage slot that a custom BIO method already uses for its method-private data. In OpenSSL these are two independent slots; collapsing them means an app that uses both BIO_set_data (method state) and BIO_set_app_data (app state) on the same BIO will overwrite one with the other. The wolfSSL_BIO_meth_get_* getters returning NULL and set_callback_ctrl being a no-op are acceptable documented stubs.
Recommendation: Keep for the narrow libodbc/libtds use, but document the single-custom-BIO limitation in the header comment. If feasible, back get_new_index with a static incrementing counter, and keep app_data in a separate slot from method data to avoid silent corruption when both are used.
Referenced code: wolfssl/openssl/bio.h:196, wolfssl/openssl/ssl.h:1771-1774 (4 lines)
Review generated by Skoll
|
@kojo1 can you rebase to latest master? That might resolve the CI issues |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 7 total — 6 posted, 1 skipped
6 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] OBJ_find_sigid_algs compat macro placed in ssl.h instead of objects.h with the other OBJ_ macros* —
wolfssl/openssl/ssl.h:1785 - [Medium] WOLFSSL_TLS_ST_OK hardcoded to 16, duplicating internal HANDSHAKE_DONE with no compile-time check —
wolfssl/openssl/ssl.h:1572-1577 - [Medium] BIO_get_app_data/BIO_set_app_data aliased to BIO_get_data/BIO_set_data (shared storage) —
wolfssl/openssl/ssl.h:1773-1775 - [Medium] No test coverage for new wolfSSL_OBJ_find_sigid_algs mapping logic —
wolfssl/openssl/objects.h:79-127 - [Low] BIO compat stubs report success/fixed values with silent limitations —
wolfssl/openssl/bio.h:194-220 - [Low] wolfSSL_OBJ_find_sigid_algs omits RSA-PSS, EdDSA, and MD5 signature NIDs —
wolfssl/openssl/objects.h:83-121
Skipped findings
- [Medium]
SSL_R_* compat macros silently change value from VERSION_ERROR to positive OpenSSL codes
Review generated by Skoll
75d8ed2 to
395fa5f
Compare
|
jenkins, Retest this please. |
09a9d53 to
f16479f
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 3 total — 2 posted, 1 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] New public API wolfSSL_BIO_get_new_index has no test coverage —
src/bio.c:2130-2141 - [Medium] wolfSSL_BIO_get_new_index is not thread-safe, unlike OpenSSL's BIO_get_new_index —
src/bio.c:2130-2141
Skipped findings
- [Low]
Missing WOLFSSL_LEAVE on error return path
Review generated by Skoll
Description
OpenSSL compatibility for libodbc.
Fixes zd#21884
Testing
Integration test with:
[ODBC application]
↓ unixODBC (libodbc.2)
├─→ psqlodbcw.so ─→ libpq.5.dylib ─┐
└─→ libtdsodbc.so ─────────┴─→ libwolfssl.44.dylib
Checklist