Re: [RFC] is_trusted - was is_literal

From: Date: Tue, 22 Jun 2021 20:01:15 +0000
Subject: Re: [RFC] is_trusted - was is_literal
References: 1 2 3 4 5 6 7 8 9 10  Groups: php.internals 
Request: Send a blank email to internals+get-115043@lists.php.net to get a copy of this message 
On 2021-06-22 21:38, Stephen Reay wrote:

On 22 Jun 2021, at 21:38, Craig Francis <craig@craigfrancis.co.uk> wrote:

If you can point me to an example where including integers in this has
introduced a security vulnerability then please do, and I mean it, that’s
what this process is for, I genuinely want people to come forward with them
so we can refine this.



It took me about a minute to think of this:

 "select * from customer_purchases where {$column} = :value”.

The developer inadvertently passes the same “trusted value” in as the
$column substitute and the value parameter. It must be safe because
we ran it through is_trusted!

 The query now executes as:

 "select * from customer_purchases where 12345 = 12345”


You cannot magically make all dynamically generated queries safe -
they tried that about a quarter of a century ago. Hint: it did not end
well - and explicitly allowing some user input is just mind boggling
given the stated goals.



Your example is interesting and kind of valid. Looks like there is a bug in the code: $column is sometimes an user-defined integer and sometimes a valid literal column name, clearly this should not happen. (If it was supposed to be integer, you would pass it as a parameter just like :value, right?)

Is this RFC about preventing bugs (accidentally used wrong variable) or preventing bad code (user input intentional but used the wrong way)? I thought it was more the about bad code. Bugs can live even in literal queries as well as outside queries, where is_literal/is_trusted can't reach them.

Another line of thought: One possible approach would be to accept only explicit integer casts (sprintf %d and %u, and a new function like implode_ints() and/or strval_int()) and otherwise only accept strings. This would avoid the accidental case where $x is supposed to be a trusted string but is an untrusted integer instead, like the given example.

-- 
Lauri Kenttä

Thread (24 messages)

« previous php.internals (#115043) next »