security: fix use-after-free in PyCapsule implementation

[davidhewitt]

Thanks to @saethlin for alerting me to this issue.

The current implementation of PyCapsule::new does not require a specific lifetime on the name: &CStr argument. This is a fundamental flaw; the argument is converted to a pointer and handed over to CPython, which simply stores the pointer.

This means that as soon as PyCapsule::new is complete, current implementations can deallocate the name and any subsequent calls to PyCapsule_GetName will read from freed memory (including our safe PyCapsule::name wrapper).

A snippet as simple as this is enough to achieve this:

let capsule: Py<PyCapsule> = Python::with_gil(|py| {
    let name = CString::new("foo").unwrap();
    PyCapsule::new(py, 5usize, &name).unwrap().into()
});

Python::with_gil(|py| {
    let name = capsule.as_ref(py).name();  // <-- Reading from freed memory previous to this PR's fix
});

The fix without any API change is to take a copy of the name as a CString and store that where we place the rest of the capsule data. We can then pass CPython the pointer from there, which guarantees that the name will outlive the capsule.

---

Given this is a reasonably clear-cut soundness error, I think we should file a RustSec advisory and also yank versions 0.16.0 -> 0.16.5 (after publishing this as a 0.16.6).

[adamreichold]

For 0.17.0, we might want to change name to Option<&str> as the Python seems to accept NULL as well as a pointer to a null-terminated string. Taking &CStr seems unnecessary when we allocate a new buffer in any case. Alternatively, we might want to take Option<CString> so calling can pass ownership of an existing null-terminated string.

[adamreichold]

It isn't visible in the diff here, but while reviewing that, I was wondering whether the closure type F should have a Send bound like the value T since it is potentially called on a different thread than the one that created the capsule?

[davidhewitt]

It isn't visible in the diff here, but I while reviewing that, I was wondering whether the closure type F should have a Send bound like the value T since it is potentially called on a different thread than the one that created the capsule?

Yes, I think you are correct. This is arguably also a soundness issue so I'm now wondering whether to also backcourt that to 0.16, however it is also breaking so that's a bit of a disaster.

For 0.17.0, we might want to change name to Option<&str> as the Python seems to accept NULL as well as a pointer to a null-terminated string. Taking &CStr seems unnecessary when we allocate a new buffer in any case. Alternatively, we might want to take Option<CString> so calling can pass ownership of an existing null-terminated string.

Arrgh that's a great point and means that in PyCapsule::name we also can convert a null-pointer to CStr.

I see three possible choices for the name argument:

• Option<&str> and we take a copy always
• Option<CString> and we take ownership
• Option<Cow<'static, CStr>> and we can then allow to reuse static data to avoid allocating

Think the Cow is probably overkill for something that's not going to be in a hot loop. IMO Option<&str> is very attractive for simplicity for users, although I guess we would have to raise an error if they passed an internal NUL. Which choice do you prefer? (Or something else?)

[adamreichold]

Which choice do you prefer? (Or something else?)

After musing about it, I'd prefer Option<CString>: Working with capsules is pretty low-level to begin with which is why I'd prefer exposing the CString stored in the CapsuleData as directly as possible instead of going for the convenience of Option<&str>. The Cow-based version looks like too much to me, especially since our capsule wrapper will allocate in any case (for boxing and for the capsule on the Python heap).

This is arguably also a soundness issue so I'm now wondering whether to also backcourt that to 0.16, however it is also breaking so that's a bit of a disaster.

So we have three pending changes now:

• PyCapsule::new needs to ensure ownership of name.
• The destructor F needs a Send bound.
• PyCapsule::name needs to handle the name pointer being NULL.

I think the third problem could also be fixed in a backward-compatible manner by returning a static reference to the empty string b"\0" instead, but at this point we might want to bite the bullet and break the interface to go for name: Option<CString> and -> Option<&CStr> in addition to fixing the Send bound?

[davidhewitt]

Agreed, Option<CString> seems especially reasonable if name() continues to return Option<CStr> (which I think is correct).

So I did some more thinking on this and I believe we can fix all issues in a non-breaking fashion as far as a 0.16 backport is concerned:

• take ownership of CString for the name as per this PR
• use static b"\0" CStr in name() as you suggest above
• print warning and return early without running the F destructor if not on the same thread as the one in which the PyCapsule was created

Given that the third fix could be quite annoying for users (although hopefully not many people are relying on such an edge case), I think we should also work on getting 0.17 out the door ASAP so that there's an upgrade pathway to a sound implementation.

I'll try to push some PRs and a plan tomorrow.

[adamreichold]

print warning and return early without running the F destructor if not on the same thread as the one in which the PyCapsule was created

👍🏽 This sounds reasonable as a soundness fix to me.

[davidhewitt]

To keep the capsule rework separate from this sanitiser fix, I'm going to merge this now and open up follow-up PRs.