Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115014
MIME-Version: 1.0
Date: Tue, 22 Jun 2021 12:48:08 +0300
To: Craig Francis <craig@craigfrancis.co.uk>, Joe Watkins
 <krakjoe@gmail.com>
Cc: PHP internals <internals@lists.php.net>
In-Reply-To: <CAFv4g+GjXqiZkA2fawt7jHDtDAKjB0rv=Lfbcobu7bn8B=wvpw@mail.gmail.com>
References: <CAFv4g+GjXqiZkA2fawt7jHDtDAKjB0rv=Lfbcobu7bn8B=wvpw@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <7b1af9310312e0b1435b93490160e08d@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-21 23:25, Craig Francis wrote:
> 
> - Integers are now included, which will help adoption:
> 
> https://wiki.php.net/rfc/is_literal
> 

Thanks for the great improvements!

sprintf seems to have some issues, though.

Currently you're checking the parameter types, not the formats.
The parameter type matters only when coercing to a string (%s).
Otherwise sprintf should consider the format, not the parameter.

Example:
<?php
function test($s) { var_dump($s, is_trusted($s)); }
setlocale(LC_ALL, "de_DE.UTF-8");
test(sprintf("SET c = %c, f = %f, e = %e", 0x27, 1234, 1234));
test(sprintf("SET d = %d, x = %x, b = %b", 1e2, 1e2, 1e2));
test(sprintf("SET weird_d = %''*d", 4, 1));
test(sprintf("SET s = '%s', int to string should be ok", 123));
?>

Currently:
string(43) "SET c = ', f = 1234,000000, e = 1.234000e+3"
bool(true)
string(32) "SET d = 100, x = 64, b = 1100100"
bool(false)
string(18) "SET weird_d = '''1"
bool(true)
string(41) "SET s = '123', int to string should be ok"
bool(true)

Obviously the results with ints and floats should be the opposite.

If you really want to allow %c, so be it, but I'd disallow it on the 
grounds that (1) it's probably not used in secure strings (usage data, 
anyone?), and thus (2) it could easily be a misspelled %d (for example, 
'%c' instead of '%d' could silently produce an empty result in a query), 
and (3) you're allowing a simple workaround with %s and chr() which 
makes the intent more obvious.

In general, as this is supposed to be a security feature, allowing 
multiple ways for uninformed people to produce "trusted" but actually 
very unsafe values doesn't look like a good idea. I'm not sure if 
allowing trusted single characters to be created through chr or %c 
serves any useful purpose, but I can imagine people using either one 
without realizing that they can create any character, including \0 or ' 
or " or non-UTF-8. Better to leave only chr(), one less thing to worry 
about.

Custom padding is a weird edge case, maybe just disallow that too?

As you said yourself, it's not easy to prove anything safe. ;)

-- 
Lauri Kenttä