rfc:is_literal
Differences
This shows you the differences between two versions of the page.
Both sides previous revisionPrevious revisionNext revision | Previous revisionNext revisionBoth sides next revision | ||
rfc:is_literal [2020/03/21 17:38] – Remove edit link craigfrancis | rfc:is_literal [2021/04/18 16:55] – some suggested rewording imsop | ||
---|---|---|---|
Line 1: | Line 1: | ||
====== PHP RFC: Is Literal Check ====== | ====== PHP RFC: Is Literal Check ====== | ||
- | * Version: 0.1 | + | * Version: 0.4 |
* Date: 2020-03-21 | * Date: 2020-03-21 | ||
+ | * Updated: 2021-02-19 | ||
* Author: Craig Francis, craig# | * Author: Craig Francis, craig# | ||
* Status: Draft | * Status: Draft | ||
* First Published at: https:// | * First Published at: https:// | ||
+ | * GitHub Repo: https:// | ||
===== Introduction ===== | ===== Introduction ===== | ||
- | Add an // | + | This RFC proposes a new function, |
- | This allows developers/ | + | The clearest example is a database library which supports parametrised queries |
- | It allows commands to be tested, to ensure they are a " | ||
- | This will also allow systems/ | + | ===== Examples |
- | + | ||
- | ===== Related JavaScript Implementation | + | |
- | + | ||
- | This proposal is taking some ideas from TC39, where a similar idea is being discussed for JavaScript, to support the introduction of Trusted Types. | + | |
- | + | ||
- | https:// | + | |
- | https:// | + | |
- | + | ||
- | They are looking at " | + | |
- | + | ||
- | ===== Taint Checking ===== | + | |
- | + | ||
- | Xinchen Hui has done some amazing work with the Taint extension: | + | |
- | + | ||
- | https:// | + | |
- | + | ||
- | Unfortunately this approach does not address all issues, mainly because it still allows string escaping, which is only " | + | |
- | + | ||
- | $sql = ' | + | |
- | + | ||
- | // delete.php? | + | |
- | + | ||
- | // DELETE FROM table WHERE id = id | + | |
- | $html = '< | + | The [[https:// |
- | + | ||
- | | + | |
- | + | ||
- | | + | |
- | The Taint extension also [[https://github.com/ | + | <code php> |
+ | // INSECURE | ||
+ | $qb-> | ||
+ | | ||
+ | | ||
+ | </ | ||
- | ===== Previous RFC ===== | + | The definition of the '' |
- | Matt Tait suggested [[https:// | + | < |
+ | $qb-> | ||
+ | | ||
+ | | ||
+ | | ||
+ | </code> | ||
- | It was noted that " | + | Similarly, Twig allows |
- | Where it would have effected every SQL function, such as // | + | <code php> |
+ | // INSECURE | ||
+ | echo $twig->createTemplate('< | ||
+ | </code> | ||
- | 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:// | + | If '' |
- | I also agree that "SQL injection is almost a solved problem [by using] prepared statements" | + | <code php> |
+ | echo $twig-> | ||
+ | </ | ||
===== Proposal ===== | ===== Proposal ===== | ||
- | Add an // | + | A literal is defined as any value which is entirely under the control of the programmer. The value may be passed between functions, as long as it is not modified in any way other than string concatenation. |
- | This uses a similar definition as the [[https:// | + | < |
+ | is_literal(' | ||
- | Thanks to [[https://news-web.php.net/ | + | $a = ' |
+ | is_literal($a); | ||
- | Unlike the Taint extension, there is no need to provide an equivalent | + | is_literal(4); |
+ | is_literal(0.3); // true | ||
+ | is_literal(' | ||
- | ===== Examples ===== | + | $a = ' |
+ | $b = $a . ' B ' . 3; | ||
+ | is_literal($b); | ||
- | ==== SQL Injection, Basic ==== | + | is_literal($_GET[' |
- | A simple example: | + | is_literal(rand(0, |
- | $sql = 'SELECT * FROM table WHERE id = ?'; | + | is_literal(sprintf('LIMIT %d', |
- | + | ||
- | $result = $db-> | + | |
- | Checked in the framework by: | + | $c = count($ids); |
+ | $a = 'WHERE id IN (' . implode(',', | ||
+ | is_literal($a); | ||
+ | </ | ||
- | class db { | + | Note that there is no way to manually mark a string as " |
- | + | ||
- | public function exec($sql, $parameters = []) { | + | |
- | + | ||
- | if (!is_literal($sql)) { | + | |
- | throw new Exception(' | + | |
- | } | + | |
- | + | ||
- | $statement = $this-> | + | |
- | $statement-> | + | |
- | return $statement-> | + | |
- | + | ||
- | } | + | |
- | + | ||
- | } | + | |
- | It will also work with string concatenation: | ||
- | define(' | + | ===== Implementation Notes ===== |
- | + | ||
- | $sql = ' | + | |
- | + | ||
- | is_literal($sql); | + | |
- | + | ||
- | $sql .= ' AND id = ' . mysqli_real_escape_string($db, | + | |
- | + | ||
- | is_literal($sql); | + | |
- | ==== SQL Injection, ORDER BY ==== | + | (Most of what's in this section probably doesn' |
- | To ensure | + | Ideally string concatenation would be allowed, but [[https://github.com/Danack/RfcLiteralString/ |
- | $order_fields = [ | + | Thanks to [[https:// |
- | ' | + | |
- | ' | + | |
- | ' | + | |
- | ]; | + | |
- | + | ||
- | $order_id = array_search(($_GET[' | + | |
- | + | ||
- | $sql = ' ORDER BY ' | + | |
- | ==== SQL Injection, WHERE IN ==== | + | As an aside, [[https:// |
- | Most SQL strings can be a concatenations of literal values, but //WHERE x IN (?,?,?)// need to use a variable number of literal placeholders. | + | ===== Comparison |
- | So there //might// need to be a special case for // | + | Some languages implement a "taint flag" which tracks whether values are considered " |
- | $in_sql = implode(' | + | These solutions rely on the assumption that the output of an escaping function is safe for a particular context. This sounds reasonable in theory, but the operation of escaping functions, and the context for which their output is safe, are very hard to define. This leads to a feature that is both complex and unreliable. |
- | + | ||
- | // or | + | |
- | + | ||
- | $in_sql = substr(str_repeat('?,', | + | |
- | To be used with: | + | The current proposal avoids this complexity by addressing a different part of the problem: separating inputs supplied by the programmer from inputs supplied by the user. |
- | $sql = ' | + | ===== Previous Work ===== |
- | ==== SQL Injection, ORM Usage ==== | + | Google currently uses a [[https:// |
- | [[https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/ | + | 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/ |
- | $users = $queryBuilder | + | And there is the [[https:// |
- | -> | + | |
- | -> | + | |
- | -> | + | |
- | -> | + | |
- | -> | + | |
- | + | ||
- | | + | |
- | Where this mistake could be identified | + | * " |
+ | * this amount of work isn't ideal for "just for one use case" ([[https:// | ||
+ | * It would have effected every SQL function, such as // | ||
+ | * 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:// | ||
- | public function where($predicates) | + | I also agree that "SQL injection |
- | { | + | |
- | if (!is_literal($predicates)) { | + | |
- | throw new Exception(' | + | |
- | } | + | |
- | ... | + | |
- | } | + | |
- | + | ||
- | [[https:// | + | |
- | + | ||
- | $users = R:: | + | |
- | + | ||
- | [[http:// | + | |
- | + | ||
- | $users = UserQuery:: | + | |
- | + | ||
- | ==== SQL Injection, ORM Internal ==== | + | |
- | + | ||
- | The // | + | |
- | + | ||
- | This would avoid mistakes such as the ORDER BY issues in the Zend framework | + | |
- | + | ||
- | ==== CLI Injection ==== | + | |
- | + | ||
- | Rather than using functions such as: | + | |
- | + | ||
- | * //exec()// | + | |
- | * // | + | |
- | * // | + | |
- | * // | + | |
- | + | ||
- | Frameworks (or PHP) could introduce something similar to // | + | |
- | + | ||
- | Or, take a verified literal for the command, and use parameters for the arguments (like SQL): | + | |
- | + | ||
- | $output = parameterised_exec(' | + | |
- | ' | + | |
- | ]); | + | |
- | + | ||
- | Rough implementation: | + | |
- | + | ||
- | function parameterised_exec($cmd, | + | |
- | + | ||
- | if (!is_literal($cmd)) { | + | |
- | throw new Exception(' | + | |
- | } | + | |
- | + | ||
- | $offset = 0; | + | |
- | $k = 0; | + | |
- | while (($pos = strpos($cmd, | + | |
- | if (!isset($args[$k])) { | + | |
- | throw new Exception(' | + | |
- | exit(); | + | |
- | } | + | |
- | $arg = escapeshellarg($args[$k]); | + | |
- | $cmd = substr($cmd, | + | |
- | $offset = ($pos + strlen($arg)); | + | |
- | $k++; | + | |
- | } | + | |
- | if (isset($args[$k])) { | + | |
- | throw new Exception(' | + | |
- | 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(' | + | |
- | + | ||
- | But small snippets of HTML are often easier to define as a literal within the PHP script: | + | |
- | + | ||
- | $template_html = ' | + | |
- | < | + | |
- | < | + | |
- | + | ||
- | Where the variables are supplied separately, in this example I'm using XPaths: | + | |
- | + | ||
- | $values = [ | + | |
- | '// | + | |
- | NULL => ' | + | |
- | ' | + | |
- | ' | + | |
- | ], | + | |
- | '// | + | |
- | ' | + | |
- | ], | + | |
- | ]; | + | |
- | + | ||
- | echo template_parse($template_html, | + | |
- | + | ||
- | 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, | + | |
- | + | ||
- | if (!is_literal($html)) { | + | |
- | throw new Exception(' | + | |
- | } | + | |
- | + | ||
- | $dom = new DomDocument(); | + | |
- | $dom-> | + | |
- | + | ||
- | $xpath = new DOMXPath($dom); | + | |
- | + | ||
- | foreach ($values as $query => $attributes) { | + | |
- | + | ||
- | if (!is_literal($query)) { | + | |
- | throw new Exception(' | + | |
- | } | + | |
- | + | ||
- | foreach ($xpath-> | + | |
- | foreach ($attributes as $attribute => $value) { | + | |
- | + | ||
- | if (!is_literal($attribute)) { | + | |
- | throw new Exception(' | + | |
- | } | + | |
- | + | ||
- | if ($attribute) { | + | |
- | $safe = false; | + | |
- | if ($attribute == ' | + | |
- | if (preg_match('/ | + | |
- | $safe = true; // Not " | + | |
- | } | + | |
- | } else if ($attribute == ' | + | |
- | if (in_array($value, | + | |
- | $safe = true; // Only allow specific classes? | + | |
- | } | + | |
- | } else if (preg_match('/ | + | |
- | if (preg_match('/ | + | |
- | $safe = true; | + | |
- | } | + | |
- | } | + | |
- | if ($safe) { | + | |
- | $element-> | + | |
- | } | + | |
- | } else { | + | |
- | $element-> | + | |
- | } | + | |
- | + | ||
- | } | + | |
- | } | + | |
- | + | ||
- | } | + | |
- | + | ||
- | $html = ''; | + | |
- | $body = $dom-> | + | |
- | if ($body-> | + | |
- | foreach ($body-> | + | |
- | $html .= $dom-> | + | |
- | } | + | |
- | } | + | |
- | + | ||
- | return $html; | + | |
- | + | ||
- | } | + | |
===== Backward Incompatible Changes ===== | ===== Backward Incompatible Changes ===== | ||
- | Not sure | + | None |
===== Proposed PHP Version(s) ===== | ===== Proposed PHP Version(s) ===== | ||
- | PHP 8? | + | PHP 8.1? |
===== RFC Impact ===== | ===== RFC Impact ===== | ||
Line 341: | Line 138: | ||
===== Open Issues ===== | ===== Open Issues ===== | ||
- | | + | On [[https:// |
- | - Systems/ | + | |
+ | - Name it something else? [[https:// | ||
+ | - Would this cause performance issues? | ||
+ | - Can //array_fill()//+//implode()// pass though the " | ||
+ | - Systems/ | ||
===== Unaffected PHP Functionality ===== | ===== Unaffected PHP Functionality ===== | ||
Line 350: | Line 151: | ||
===== Future Scope ===== | ===== Future Scope ===== | ||
- | Certain | + | As noted by [[https:// |
+ | |||
+ | **Phase 2** could introduce a way for programmers | ||
+ | |||
+ | For example, | ||
+ | |||
+ | **Phase 3** could set a default of 'only literals' | ||
+ | |||
+ | And, for a bit of silliness (Spaß ist verboten), there could be a // | ||
===== Proposed Voting Choices ===== | ===== Proposed Voting Choices ===== | ||
- | Not sure | + | N/A |
===== Patches and Tests ===== | ===== Patches and Tests ===== | ||
- | A volunteer is needed to help with implementation. | + | N/A |
===== Implementation ===== | ===== Implementation ===== | ||
- | N/A | + | [[https:// |
===== References ===== | ===== References ===== | ||
- | - https:// | + | N/A |
===== Rejected Features ===== | ===== Rejected Features ===== |
rfc/is_literal.txt · Last modified: 2022/02/14 00:36 by craigfrancis