Differences

This shows you the differences between two versions of the page.

--- rfc:is_literal [2020/03/21 17:38] – created craigfrancis
+++ rfc:is_literal [2021/04/11 11:37] – Example updates craigfrancis
@@ Line 1: / Line 1: @@
-https://wiki.php.net/rfc/is_literal?do=edit
 ====== PHP RFC: Is Literal Check ======
-  * Version: 0.1
+  * Version: 0.3
   * Date: 2020-03-21
+  * Updated: 2021-02-19
   * Author: Craig Francis, craig#at#craigfrancis.co.uk
   * Status: Draft
   * First Published at: https://wiki.php.net/rfc/is_literal
+  * GitHub Repo: https://github.com/craigfrancis/php-is-literal-rfc
 ===== Introduction =====
-Add an //is_literal()// function, so developers/frameworks can be sure they are working with a safe value - one created from one or more literals, defined within PHP scripts.
+Add an //is_literal()// function, so developers/frameworks can check if a given variable is **safe**.
-This allows developers/frameworks, at runtime, to warn or block SQL Injection, Command Line Injection, and many cases of HTML Injection.
+As in, at runtime, being able to check if a variable has been created by literals, defined within a PHP script, by a trusted developer.
-It allows commands to be tested, to ensure they are a "programmer supplied constant/static/validated string", and all other unsafe variables are provided separately (as noted by [[https://news-web.php.net/php.internals/87725|Yasuo Ohgaki]]).
+This check can be used to warn or completely block (by default) many, if not all, injection based vulnerabilities.
-This will also allow systems/frameworks to decide if they want to **block**, **educate** (via a notice), or **ignore** these issues (to avoid the "don't nanny" concern raised by [[https://news-web.php.net/php.internals/87383|Lester Caine]]).
+See the [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md|justification for why this is important]].
-===== Related JavaScript Implementation =====
+In short, abstractions like Doctrine could identify [[https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/security.html|common mistakes]], like this [[https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/query-builder.html#high-level-api-methods|Query Builder]] SQL Injection vulnerability:
-This proposal is taking some ideas from TC39, where a similar idea is being discussed for JavaScript, to support the introduction of Trusted Types.
+<code php>
+$users = $queryBuilder
+  ->select('u')
+  ->from('User', 'u')
+  ->where('u.id = ' . $_GET['id'])
+  ->getQuery()
+  ->getResult();
+</code>
-https://github.com/tc39/proposal-array-is-template-object\\
+Or Twig could project against HTML Injection vulnerabilities (aka XSS), like this flawed [[https://twig.symfony.com/doc/2.x/recipes.html#loading-a-template-from-a-string|HTML Template]]:
-https://github.com/mikewest/tc39-proposal-literals
-They are looking at "Distinguishing strings from a trusted developer, from strings that may be attacker controlled".
+<code php>
+echo $twig->createTemplate('<p>Hi ' . $_GET['name'] . '</p>')->render();
-===== Taint Checking =====
+</code>
-Xinchen Hui has done some amazing work with the Taint extension:
-https://github.com/laruence/taint
-Unfortunately this approach does not address all issues, mainly because it still allows string escaping, which is only "[[https://www.php.net/manual/en/pdo.quote.php|Theoretically Safe]]" (typically due to character encoding issues), nor does it address issues such as missing quotes:
-  $sql = 'DELETE FROM table WHERE id = ' . mysqli_real_escape_string($db, $_GET['id']);
-  // delete.php?id=id
-  // DELETE FROM table WHERE id = id
-  $html = '<img src=' . htmlentities($_GET['url']) . ' />';
-  // example.php?url=x%20onerror=alert(cookie)
-  // <img src=x onerror=alert(cookie) />
-The Taint extension also [[https://github.com/laruence/taint/blob/4a6c4cb2613e27f5604d2021802c144a954caff8/taint.c#L63|conflicts with XDebug]] (sorry Derick),
-===== Previous RFC =====
-Matt Tait suggested [[https://wiki.php.net/rfc/sql_injection_protection||Automatic SQL Injection Protection]].
-It was noted that "unfiltered input can affect way more than only SQL" ([[https://news-web.php.net/php.internals/87355|Pierre Joye]]), and this amount of work isn't ideal for "just for one use case" ([[https://news-web.php.net/php.internals/87647|Julien Pauli]]).
-Where it would have effected every SQL function, such as //mysqli_query()//, //$pdo->query()//, //odbc_exec()//, etc (concerns raised by [[https://news-web.php.net/php.internals/87436|Lester Caine]] and [[https://news-web.php.net/php.internals/87650|Anthony Ferrara]]).
-And each of those functions would need a bypass for cases where unsafe SQL was intentionally being used (e.g. phpMyAdmin taking SQL from POST data) because some applications intentionally "pass raw, user submitted, SQL" (Ronald Chmara [[https://news-web.php.net/php.internals/87406|1]]/[[https://news-web.php.net/php.internals/87446|2]]).
-I also agree that "SQL injection is almost a solved problem [by using] prepared statements" ([[https://news-web.php.net/php.internals/87400|Scott Arciszewski]]), but we do need something that can identify mistakes, ideally at runtime.
 ===== Proposal =====
-Add an //is_literal()// function to check if a given variable has only been created by Literal(s).
+Literals are safe values, defined within the PHP script, for example:
-This uses a similar definition as the [[https://wiki.php.net/rfc/sql_injection_protection#safeconst|SafeConst]] by Matt Tait, but it does not need to accept Integer or FloatingPoint variables as safe (unless it makes the implementation easier), nor should it effect any existing functions.
+<code php>
+is_literal('Example'); // true
-Thanks to [[https://news-web.php.net/php.internals/87396|Xinchen Hui]], we know the PHP5 Taint extension was complex, but "with PHP7's new zend_string, and string flags, the implementation will become easier".
+$a = 'Example';
+is_literal($a); // true
-Unlike the Taint extension, there is no need to provide an equivalent //untaint()// function.
+is_literal(4); // true
+is_literal(0.3); // true
+is_literal('a' . 'b'); // true, compiler can concatenate
-===== Examples =====
+$a = 'A';
+$b = $a . ' B ' . 3;
+is_literal($b); // true, ideally (more details below)
-==== SQL Injection, Basic ====
+is_literal($_GET['id']); // false
-A simple example:
+is_literal(rand(0, 10)); // false
-  $sql = 'SELECT * FROM table WHERE id = ?';
+is_literal(sprintf('LIMIT %d', 3)); // false
-  $result = $db->exec($sql, [$id]);
-Checked in the framework by:
+$c = count($ids);
+$a = 'WHERE id IN (' . implode(',', array_fill(0, $c, '?')) . ')';
+is_literal($a); // true, the one exception that involves functions.
+</code>
-  class db {
+Ideally string concatenation would be allowed, but [[https://github.com/Danack/RfcLiteralString/issues/5|Danack]] suggested this might raise performance concerns, and an array implode like function could be used instead (or a query builder).
-    public function exec($sql, $parameters = []) {
-      if (!is_literal($sql)) {
-        throw new Exception('SQL must be a literal.');
-      }
-      $statement = $this->pdo->prepare($sql);
-      $statement->execute($parameters);
-      return $statement->fetchAll();
-    }
-  }
-It will also work with string concatenation:
+Thanks to [[https://chat.stackoverflow.com/transcript/message/51565346#51565346|NikiC]], it looks like we can reuse the GC_PROTECTED flag for strings.
-  define('TABLE', 'example');
+As an aside, [[https://news-web.php.net/php.internals/87396|Xinchen Hui]] found the Taint extension was complex in PHP5, but "with PHP7's new zend_string, and string flags, the implementation will become easier". Also, [[https://chat.stackoverflow.com/transcript/message/48927813#48927813|MarkR]] suggested that it might be possible to use the fact that "interned strings in PHP have a flag", which is there because these "can't be freed".
-  $sql = 'SELECT * FROM ' . TABLE . ' WHERE id = ?';
-    is_literal($sql); // Returns true
-  $sql .= ' AND id = ' . mysqli_real_escape_string($db, $_GET['id']);
-    is_literal($sql); // Returns false
-==== SQL Injection, ORDER BY ====
+Unlike the Taint extension, there must **not** be an equivalent //untaint()// function, or support any kind of escaping.
-To ensure //ORDER BY// can be set via the user, but only use acceptable values:
+===== Previous Work =====
-  $order_fields = [
+There is the [[https://github.com/laruence/taint|Taint extension]] by Xinchen Hui, but in contrast to this, there must **not** be an equivalent //untaint()// function, or support any kind of escaping (see the [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md|justification page]]).
-      'name',
-      'created',
-      'admin',
-    ];
-  $order_id = array_search(($_GET['sort'] ?? NULL), $order_fields);
-  $sql = ' ORDER BY ' . $order_fields[$order_id];
-==== SQL Injection, WHERE IN ====
+Google currently uses a [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md#go-implementation|similar approach in Go]] which uses "compile time constants", [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md#perl-implementation|Perl has a Taint Mode]] (but uses regular expressions to un-taint data), and there are discussions about [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md#javascript-implementation|adding it to JavaScript]] to support Trusted Types.
-Most SQL strings can be a concatenations of literal values, but //WHERE x IN (?,?,?)// need to use a variable number of literal placeholders.
+As noted be [[https://news-web.php.net/php.internals/109192|Tyson Andre]], it might be possible to use static analysis, for example [[https://psalm.dev/|psalm]]. But I can't find any which do these checks by default, [[https://github.com/vimeo/psalm/commit/2122e4a1756dac68a83ec3f5abfbc60331630781|can be incomplete]], they are likely to miss things (especially at runtime), and we can't expect all programmers to use static analysis (especially those who are new to programming, who need this more than developers who know the concepts and just make the odd mistake).
-So there //might// need to be a special case for //array_fill()//+//implode()// or //str_repeat()//+//substr()// to create something like '?,?,?'
+And there is the [[https://wiki.php.net/rfc/sql_injection_protection|Automatic SQL Injection Protection]] RFC by Matt Tait, where this RFC uses a similar concept of the [[https://wiki.php.net/rfc/sql_injection_protection#safeconst|SafeConst]]. When Matt's RFC was being discussed, it was noted:
-  $in_sql = implode(',', array_fill(0, count($ids), '?'));
+  * "unfiltered input can affect way more than only SQL" ([[https://news-web.php.net/php.internals/87355|Pierre Joye]]);
+  * this amount of work isn't ideal for "just for one use case" ([[https://news-web.php.net/php.internals/87647|Julien Pauli]]);
-  // or
+  * It would have effected every SQL function, such as //mysqli_query()//, //$pdo->query()//, //odbc_exec()//, etc (concerns raised by [[https://news-web.php.net/php.internals/87436|Lester Caine]] and [[https://news-web.php.net/php.internals/87650|Anthony Ferrara]]);
+  * Each of those functions would need a bypass for cases where unsafe SQL was intentionally being used (e.g. phpMyAdmin taking SQL from POST data) because some applications intentionally "pass raw, user submitted, SQL" (Ronald Chmara [[https://news-web.php.net/php.internals/87406|1]]/[[https://news-web.php.net/php.internals/87446|2]]).
-  $in_sql = substr(str_repeat('?,', count($ids)), 0, -1);
-To be used with:
+I also agree that "SQL injection is almost a solved problem [by using] prepared statements" ([[https://news-web.php.net/php.internals/87400|Scott Arciszewski]]), but we still //is_literal()// to identify mistakes.
-  $sql = 'SELECT * FROM table WHERE id IN (' . $in_sql . ')';
-==== SQL Injection, ORM Usage ====
-[[https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/query-builder.html#high-level-api-methods|Doctrine]] could use this to ensure //$predicates// is a literal:
-  $users = $queryBuilder
-    ->select('u')
-    ->from('User', 'u')
-    ->where('u.id = ' . $_GET['id'])
-    ->getQuery()
-    ->getResult();
-  // example.php?id=u.id
-Where this mistake could be identified by:
-  public function where($predicates)
-  {
-      if (!is_literal($predicates)) {
-          throw new Exception('Can only accept a literal');
-      }
-      ...
-  }
-[[https://redbeanphp.com/index.php?p=/finding|RedBean]] could check //$sql// is a literal:
-  $users = R::find('user', 'id = ' . $_GET['id']);
-[[http://propelorm.org/Propel/reference/model-criteria.html#relational-api|PropelORM]] could check //$clause// is a literal:
-  $users = UserQuery::create()->where('id = ' . $_GET['id'])->find();
-==== SQL Injection, ORM Internal ====
-The //is_literal()// function could be used by ORM developers, so they can be sure they have created an SQL string out of literals.
-This would avoid mistakes such as the ORDER BY issues in the Zend framework [[https://framework.zend.com/security/advisory/ZF2014-04|1]]/[[https://framework.zend.com/security/advisory/ZF2016-03|2]].
-==== CLI Injection ====
-Rather than using functions such as:
-  * //exec()//
-  * //shell_exec()//
-  * //system()//
-  * //passthru()//
-Frameworks (or PHP) could introduce something similar to //pcntl_exec()//, where arguments are provided separately.
-Or, take a verified literal for the command, and use parameters for the arguments (like SQL):
-  $output = parameterised_exec('grep ? /path/to/file | wc -l', [
-      'example',
-    ]);
-Rough implementation:
-  function parameterised_exec($cmd, $args = []) {
-    if (!is_literal($cmd)) {
-      throw new Exception('The first argument must be a literal');
-    }
-    $offset = 0;
-    $k = 0;
-    while (($pos = strpos($cmd, '?', $offset)) !== false) {
-      if (!isset($args[$k])) {
-        throw new Exception('Missing parameter "' . ($k + 1) . '"');
-        exit();
-      }
-      $arg = escapeshellarg($args[$k]);
-      $cmd = substr($cmd, 0, $pos) . $arg . substr($cmd, ($pos + 1));
-      $offset = ($pos + strlen($arg));
-      $k++;
-    }
-    if (isset($args[$k])) {
-      throw new Exception('Unused parameter "' . ($k + 1) . '"');
-      exit();
-    }
-    return exec($cmd);
-  }
-==== HTML Injection ====
-Template engines should receive variables separately from the raw HTML.
-Often the engine will get the HTML from static files:
-  $html = file_get_contents('/path/to/template.html');
-But small snippets of HTML are often easier to define as a literal within the PHP script:
-  $template_html = '
-    <p>Hello <span id="username"></span></p>
-    <p><a>Website</a></p>';
-Where the variables are supplied separately, in this example I'm using XPaths:
-  $values = [
-      '//span[@id="username"]' => [
-          NULL      => 'Name', // The textContent
-          'class'   => 'admin',
-          'data-id' => '123',
-        ],
-      '//a' => [
-          'href' => 'https://example.com',
-        ],
-    ];
-  echo template_parse($template_html, $values);
-Being sure the HTML does not contain unsafe variables, the templating engine can accept and apply the supplied variables for the relevant context, for example:
-  function template_parse($html, $values) {
-    if (!is_literal($html)) {
-      throw new Exception('Invalid Template HTML.');
-    }
-    $dom = new DomDocument();
-    $dom->loadHTML('<?xml encoding="UTF-8">' . $html);
-    $xpath = new DOMXPath($dom);
-    foreach ($values as $query => $attributes) {
-      if (!is_literal($query)) {
-        throw new Exception('Invalid Template XPath.');
-      }
-      foreach ($xpath->query($query) as $element) {
-        foreach ($attributes as $attribute => $value) {
-          if (!is_literal($attribute)) {
-            throw new Exception('Invalid Template Attribute.');
-          }
-          if ($attribute) {
-            $safe = false;
-            if ($attribute == 'href') {
-              if (preg_match('/^https?:\/\//', $value)) {
-                $safe = true; // Not "javascript:..."
-              }
-            } else if ($attribute == 'class') {
-              if (in_array($value, ['admin', 'important'])) {
-                $safe = true; // Only allow specific classes?
-              }
-            } else if (preg_match('/^data-[a-z]+$/', $attribute)) {
-              if (preg_match('/^[a-z0-9 ]+$/i', $value)) {
-                $safe = true;
-              }
-            }
-            if ($safe) {
-              $element->setAttribute($attribute, $value);
-            }
-          } else {
-            $element->textContent = $value;
-          }
-        }
-      }
-    }
-    $html = '';
-    $body = $dom->documentElement->firstChild;
-    if ($body->hasChildNodes()) {
-      foreach ($body->childNodes as $node) {
-        $html .= $dom->saveXML($node);
-      }
-    }
-    return $html;
-  }
 ===== Backward Incompatible Changes =====
-Not sure
+None
 ===== Proposed PHP Version(s) =====
-PHP 8?
+PHP 8.1?
 ===== RFC Impact =====
@@ Line 343: / Line 114: @@
 ===== Open Issues =====
-  - Can //array_fill()//+//implode()// or //str_repeat()//+//substr()// pass though the "is_literal" flag for the "WHERE IN" case?
+On [[https://github.com/craigfrancis/php-is-literal-rfc/issues|GitHub]]:
-  - Systems/Frameworks that define certain variables (e.g. table name prefixes) without the use of a literal (e.g. ini/json/yaml files), won't be able to use this check, as originally noted by [[https://news-web.php.net/php.internals/87667|Dennis Birkholz]].
+  - Name it something else? [[https://news-web.php.net/php.internals/109197|Jakob Givoni]] suggested //is_from_literal()//; or maybe //is_safe()//.
+  - Would this cause performance issues?
+  - Can //array_fill()//+//implode()// pass though the "is_literal" flag for the "WHERE IN" case?
+  - Systems/Frameworks that define certain variables (e.g. table name prefixes) without the use of a literal (e.g. ini/json/yaml files), they might need to make some changes to use this check, as originally noted by [[https://news-web.php.net/php.internals/87667|Dennis Birkholz]].
 ===== Unaffected PHP Functionality =====
@@ Line 352: / Line 127: @@
 ===== Future Scope =====
-Certain functions (mysqli_query, preg_match, etc) might use this information to generate a error/warning/notice.
+As noted by [[https://chat.stackoverflow.com/transcript/message/51573226#51573226|MarkR]], the biggest benefit will come when it can be used by PDO and similar functions (//mysqli_query//, //preg_match//, //exec//, etc). But the basic idea can be used immediately by frameworks and general abstraction libraries, and they can give feedback for future work.
+**Phase 2** could introduce a way for programmers to specify that certain function arguments only accept safe literals, and/or specific value-objects their project trusts (this idea comes from [[https://web.dev/trusted-types/|Trusted Types]] in JavaScript).
+For example, a project could require the second argument for //pg_query()// only accept literals or their //query_builder// object (which provides a //__toString// method); and that any output (print, echo, readfile, etc) must use the //html_output// object that's returned by their trusted HTML Templating system (using //ob_start()// might be useful here).
+**Phase 3** could set a default of 'only literals' for all of the relevant PHP function arguments, so developers are given a warning, and later prevented (via an exception), when they provide an unsafe value to those functions (they could still specify that unsafe values are allowed, e.g. phpMyAdmin).
+And, for a bit of silliness (Spaß ist verboten), there could be a //is_figurative()// function, which MarkR seems to [[https://chat.stackoverflow.com/transcript/message/48927770#48927770|really]], [[https://chat.stackoverflow.com/transcript/message/51573091#51573091|want]] :-)
 ===== Proposed Voting Choices =====
-Not sure
+N/A
 ===== Patches and Tests =====
-A volunteer is needed to help with implementation.
+N/A
 ===== Implementation =====
-N/A
+[[https://github.com/Danack/|Danack]] has [[https://github.com/php/php-src/compare/master...Danack:is_literal_attempt_two|started an implementation]].
 ===== References =====
-- https://wiki.php.net/rfc/sql_injection_protection
+N/A
 ===== Rejected Features =====

Differences

Page Tools