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/23 17:44] – Add a note about "interned strings" 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 function | + | The clearest example is a database library which supports parametrised queries at the driver level. The correct usage would be something like '' |
- | Commands can then be tested to ensure they are a " | ||
- | This will also allow systems/ | + | ===== Examples |
- | + | ||
- | Literals are values defined within the PHP scripts, for example: | + | |
- | + | ||
- | $a = ' | + | |
- | $b = ' | + | |
- | is_literal($b); | + | |
- | + | ||
- | $c = ' | + | |
- | is_literal($c); | + | |
- | + | ||
- | ===== 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); | ||
- | And thanks to [[https://chat.stackoverflow.com/transcript/message/ | + | is_literal(4); |
+ | is_literal(0.3); // true | ||
+ | is_literal(' | ||
- | Unlike the Taint extension, there is no need to provide an equivalent //untaint()// function. | + | $a = ' |
+ | $b = $a . ' B ' . 3; | ||
+ | is_literal($b); // true, ideally (more details below) | ||
- | ===== Examples ===== | + | is_literal($_GET[' |
- | ==== SQL Injection, Basic ==== | + | is_literal(rand(0, 10)); // false |
- | A simple example: | + | is_literal(sprintf(' |
- | | + | $c = count($ids); |
- | + | $a = 'WHERE id IN (' . implode(',', | |
- | $result = $db-> | + | is_literal($a); // true, the one exception that involves functions. |
+ | </ | ||
- | Checked in the framework by: | + | Note that there is no way to manually mark a string as " |
- | class db { | ||
- | | ||
- | public function exec($sql, $parameters = []) { | ||
- | | ||
- | if (!is_literal($sql)) { | ||
- | throw new Exception(' | ||
- | } | ||
- | | ||
- | $statement = $this-> | ||
- | $statement-> | ||
- | return $statement-> | ||
- | | ||
- | } | ||
- | | ||
- | } | ||
- | It will also work with string concatenation: | + | ===== Implementation Notes ===== |
- | define('TABLE', | + | (Most of what's in this section probably doesn't need to be in the final RFC.) |
- | + | ||
- | $sql = ' | + | |
- | + | ||
- | is_literal($sql); | + | |
- | + | ||
- | $sql .= ' AND id = ' . mysqli_real_escape_string($db, | + | |
- | + | ||
- | is_literal($sql); // Returns false | + | |
- | ==== SQL Injection, ORDER BY ==== | + | Ideally string concatenation would be allowed, but [[https:// |
- | To ensure | + | Thanks to [[https://chat.stackoverflow.com/transcript/message/ |
- | $order_fields = [ | + | As an aside, |
- | ' | + | |
- | ' | + | |
- | ' | + | |
- | ]; | + | |
- | + | ||
- | $order_id = array_search(($_GET[' | + | |
- | + | ||
- | $sql = ' ORDER BY ' . $order_fields[$order_id]; | + | |
- | ==== SQL Injection, WHERE IN ==== | + | ===== Comparison to Taint Tracking ===== |
- | Most SQL strings can be a concatenations of literal | + | Some languages implement |
- | So there //might// need to be a special case for // | + | 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. |
- | $in_sql = implode(',', | + | 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. |
- | + | ||
- | // or | + | |
- | + | ||
- | $in_sql = substr(str_repeat('?,', | + | |
- | To be used with: | + | ===== Previous Work ===== |
- | $sql = ' | + | Google currently uses a [[https:// |
- | ==== SQL Injection, ORM Usage ==== | + | As noted be [[https:// |
- | [[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 // | + | 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: |
- | | + | |
- | ->select(' | + | * this amount of work isn't ideal for "just for one use case" ([[https:// |
- | | + | * It would have effected every SQL function, such as // |
- | | + | |
- | | + | |
- | | + | |
- | | + | |
- | | + | |
- | Where this mistake could be identified by: | + | I also agree that "SQL injection |
- | + | ||
- | public function where($predicates) | + | |
- | { | + | |
- | 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 352: | Line 138: | ||
===== Open Issues ===== | ===== Open Issues ===== | ||
+ | On [[https:// | ||
+ | |||
+ | - Name it something else? [[https:// | ||
- Would this cause performance issues? | - Would this cause performance issues? | ||
- | - Can // | + | - Can // |
- | - Should the function be named // | + | - Systems/ |
- | - Systems/ | + | |
- | + | ||
- | ===== Alternatives ===== | + | |
- | + | ||
- | - The current Taint Extension (notes above) | + | |
- | - Using static analysis (not runtime), for example [[https:// | + | |
===== Unaffected PHP Functionality ===== | ===== Unaffected PHP Functionality ===== | ||
Line 368: | 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