Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115387
MIME-Version: 1.0
References: <CAFv4g+HRHrvCOTgpbVfCXj06De1kL+1+RwnGgq6tcyLNAtLfQA@mail.gmail.com>
 <CAFPFaMJs7qvko=tfBsXuwCqMV3VqXL4TjO2aKWWPjrAicZzM9w@mail.gmail.com> <CAFv4g+G85cfjykjKRbOwWg+KXctZ1u93Onu5cLh1bNrtQ0=DPg@mail.gmail.com>
In-Reply-To: <CAFv4g+G85cfjykjKRbOwWg+KXctZ1u93Onu5cLh1bNrtQ0=DPg@mail.gmail.com>
Date: Sun, 11 Jul 2021 18:09:24 +0100
Message-ID: <CA+kxMuQkBevK5k-Khhpo3tFfHXeSrM+-GCOWRoZoKJ8Y=_mV4Q@mail.gmail.com>
To: Craig Francis <craig@craigfrancis.co.uk>
Cc: "G. P. B." <george.banyard@gmail.com>, PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] [RFC] [VOTE] is_literal
From: Danack@basereality.com (Dan Ackroyd)

On Tue, 6 Jul 2021 at 10:32, Craig Francis <craig@craigfrancis.co.uk> wrote:
> which will result in too many invalid errors, requiring them to make
> substantial/unnecessary changes

My understanding is that the majority of PHP developers use one of the
many frameworks available. All of those provide libraries that people
use rather than accessing the DB directly.

At the other end of the scale, you have companies with large-ish
engineering teams (e.g. 30+) where access to the DB is done through an
in-house library.

The only people who don't already go through an access layer of code
to reach the DB are people who don't depend on frameworks, and
maintain their own applications with a small (or singular) engineering
team.

This type of person may be over-represented in internals, but are
probably relatively few in number compared to the majority of PHP
users.

> requiring them to make substantial/unnecessary changes

My very strong suspicion is that if/when people start using this, the
majority of changes that would be required would be actual security
problems that ought to be fixed.

> (i.e. replacing every string concat with
> `literal_concat()`, or using a special Query Builder for their
> SQL/HTML/CLI/etc).

Most people should be using a library for building HTML and CLI
escaping, because remembering how to do the escaping properly is
really difficult. I certainly never remember the difference between
the correct escaping for CSS and JS.

But the changes required for supporting DB queries just don't seem
that big to me. I've put a code example of an implementation below.

I'm pretty sure that the changes needed to fix the actual security
problems that exist in their code would be bigger, and also that
tracking down a single leaked non-literal would take longer than
migrating code to use the new feature.

As I said in my reply to Rowan, making it easy to track down issues
where they occur, minimises the cost of using this feature over the
years it would be used.

cheers
Dan
Ack


# Before

$query = "select * from preferences user_id = " . $_GET['user_id'];

db_exec($query);

# After

$query[] = "select * from preferences user_id = ";
$query[] = $_GET['user_id'];

db_exec($query);

And the function db_exec would be changed to something like:

function db_exec(string|array $query_parts)
{
    if (is_string($query_parts) && !is_literal($query_parts)) {
        throw new \Exception("Cannot use non literal string as query.
Please pass the parts in as an array");
    } else {
        foreach ($query_parts as $query_part) {
            if (is_string($query_part) && !is_literal($query_part) {
                throw new \Exception("non-literal string found [$query_part]");
            }
            else if (is_int($query_part)) {
                // todo - decide if you want to allow this or not.
                // I personally wouldn't.
            }
            else {
                // todo - support other types
            }
        }

        $query = implode("", $query_parts);
    }

    // rest of db_exec here...
}