more ssl impl by youknowone · Pull Request #6228 · RustPython/RustPython

1981-1996: Verify SSL_set_SSL_CTX usage and consider error handling.

The set_context method calls SSL_set_SSL_CTX which is a non-trivial operation that changes the SSL context of an active SSL connection. This can fail or have side effects depending on the SSL connection state.

Concerns:

1. Handshake state: Can this be called mid-handshake or after handshake? CPython may restrict when this can be called.
2. NULL return: Line 1989 checks for NULL but only returns a generic error. Consider capturing and converting the OpenSSL error stack.
3. Session compatibility: Should this check if a session is in progress and whether the new context is compatible?

Apply this improvement:

             // Set the new SSL_CTX on the SSL object
             unsafe {
                 let result = SSL_set_SSL_CTX(stream.ssl().as_ptr(), value.ctx().as_ptr());
                 if result.is_null() {
-                    return Err(vm.new_runtime_error("Failed to set SSL context".to_owned()));
+                    return Err(convert_openssl_error(vm, openssl::error::ErrorStack::get()));
                 }
             }

Verify CPython's _ssl.c to see if there are additional checks on when context can be reassigned (e.g., not after handshake, only for TLS, etc.).

1323-1338: File handle leaks on error path after PEM_read_DHparams.

If PEM_read_DHparams returns NULL (line 1336), the code calls convert_openssl_error which returns early at line 1337, but fclose(fp) was already called at line 1333. However, if PEM_read_DHparams fails, the early return at line 1337 means we correctly don't proceed to use the null dh pointer.

Wait, re-reading: the fclose at line 1333 is AFTER the PEM_read_DHparams call (lines 1325-1330), so the file is always closed. But if dh.is_null() at line 1336, we return at line 1337. This is correct—no leak.

Actually the flow is correct. The file is closed at line 1333, then we check if dh is null and return error if so. No issues here.

527-650: Verify lifetime safety of vm_ptr in SniCallbackData.

The SniCallbackData struct stores a raw pointer to VirtualMachine at line 530, which is dereferenced in the SNI callback at line 604. While the comment at lines 602-604 claims this is safe because "the handshake happens synchronously in the same thread," this depends on the SSL connection lifecycle.

Potential issues:

1. Lifetime guarantee: Is the VM guaranteed to outlive the SSL connection? If the SSL object is stored and used after the original wrapping call returns, the VM pointer could be dangling.
2. Thread safety: The comment assumes synchronous handshake, but what if the socket is passed to another thread?
3. Drop order: When PySslSocket is dropped, is the VM still valid?

Recommendations:

• Document the lifetime contract clearly: "vm_ptr must remain valid for the entire SSL connection lifetime"
• Consider storing a reference-counted VM handle if available, rather than a raw pointer
• Add debug assertions in development builds to detect use-after-free

The current implementation may be safe if:

• The SSL socket holds a reference to the Python socket which keeps the VM alive
• Handshakes only happen synchronously during method calls from Python

Please verify these assumptions hold in all code paths.

3275-3291: LGTM: cipher_description implementation is safe.

The function allocates a 256-byte buffer for SSL_CIPHER_description, which is more than the 128 bytes mentioned in the comment (line 3277) but provides a safety margin. The null-terminator search at line 3288 ensures safe string conversion.

Good defensive programming with the oversized buffer and proper null-terminator handling.

1490-1492: Good: null byte check in server_hostname.

Adding the null byte check for server_hostname at lines 1490-1492 (in _wrap_socket) and 1614-1616 (in _wrap_bio) prevents potential security issues with embedded null bytes in hostnames. This is a good defensive practice.

Also applies to: 1614-1616

2501-2508: Good: special-case handling for zero-byte reads.

The early return for n == 0 at lines 2501-2508 is a good optimization that avoids unnecessary SSL and socket operations when no data is requested. This matches expected behavior where reading 0 bytes should immediately return 0 or an empty bytes object.

782-782: Verify BASETYPE flag is appropriate for PySslContext.

Line 782 marks PySslContext with both BASETYPE and IMMUTABLETYPE flags, while other SSL types (PySslSocket, PySslMemoryBio, PySslSession) use only IMMUTABLETYPE. The BASETYPE flag allows Python subclassing.

Questions:

1. Does CPython's _ssl.SSLContext allow subclassing? If not, remove BASETYPE.
2. Is it safe to subclass SSLContext given it holds SslContextBuilder and uses unsafe FFI?
3. Do any internal implementations rely on the type being final/non-subclassable?

Check CPython's implementation to confirm whether SSLContext should be subclassable. If it should not be, apply this diff:

-    #[pyclass(flags(BASETYPE, IMMUTABLETYPE), with(Constructor))]
+    #[pyclass(flags(IMMUTABLETYPE), with(Constructor))]
     impl PySslContext {

Also applies to: 1960-1960, 2892-2892, 2963-2963