Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:106631
MIME-Version: 1.0
References: <CAFv4g+EG6f9e6yJqp1sesbJstYJwpdkVso3CKjAD_EAkcrtHqA@mail.gmail.com>
 <CAKuGYLQLEkWhCapqc368jjbi7oGZLjqf6uRkXXWJ5fadLtxc-g@mail.gmail.com>
 <CAFv4g+EvygJyv0mF4OSBb7iJWrFCkoE800JZtgt-AbnWn+9thQ@mail.gmail.com> <CAKuGYLQQi9SVxpV4SNJxRuUULd4WDXEEMTuX2a3fF2DcKCKMOg@mail.gmail.com>
In-Reply-To: <CAKuGYLQQi9SVxpV4SNJxRuUULd4WDXEEMTuX2a3fF2DcKCKMOg@mail.gmail.com>
Date: Thu, 15 Aug 2019 23:21:24 +0100
Message-ID: <CAFv4g+F9DvCXMDn3xAiAjKHSM1hgf0sNjHtBDiUUvkUDBC2-8w@mail.gmail.com>
To: Matthew Brown <matthewmatthew@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000f3f54605902f4b05"
Subject: Re: [PHP-DEV] Literal / Taint checking
From: craig@craigfrancis.co.uk (Craig Francis)

--000000000000f3f54605902f4b05
Content-Type: text/plain; charset="UTF-8"

On Thu, 15 Aug 2019 at 21:37, Matthew Brown <matthewmatthew@gmail.com>
wrote:

>
> If anything, this proposal would help user-land solutions (it gives them
>> more information while the code is in running).
>>
>
> Well, it might help runtime-based user-land solutions, but not static
> analysis-based solutions.
>


I mostly see us needing to use both solutions - static analysis does a deep
dive (ideally helped with any information the PHP engine can provide, even
if it's just parsing), and this runtime check running constantly - only
because static analysis by itself can skip bits, e.g.

https://gerrit.wikimedia.org/g/mediawiki/tools/phan/SecurityCheckPlugin/#general-limitations



> In our bug disclosure program at Vimeo we've had no SQL injection issues
> reported, but a number of XSS issues (echoing attacker-controlled data),
> and those issues cannot so easily be prevented by this technique as there's
> generally little reason to echo literal values.
>


This proposal can ensure SQL injection is impossible, rather than our
current processes, which has some gaps (I'm glad to see you haven't had any
reported issues, but I believe you're in the minority).

This can also be expanded for command line injection issues (kind of moving
away from escapeshellarg).

I've not spent enough time on the templating side of things yet (I've been
working more on the browser side for this, e.g. CSP and
application/xhtml+xml).

But I'm hopeful this proposal can still be useful, in a similar way to how
the JavaScript changes will help templating (ref Trusted Types).

Even if we only use this for guarding some inputs - e.g. a templating
system being sure which bits are safe HTML literals, loaded into a
DomDocument, and unsafe user data being applied with setAttribute() after
some sanity checks.

But there are some annoying edge cases, which means that I don't think this
can be perfect:

  $user_homepage = 'javascript:alert(document.cookie)';

  <a href="<?= htmlentities($user_homepage) ?>">Example</a>



> I can also think of a number of user-constructed SQL queries (e.g. WHERE
> ... IN) that require non-literal values to work (if this were to come to
> pass there might be a set of special `unsafe` methods).
>


This is what I've been using for `WHERE ... IN` to create a literal for the
SQL string:

  $parameters = [];

  $in_sql = $db->*parameter_in*($parameters, 'i', $ids); // I'm using
`maxdb_stmt::bind_param` which needs a type.

  $sql = 'SELECT * FROM table WHERE id IN (' . $in_sql . ')';

  $db->fetch_all($sql, $parameters)

  ...

  public function *parameter_in*(&$parameters, $type, $values) {
    $count = count($values);
    if ($count == 0) {
      throw new Exception('At least 1 value is required for an IN list');
    }
    if ($type == 'i') {
      $values = array_map('intval', $values);
    } else if ($type == 's') {
      $values = array_map('strval', $values);
    } else {
      throw new Exception('Unknown parameter type for parameter_in(),
should be "i" or "s"');
    }
    foreach ($values as $value) {
      $parameters[] =  [$type, $value];
    }
    return substr(str_repeat('?,', $count), 0, -1); // Returns a literal
string.
  }

--000000000000f3f54605902f4b05--