Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:115104 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 55512 invoked from network); 24 Jun 2021 09:37:14 -0000 Received: from unknown (HELO php-smtp4.php.net) (45.112.84.5) by pb1.pair.com with SMTP; 24 Jun 2021 09:37:14 -0000 Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id 4A0121804C3 for ; Thu, 24 Jun 2021 02:55:57 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on php-smtp4.php.net X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,HTML_MESSAGE, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.2 X-Spam-Virus: No X-Envelope-From: Received: from mail1.25mail.st (mail1.25mail.st [206.123.115.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by php-smtp4.php.net (Postfix) with ESMTPS for ; Thu, 24 Jun 2021 02:55:56 -0700 (PDT) Received: from smtpclient.apple (unknown [49.48.221.121]) by mail1.25mail.st (Postfix) with ESMTPSA id 9D886604F7; Thu, 24 Jun 2021 09:55:43 +0000 (UTC) Message-ID: <4DE5E2EC-26D6-4D2C-95A9-B843B440EE87@koalephant.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_92D102E9-B246-4088-A7E4-9EBB061C1151" Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.100.0.2.22\)) Date: Thu, 24 Jun 2021 16:55:40 +0700 In-Reply-To: Cc: Bruce Weirdan , Craig Francis , Larry Garfield , php internals To: Scott Arciszewski References: <03f7955c-69a8-4841-9245-449d7851e207@www.fastmail.com> <95D16F2E-E9DD-4964-A0E2-62E1FB0D976B@koalephant.com> X-Mailer: Apple Mail (2.3654.100.0.2.22) Subject: Re: [PHP-DEV] [RFC] Name issue - is_literal/is_trusted From: php-lists@koalephant.com (Stephen Reay) --Apple-Mail=_92D102E9-B246-4088-A7E4-9EBB061C1151 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 24 Jun 2021, at 14:29, Scott Arciszewski = wrote: >=20 > On Thu, Jun 24, 2021 at 2:10 AM Stephen Reay = wrote: >>=20 >>=20 >>=20 >> On 24 Jun 2021, at 08:30, Scott Arciszewski = wrote: >>=20 >> On Wed, Jun 23, 2021, 9:23 PM Bruce Weirdan = wrote: >>=20 >> On Thu, Jun 24, 2021 at 3:41 AM Scott Arciszewski = >> wrote: >>=20 >> The failure condition of this query is >> "return all rows from the table already being queried", not "return >> arbitrary data the attacker selects from any table that the >> application can read". >>=20 >>=20 >> Imagine that was a DELETE rather than SELECT and the failure = condition >> becomes 'the table is emptied'. >> It may have less disastrous consequences (depending on how good your >> backup / restore procedures are) compared to arbitrary reads you >> demonstrated, but it is still, quite clearly, a glaring security hole >> caused by user input in SQL query - AKA SQL injection in layman's >> terms. >>=20 >> it differs from Injection vulnerabilities in one >> fundamental way: The attacker cannot change the structure of the SQL >> query being executed. >>=20 >>=20 >> I would say replacing a column name with value is changing the >> structure of SQL query, and, basically, in exactly the way you >> describe SQL injection: confusing the code (column name) with data. >>=20 >> I wholeheartedly welcome this RFC as it was originally proposed: >> is_literal() doing exactly what it says on the tin, without any >> security claims. But it has gone far from there real quick and now >> people can't even name the thing. >>=20 >>=20 >> -- >> Best regards, >> Bruce Weirdan mailto: >> weirdan@gmail.com >>=20 >>=20 >>=20 >>=20 >> We can agree that it is a bug. We don't agree on the definition of = SQL >> injection. >>=20 >> Changing a column name to a number (which prepared statements = shouldn't >> allow in the first place) is a bug. This changes the effect of the = command, >> but the *structure* of the query remains unchanged. >>=20 >>=20 >> Hi Scott, >>=20 >> I wrote that example where an integer could be dangerous. >>=20 >> So firstly - just to clarify, because some replies seemed to be = confused on the topic, as was literally mentioned in the original = comment, it is definitely not correct behaviour - it=E2=80=99s a = developer mistake that might work some of the time, and thus go = unnoticed in testing. If you can show me a developer who=E2=80=99s never = inadvertently passed the wrong parameter in some condition, I=E2=80=99ll = show you an imaginary developer. >>=20 >>=20 >> Additionally - pointing out that this is a "developer error=E2=80=9D = doesn=E2=80=99t help your case. Using non-parameterised queries should = already be a =E2=80=9Cdeveloper error=E2=80=9D for anyone who can walk = and breathe at the same time - and yet that usage is being actively = encouraged if this function supports integers. I=E2=80=99ve still seen = zero responses about legitimate reasons this needs to support integers - = giving people a shitty way to build an IN() clause is not legitimate. = Parameterisation exists, and works. >>=20 >>=20 >> I don=E2=80=99t even understand why you mentioned prepared statements = (I guess you meant using parameterised queries?) - the column name = inherently can=E2=80=99t be parameterised - hence having to use a string = substitution in the query. >>=20 >>=20 >> That part was weird and confusing, but not as odd as your claim that = altering the query, so that it causes the where clause to become moot, = is not an SQL Injection? REALLY? That=E2=80=99s your claim? >>=20 >>=20 >> I did a little research, and it turns out Wikipedia = (https://en.wikipedia.org/wiki/SQL_injection#Technical_implementations), = Cloudflare = (https://www.cloudflare.com/en-au/learning/security/threats/sql-injection/= ), and OWASP = (https://owasp.org/www-community/attacks/SQL_Injection#example-2) all = have examples with a `1=3D1` type query manipulation. Do you want to = write and tell them that they=E2=80=99re all wrong, or should I ask them = to call you? >>=20 >> Also, while researching the specifics of what is considered an =E2=80=9C= SQL Injection=E2=80=9D I came across an article, that talks specifically = about the dangers of allowing user input (i.e. the thing `is_trusted` is = meant to prevent) as a column or table identifier. It=E2=80=99s from = this little organisation, you may have heard of them: =E2=80=9CParagon = Initiative=E2=80=9D = (https://paragonie.com/blog/2015/05/preventing-sql-injection-in-php-applic= ations-easy-and-definitive-guide). >>=20 >>=20 >>=20 >> I would absolutely make use of a function that tells me if the string = given is in fact from something controlled by the developer. But once = that same string can also include input from the request or the = environment or whatever by nature of integers, the function becomes = useless for the stated purpose. >>=20 >>=20 >> Cheers >>=20 >> Stephen >>=20 >=20 > One other thing. What do you think the result of this code will be? > How many rows do you think it will spit out? >=20 > ``` > $db =3D new PDO('sqlite::memory:'); >=20 > // These are the only reasonable settings to ever use: > $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); > $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); >=20 > // Let's create a table: > $db->exec("CREATE TABLE foo (username TEXT, pwhash TEXT, email = TEXT)"); >=20 > // Dummy rows: > $db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('scott', > 'lolno', 'scott@paragonie.com')"); > $db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('robyn', > 'fake!', 'robyn@paragonie.com')"); >=20 > // Okay, now let's do a prepared statement, with an integer field > $int =3D 12345; >=20 > $stmt =3D $db->prepare("SELECT * FROM foo WHERE {$int} =3D ?"); > $stmt->execute([12345]); > var_dump($stmt->fetchAll(PDO::FETCH_ASSOC)); > ``` >=20 Two things:=20 (a) try that same query, unaltered on mysql. It won=E2=80=99t be 0 rows. = I=E2=80=99m pretty sure its also not 0 on Postgres. (b) try binding the parameter as an integer and running it on SQLite. It = won=E2=80=99t be zero rows. Actually I can even show you that one: = https://3v4l.org/VL7UC#focus=3D8.0.7 = So what do we call the function now: = is_trusted_string_or_integer_but_dont_dare_use_the_int_as_an_int_or_with_m= ysql_at_all() ? And, this horse is practically glue based on how much I=E2=80=99m = beating it, but still I have to keep asking: Why integers at all? Who is = using =E2=80=9C2=E2=80=9D or some other digit-only identifier as a table = or column name, but also _doesn=E2=80=99t_ define that name as a string? --Apple-Mail=_92D102E9-B246-4088-A7E4-9EBB061C1151--