Share more ssl consts and fix openssl #6462
Conversation
WalkthroughRefactors SSL error constants and error construction, moves SSL_ERROR_* constants to ssl/error.rs with Python exposure, and implements thread-local handshake state, SSL ex_data storage, and a full Python-invoking message callback; also adjusts read/shutdown/BIO behavior and file-path validation for SSL inputs. Changes
Sequence DiagramsequenceDiagram
participant OS as OpenSSL
participant CB as _msg_callback (native)
participant TLS as SSL* (ex_data)
participant TL as Thread-Local (HANDSHAKE_VM / SSL_PTR)
participant VM as Python VM
participant PS as PySslSocket
participant UCB as User Callback
OS->>CB: call _msg_callback(ssl, content_type, msg_type, buf, len, arg)
CB->>TL: load HANDSHAKE_VM / HANDSHAKE_SSL_PTR
alt handshake VM present
TL-->>CB: vm_ref, ssl_ptr
else no VM
CB-->>OS: return (no-op)
end
CB->>TLS: read ex_data -> PySslSocket pointer
TLS-->>CB: pyssl socket (PS)
CB->>VM: create Python bytes from buf, build args (conn, direction, version, content_type, msg_type, data)
CB->>VM: invoke user callback via PS
VM->>UCB: execute user callback
alt callback returns
UCB-->>VM: result
VM-->>CB: success
CB-->>OS: return success
else callback raises
UCB-->>VM: exception
VM-->>CB: exception info
CB->>CB: run_unraisable(exception)
CB-->>OS: return (exception handled)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧬 Code graph analysis (1)crates/stdlib/src/openssl.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/stdlib/src/openssl.rs(20 hunks)crates/stdlib/src/ssl.rs(0 hunks)crates/stdlib/src/ssl/error.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/stdlib/src/ssl.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/stdlib/src/openssl.rscrates/stdlib/src/ssl/error.rs
🧬 Code graph analysis (1)
crates/stdlib/src/openssl.rs (2)
crates/stdlib/src/ssl/error.rs (3)
create_ssl_eof_error(120-126)create_ssl_want_read_error(104-110)create_ssl_want_write_error(112-118)crates/stdlib/src/ssl.rs (4)
_msg_callback(1669-1671)read(3183-3316)read(4050-4070)e(1109-1109)
🔇 Additional comments (11)
crates/stdlib/src/ssl/error.rs (2)
13-33: LGTM! SSL error constants properly defined.The error constants are correctly defined with appropriate values matching OpenSSL/CPython conventions, properly annotated with
#[pyattr]for Python exposure, and use suitablepub(crate)visibility for internal use while being accessible to Python.
120-134: LGTM! Error helper functions correctly updated.The helper functions now properly pass specific SSL error codes (
SSL_ERROR_EOFandSSL_ERROR_ZERO_RETURN) as errno values, providing richer error information consistent with the PR's error handling improvements.crates/stdlib/src/openssl.rs (9)
56-60: LGTM! Import changes align with centralized error handling.The imports correctly reflect the migration from direct error type usage to helper functions (
create_ssl_*helpers), and the use of canonical error type names (PySSLCertVerificationError,PySSLError) improves consistency.
1807-1828: Review ex_data pointer management for safety.The ex_data setup stores raw pointers to
Py<PySslSocket>(index 0) andSniCallbackData(SNI index):
Index 0 pointer (lines 1814-1815, 1882-1883): Stores
&*py_refas*const Py<PySslSocket>. This is safe because:
py_ref(thePyRef<PySslSocket>) is returned to Python, so the object remains alive.- The SSL connection is owned by
PySslSocket.connection, so the SSL object cannot outlive thePySslSocket.- Callbacks dereference this pointer, which remains valid as long as the
PySslSocketexists.SNI callback data (lines 1821-1827, 1889-1895): Uses
Box::into_rawto leak theSniCallbackData, with cleanup viacleanup_sni_ex_dataafter handshake. The weak reference toPySslSocketprevents reference cycles.The implementation appears correct. However, the safety relies on the invariant that the SSL object (stored in
PySslSocket.connection) does not outlive thePySslSocketitself. This is enforced by Rust ownership, but worth documenting explicitly.Also applies to: 1875-1896
1983-2000: LGTM! Socket select logic correctly avoids busy-looping.The updated logic properly handles blocking sockets by passing
Noneas the timeout tosock_select, allowing the socket to wait indefinitely instead of busy-looping. This is a correctness improvement that aligns with the comment on lines 1983-1984.
2290-2302: LGTM! Thread-local SSL pointer pattern correctly avoids deadlocks.These methods now use
get_ssl_ptr_for_context_changeinstead of directly acquiringconnection.read(). This pattern avoids deadlocks when these methods are called during callbacks (SNI, msg_callback) where the connection lock is already held:
- During handshake/callbacks:
HANDSHAKE_SSL_PTRthread-local is set byHandshakeVmGuard, so the SSL pointer is retrieved without locking.- Outside callbacks: Thread-local is
None, so the lock is safely acquired.This is a well-designed solution for the callback context problem.
Also applies to: 2311-2372, 2375-2398, 2509-2522, 2724-2728
2538-2541: LGTM! Handshake error handling correctly uses canonical error types.The handshake error paths now use
PySSLCertVerificationError::class()(canonical name) and properly set verification info on certificate verification errors, consistent with the PR's error handling improvements.Also applies to: 2593-2597
2731-2838: LGTM! Read method signature and logic correctly updated.The parameter type change from
i32toisizealigns with CPython's use ofPy_ssize_t. The complex logic for handling negativenand buffer presence (lines 2737-2756) correctly implements CPython's behavior:
- With buffer: If
n <= 0orn > buffer_len, usebuffer_len; otherwise usen.- Without buffer: Raise
ValueErrorifn < 0.The buffer slicing at lines 2777-2780 includes a safety fallback, though it should not normally execute given the earlier validation. The zero-length read optimization (lines 2759-2765) is appropriate.
3318-3427: LGTM! Error handling consistently uses canonical error types.The
convert_openssl_errorandnew_ssl_errorfunctions now consistently use canonical error type names (PySSLError,PySSLCertVerificationError) instead of aliases, improving code consistency and aligning with the PR's error handling refactoring.
3454-3501: LGTM! SSL error conversion uses centralized helper functions.The
convert_ssl_errorfunction now uses helper functions (create_ssl_want_read_error,create_ssl_want_write_error,create_ssl_eof_error) for specific error cases, aligning with the PR's centralized error handling approach. The error detection logic remains correct, with proper handling of WANT_READ/WANT_WRITE, SYSCALL, and unexpected EOF cases.
1345-1347: LGTM! ASCII validation for cadata string is appropriate.The added ASCII check for
cadatawhen provided as a string (line 1345) is correct, as PEM format is ASCII-based. This improves input validation and provides a clear error message for invalid input.
| unsafe extern "C" fn _msg_callback( | ||
| _write_p: libc::c_int, | ||
| _version: libc::c_int, | ||
| _content_type: libc::c_int, | ||
| _buf: *const libc::c_void, | ||
| _len: usize, | ||
| _ssl_ptr: *mut sys::SSL, | ||
| write_p: libc::c_int, | ||
| mut version: libc::c_int, | ||
| content_type: libc::c_int, | ||
| buf: *const libc::c_void, | ||
| len: usize, | ||
| ssl_ptr: *mut sys::SSL, | ||
| _arg: *mut libc::c_void, | ||
| ) { | ||
| // Intentionally empty to avoid deadlocks | ||
| if ssl_ptr.is_null() { | ||
| return; | ||
| } | ||
|
|
||
| unsafe { | ||
| // Get SSL socket from SSL_get_ex_data (index 0) | ||
| let ssl_socket_ptr = sys::SSL_get_ex_data(ssl_ptr, 0); | ||
| if ssl_socket_ptr.is_null() { | ||
| return; | ||
| } | ||
|
|
||
| // ssl_socket_ptr is a pointer to Py<PySslSocket>, set in _wrap_socket/_wrap_bio | ||
| let ssl_socket: &Py<PySslSocket> = &*(ssl_socket_ptr as *const Py<PySslSocket>); | ||
|
|
||
| // Get the callback from the context | ||
| let callback_opt = ssl_socket.ctx.read().msg_callback.lock().clone(); | ||
| let Some(callback) = callback_opt else { | ||
| return; | ||
| }; | ||
|
|
||
| // Get VM from thread-local storage (set by HandshakeVmGuard in do_handshake) | ||
| let Some(vm_ptr) = HANDSHAKE_VM.with(|cell| cell.get()) else { | ||
| // VM not available - this shouldn't happen during handshake | ||
| return; | ||
| }; | ||
| let vm = &*vm_ptr; | ||
|
|
||
| // Get SSL socket owner object | ||
| let ssl_socket_obj = ssl_socket | ||
| .owner | ||
| .read() | ||
| .as_ref() | ||
| .and_then(|weak| weak.upgrade()) | ||
| .unwrap_or_else(|| vm.ctx.none()); | ||
|
|
||
| // Create the message bytes | ||
| let buf_slice = std::slice::from_raw_parts(buf as *const u8, len); | ||
| let msg_bytes = vm.ctx.new_bytes(buf_slice.to_vec()); | ||
|
|
||
| // Determine direction string | ||
| let direction_str = if write_p != 0 { "write" } else { "read" }; | ||
|
|
||
| // Calculate msg_type based on content_type (debughelpers.c behavior) | ||
| let msg_type = match content_type { | ||
| SSL3_RT_CHANGE_CIPHER_SPEC => SSL3_MT_CHANGE_CIPHER_SPEC, | ||
| SSL3_RT_ALERT => { | ||
| // byte 1 is alert type | ||
| if len >= 2 { buf_slice[1] as i32 } else { -1 } | ||
| } | ||
| SSL3_RT_HANDSHAKE => { | ||
| // byte 0 is handshake type | ||
| if !buf_slice.is_empty() { | ||
| buf_slice[0] as i32 | ||
| } else { | ||
| -1 | ||
| } | ||
| } | ||
| SSL3_RT_HEADER => { | ||
| // Frame header: version in bytes 1..2, type in byte 0 | ||
| if len >= 3 { | ||
| version = ((buf_slice[1] as i32) << 8) | (buf_slice[2] as i32); | ||
| buf_slice[0] as i32 | ||
| } else { | ||
| -1 | ||
| } | ||
| } | ||
| SSL3_RT_INNER_CONTENT_TYPE => { | ||
| // Inner content type in byte 0 | ||
| if !buf_slice.is_empty() { | ||
| buf_slice[0] as i32 | ||
| } else { | ||
| -1 | ||
| } | ||
| } | ||
| _ => -1, | ||
| }; | ||
|
|
||
| // Call the Python callback | ||
| // Signature: callback(conn, direction, version, content_type, msg_type, data) | ||
| match callback.call( | ||
| ( | ||
| ssl_socket_obj, | ||
| vm.ctx.new_str(direction_str), | ||
| vm.ctx.new_int(version), | ||
| vm.ctx.new_int(content_type), | ||
| vm.ctx.new_int(msg_type), | ||
| msg_bytes, | ||
| ), | ||
| vm, | ||
| ) { | ||
| Ok(_) => {} | ||
| Err(exc) => { | ||
| // Log the exception but don't propagate it | ||
| vm.run_unraisable(exc, None, vm.ctx.none()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SSL_set_ex_data calls with index 0 in openssl.rs
grep -n "SSL_set_ex_data" crates/stdlib/src/openssl.rs | head -20Repository: RustPython/RustPython
Length of output: 490
🏁 Script executed:
#!/bin/bash
# Examine SSL_set_ex_data setup and any cleanup patterns
echo "=== Finding SSL_set_ex_data calls ==="
grep -n "SSL_set_ex_data" crates/stdlib/src/openssl.rs
echo -e "\n=== Examining _wrap_socket around lines 1814-1815 ==="
sed -n '1800,1830p' crates/stdlib/src/openssl.rs
echo -e "\n=== Examining _wrap_bio around lines 1882-1883 ==="
sed -n '1870,1900p' crates/stdlib/src/openssl.rs
echo -e "\n=== Looking for PySslSocket drop implementation ==="
grep -n "impl.*Drop.*PySslSocket" crates/stdlib/src/openssl.rs
echo -e "\n=== Looking for SSL_free or cleanup of SSL object ==="
grep -n "SSL_free" crates/stdlib/src/openssl.rsRepository: RustPython/RustPython
Length of output: 3865
Critical: Use-after-free vulnerability in _msg_callback ex_data access.
Lines 1815 and 1883 store a pointer to a local reference in SSL ex_data at index 0:
let py_ptr: *const Py<PySslSocket> = &*py_ref;
sys::SSL_set_ex_data(ssl_ptr, 0, py_ptr as *mut _);The problem: py_ref is a temporary reference created by into_ref_with_type() that goes out of scope when the function returns. The pointer stored in ex_data becomes a dangling pointer. When _msg_callback later dereferences it (line 742), it accesses freed memory—a use-after-free vulnerability.
The code comment claims "ssl_socket owns the SSL object and outlives it," but the implementation stores a pointer to a stack reference, not to the Py<PySslSocket> object itself.
Fix: Store a clone of Py<PySslSocket> or use a Box allocation that persists for the SSL object's lifetime, and ensure it's cleaned up via an ex_data free callback registered with SSL_get_ex_new_index.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.