PyString::from_object does not convert rust strings to c-strings before calling FFI, causing OOB reads

[vthib]

Bug Description

PyString::from_object takes &str parameters and passes them through to a ffi function that expects c-style strings. This causes OOB accesses and potential crashes.

Steps to Reproduce

use pyo3::prelude::*;
use pyo3::types::{PyBytes, PyString};

fn main() {
    pyo3::prepare_freethreaded_python();
    Python::with_gil(|py| {
        let py_bytes = PyBytes::new(py, b"ab\xFFcd");

        // The following line is faulty
        let py_string = PyString::from_object(&py_bytes, "utf-8", "ignore").unwrap();
        // This would work:
        // let py_string = PyString::from_object(&py_bytes, "utf-8\0", "ignore\0").unwrap();

        let result = py_string.to_str().unwrap();
        assert_eq!(result, "abcd");
    });
}

This returns something like this when executed:

thread 'main' panicked at src/main.rs:8:77:
called `Result::unwrap()` on an `Err` value: PyErr { type: <class 'LookupError'>, value: LookupError('unknown encoding: utf-8ignoresrc/main.rsabcd/home/foo/.cargo/registry/src/index.crates.io-1949cf8c6b...

which seems expected since those strings are in .rdata, but will depend a lot depending on how those strings are built.

Your operating system and version

Any

Your Python version (python --version)

3.11.2

Your Rust version (rustc --version)

1.85.1

Your PyO3 version

0.24.0

How did you install python? Did you use a virtualenv?

apt, no virtualenv

Additional Info

Thanks a lot for this great project :)

I stumbled upon this issue when trying to use this function to guarantee the same bytes => string conversion with an existing python library.

[alex]

Yeah, this seems very clearly to be a soundness hole.

It also looks like we don't have any tests for it (which is I guess why we never noticed).

[davidhewitt]

Sigh, yes. Thank you for reporting. Looks like this function has existed for all of PyO3's history, probably both unused and untested. 8363260

I think we can make a patch fix which internally creates a temporary CString when needed. We should probably also deprecate the API, maybe make a new PyString::from_encoded_object which takes &CStr arguments.

Given this function has existed for all time, and that a developer which is using this API probably creates a crash immediately, I think we should issue an advisory but it's not worth yanking anything.

[davidhewitt]

Reported at rustsec/advisory-db#2266