On 2021-06-21 23:25, Craig Francis wrote:
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ä