UAF with detoasted pgx::datum::Arrays

[eeeebbbbrrrr]

This function demonstrates a use after free if the Array was originally toasted:

#[pg_extern]
fn echo<'a>(a: Array<&'a str>) -> Vec<Option<&'a str>> {
    let v = a.iter().collect();
    drop(a);
    v
}

To create such a situation in Postgres:

[v13.9][25573] arrays=# create table foo (a text[]);
CREATE TABLE
Time: 7.906 ms
[v13.9][25573] arrays=# insert into foo (a) select array_agg(x::text) from generate_series(1, 10000) x;
INSERT 0 1
Time: 13.739 ms
[v13.9][25573] arrays=# select arrays.echo(a) from foo;
                                                                                                                                    >
------------------------------------------------------------------------------------------------------------------------------------>
 {^?,^?,^?,^?,^?,^?,^?,^?,^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^?,^?^>
(1 row)

Using an owned String instead of a &str doesn't have this problem since the String is a copy inside Rust allocated memory. Off the top of my head I'm not sure how to fix it.

[workingjubilee]

Some of the problem here is that for an Array, when we allow Array<&'a str>, but it was detoasted data, it may seem like we can simply return &'a str, but actually our lifetime constraint is &'detoast str, where 'detoast is the lifetime of the actual detoasted allocation, which is an implementation detail of Array that should not be exposed. Accordingly, it's hard to say whether or not it's possible for us to actually pass &'a str through Arrays. The case you cited actually risks exploding the moment you call drop, and attempts to interact with the Vec could also have materialized unsound behavior.

And we may not necessarily want to simply apply the lifetime of the Array as a constraint to the underlying strings, either, though that would be a plausible fix.

A simple way to fix it would be to elevate the constraints on the bounds of the various functions involved here until they say "the data returned must be an owned form", but I am willing to guess that's not exactly desired, as it would involve a huge overhead for the no-op (and entirely valid, if frivolous)

#[pg_extern]
fn echo<'a>(a: Array<&'a str>) -> Array<&'a str> {
    a
}

[workingjubilee]

Thinking about it, it could be required instead that the data, on leaving an Array, is in an owned form, and tightening the lifetime bounds so as to enforce this. This would allow borrowing strings from the Array with low overhead (e.g. if someone wants to just count them up or something like that), but prevent the drop handling from affecting extracting the data from the Array into a Vec, since that's really the transformation we want to preserve.

[workingjubilee]

I did some tampering with the current Array implementation. Even adding a new lifetime param to ArrayIterator doesn't do much to inhibit this. This seems to be a classic example of the LendingIterator problem, which the current impl of GATs does not fully solve.

A cheap way of solving this is to make the bound on get and iter assert that T: 'static, or to add an Owned bound and start returning Owned::ToOwned, but maybe this is overly restrictive. We could still permit ownership-consuming functions because we could use ManuallyDrop to prevent early pfree.

Another way to stop this would be reflecting that kind of difference internally: change our implementations to add something like a BorrowedDatum and OwnedDatum, and then clearly bounding and shredding apart Array's implementations and associated iterators.

[eeeebbbbrrrr]

Looking at addressing this in some manner in #1149.

Basically, we don't free the backing detoasted array pointer and the outstanding &str references then don't have their memory clobbered.

I'd prefer the example code here just not compile, but I don't think our FromDatum framework is robust enough to express this kind of borrowing throughout.

I'm going to keep this issue open as we probably want to come back and actually solve this at compile-time.

[workingjubilee]

Solved in:

• Unbox types properly from arrays #1476
• pfree toasted Arrays on drop #1571