Re: [RFC] is_trusted - was is_literal
Sent from my iPhone
> On 23 Jun 2021, at 03:08, Lauri Kenttä <lauri.kentta@gmail.com> wrote:
>
> 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ä
>
Yes it very deliberately shows a case where developer error can lead to incorrect behaviour, because
of a bug. I don’t think it’s a stretch to say that not validating the dynamic elements used to
generate a query would also be considered a bug caused by developer error.
Given that this is meant to be a security feature, an approach of “well this is probably fine”
seems like a pretty cavalier approach to start from, especially given that there are existing
solutions to prevent injections of integer values.
Rather than people keep repeating “integers aren’t injections”, perhaps someone can show just
one example why you’d need to use a literal integer as an identifier in sql (ie a table name etc)
and thus can’t use parameterisation.
Thread (24 messages)