Potential use after free in Statement::column_names() and similar functions

[weiznich]

While investigating the consequences of diesel-rs/diesel#2663 I had a look at the corresponding rusqlite code to gather some information how the underlying problem of the "strange" lifetime of the column name pointer is handled there. While doing that I noticed that at least one case is also not handled here.

The returned string pointer is valid until either the prepared statement is destroyed by sqlite3_finalize() or until the statement is automatically reprepared by the first call to sqlite3_step() for a particular run or until the next call to sqlite3_column_name() or sqlite3_column_name16() on the same column.

SQLite Documentation for sqlite3_column_name (emphasis by me)

That sounds for me that calling this function from a user application at least twice and using the first result after the second call will be a potential use after free. The core issue here is that this function just forwards every call to the corresponding sqlite3_column_name function. (This likely applies to any other function in column.rs which internally calls that function in raw_statement.rs).

[gwenn]

I guess you need a mutable reference to Statement to reprepared / step, no ?
So you should not be able to keep a reference to column names and step.

And if you call column_names twice, they should share the same references.
But to be sure, we will add a unit test...

[thomcc]

We should probably just returned an owned copy of the names in either case, but it would be good to know if it actually causes a problem in practice to know how big of a deal this is.

[weiznich]

@gwenn @thomcc The sqlite documentation explicitly states that the return pointer is only valid till the next call to sqlite3_column_name for the same column. Its hard to show how this could cause problems in real code as this requires triggering something that actually overrides the freed pointer.

[thomcc]

Yeah, I definitely agree we should fix this soon.

I'd like to figure out if it's actually a UAF¹ (and if so how likely it is), which makes a difference for how urgent the fix is² and whether or not it's worth backporting to the last stable release of rusqlite (the current release has a decent amount of breakage, although honestly we should be getting the release out soon anyway — it's been long enough, probably).

So, from what I can tell from here and here, it will overwrite the same buffer with the same column name. We never call sqlite_column_name16 so I think that's not a concern... So the only case I can come up with where it could be a UAF might be if ALTER TABLE RENAME COLUMN happens elsewhere? Not sure...

That said: if you actually saw a UAF, then it's probably not a crazy edge case like that, and I just misread it. Anyway, some minor experimentation and this reading doesn't tell me it's definitely super urgent, so I'm going to see if our asan support and/or -DSQLITE_MEMDEBUG can reveal anything...

Admittedly, the right course of action here is to cut something ASAP and backport it, but I'd like to make sure all the cases of this are caught in one go, and also I'd like to avoid making the API much worse where possible.

---

¹ I had actually assumed the invalidation in step and reset were referring to overwriting a local buffer...

² Of course, even if it's fine now, ultimately we have to assume that a future version of SQLite will update this code to blow up in our faces, so there's some urgency either way.

[trevyn]

(A quick note on major release timing: SQLite 3.35.0 is targeted for release around March 30th, and includes a long-awaited ALTER TABLE DROP COLUMN feature, and it would be awesome to have a rusqlite release shortly after that... drops.)

[weiznich]

That said: if you actually saw a UAF, then it's probably not a crazy edge case like that, and I just misread it.

To be clear I do not have any code to show a actual consequence of an UAF for this. I only came here to have a look how rusqlite handles column names and get a bit inspiration from that after we found that issue in diesel. While doing that I noticed that this would be a theoretical UAF at least given what's documented in the SQLite API documentation. I do not believe that's something super urgent to be fixed, but you should probably change that with the next normal release.

[gwenn]

Are you ok to introduce trybuild tests that don't compile ?

        let t = trybuild::TestCases::new();
        t.compile_fail("tests/column_names.rs");

fn main() -> Result<() {
        let db = Connection::open_in_memory()?;
        let mut stmt = db.prepare("SELECT 1 as x")?;
        let column_names = stmt.column_names();
        let x = stmt.query_row([], |r| r.get::<_, i64>(0))?;
        assert_eq!(1, x);
        assert_eq!(1, column_names.len());
        Ok(())
}

error[E0502]: cannot borrow `stmt` as mutable because it is also borrowed as immutable
   --> src/column.rs:243:17
    |
242 |         let column_names = stmt.column_names();
    |                            ---- immutable borrow occurs here
243 |         let x = stmt.query_row([], |r| r.get::<_, i64>(0))?;
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
244 |         assert_eq!(1, x);
245 |         assert_eq!(1, column_names.len());
    |                       ------------ immutable borrow later used here

[thomcc]

I think we could probably add a test that we expect to fail via a dummy compile_fail doctest, no? Maybe on something like RawStatement::column_names() which isn't public but is related?

[gwenn]

See #918

[thomcc]

Hmm, we should probably fix this anyway just to avoid any possible future issues. It's pretty annoying, though.