Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115043
MIME-Version: 1.0
Date: Tue, 22 Jun 2021 23:01:15 +0300
To: Stephen Reay <php-lists@koalephant.com>
Cc: Craig Francis <craig@craigfrancis.co.uk>, Benjamin Morel
 <benjamin.morel@gmail.com>, Derick Rethans <derick@php.net>, PHP Internals
 <internals@lists.php.net>, Yasuo Ohgaki <yohgaki@ohgaki.net>
In-Reply-To: <358756DC-87F9-4FD9-AE62-CB27BA296F33@koalephant.com>
References: <CAFv4g+GjXqiZkA2fawt7jHDtDAKjB0rv=Lfbcobu7bn8B=wvpw@mail.gmail.com>
 <CAGa2bXbGv33zHfRpu-hN_wNcNKPpFw_ed1ZRnn96tpDf0ROcLg@mail.gmail.com>
 <0CD1762E-6094-4DEB-B1B5-22CFBDAAFF44@php.net>
 <CAG9XoMQUUYGhA6QL3Se4FNJgydjzX0MAeR_CWr5O1SH8xgypLA@mail.gmail.com>
 <CAFv4g+ET4YDpygjKqGbAwxt+jkA8kWTeUrmTcR3hbFGEGNgmDg@mail.gmail.com>
 <BFFD3FBB-721B-491B-ADDB-DE3CA535BB52@koalephant.com>
 <CAFv4g+GJrX6uNSAq=kAXDk2Lx=NLx=GxAw9Qh+cpZwbaVdgtJA@mail.gmail.com>
 <9B304735-E0AD-4CC0-98BF-AAF4CE5FA52C@koalephant.com>
 <CAFv4g+GuqPYHjAzQ2YzdBh2R5+62xneZCz-Wdj5cE8On4BpHog@mail.gmail.com>
 <358756DC-87F9-4FD9-AE62-CB27BA296F33@koalephant.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <a6f8a38404f5bce7c3c93a9f86c76b76@gmail.com>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [RFC] is_trusted - was is_literal
From: lauri.kentta@gmail.com (=?UTF-8?Q?Lauri_Kentt=C3=A4?=)

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ä