Differences

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

--- rfc:is_literal [2021/02/19 19:36] – Moving justification and examples to GitHub craigfrancis
+++ rfc:is_literal [2021/04/19 13:44] – Updated examples, and general tweaks craigfrancis
@@ Line 1: / Line 1: @@
 ====== PHP RFC: Is Literal Check ======
-  * Version: 0.3
+  * Version: 0.5
   * Date: 2020-03-21
-  * Updated: 2021-02-19
+  * Updated: 2021-04-19
   * Author: Craig Francis, craig#at#craigfrancis.co.uk
   * Status: Draft
@@ Line 11: / Line 11: @@
 ===== Introduction =====
-Add an //is_literal()// function, so developers/frameworks can check if a given variable is **safe**.
+This RFC proposes a new function, //is_literal(string $string)//, to help enforce a separation of hard-coded values, from user-supplied data.
-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.
+This addresses some of the same use cases as "taint flags", but is both simpler and stricter. It does not address how user data is transmitted or escaped, only whether it has been passed to a particular library function separately from the programmer defined values.
-This simple check can be used to warn or completely block SQL Injection, Command Line Injection, and many cases of HTML Injection (aka XSS).
+The clearest example is a database library which supports parametrised queries at the driver level, where the programmer could use either of these:
-See the [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md|justification for why this is important]].
+<code php>
+$db->query('SELECT * FROM users WHERE id = ' . $_GET['id']); // INSECURE
-But, in short, abstractions like [[https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/security.html|Doctrine could protect itself against common mistakes]] like this:
+$db->query('SELECT * FROM users WHERE id = ?', [$_GET['id']]);
+</code>
+By rejecting the SQL that was not written as a literal (first example), the library can provide protection against this incorrect use.
+===== Examples =====
+The [[https://www.doctrine-project.org/projects/doctrine-orm/en/current/reference/query-builder.html#high-level-api-methods|Doctrine Query Builder]] allows custom WHERE clauses to be provided as strings. This is intended for use with literals and placeholders, but does not protect against this simple mistake:
 <code php>
-$query = $em->createQuery('SELECT u FROM User u WHERE u.id = ' . $_GET['id']);
+// INSECURE
+$qb->select('u')
+   ->from('User', 'u')
+   ->where('u.id = ' . $_GET['id'])
+</code>
+The definition of the //where()// method could check with //is_literal()// and throw an exception advising the programmer to replace it with a safer use of placeholders:
+<code php>
+$qb->select('u')
+   ->from('User', 'u')
+   ->where('u.id = :identifier')
+   ->setParameter('identifier', $_GET['id']);
+</code>
+Similarly, Twig allows [[https://twig.symfony.com/doc/2.x/recipes.html#loading-a-template-from-a-string|loading a template from a string]], which could allow accidentally skipping the default escaping functionality:
+<code php>
+// INSECURE
+echo $twig->createTemplate('<p>Hi ' . $_GET['name'] . '</p>')->render();
+</code>
+If //createTemplate()// checked with //is_literal()//, the programmer could be advised to write this instead:
+<code php>
+echo $twig->createTemplate('<p>Hi {{ name }}</p>')->render(['name' => $_GET['name']]);
 </code>
 ===== Proposal =====
-Literals are safe values, defined within the PHP script, for example:
+A literal is defined as a value (string) which has been written by the programmer. The value may be passed between functions, as long as it is not modified in any way other than string concatenation.
 <code php>
+is_literal('Example'); // true
 $a = 'Example';
 is_literal($a); // true
-$a = 'Example ' . $a . ', ' . 5;
+is_literal(4); // true
-is_literal($a); // true
+is_literal(0.3); // true
+is_literal('a' . 'b'); // true, compiler can concatenate
-$a = 'Example ' . $_GET['id'];
-is_literal($a); // false
-$a = 'Example ' . time();
+$a = 'A';
-is_literal($a); // false
+$b = $a . ' B ' . 3;
+is_literal($b); // true, ideally (more details below)
-$a = sprintf('LIMIT %d', 3);
+is_literal($_GET['id']); // false
-is_literal($a); // false
-$c = count($ids);
+is_literal(rand(0, 10)); // false
-$a = 'WHERE id IN (' . implode(',', array_fill(0, $c, '?')) . ')';
-is_literal($a); // true, the odd one that involves functions.
-$limit = 10;
+is_literal(sprintf('LIMIT %d', 3)); // false, should use parameters
-$a = 'LIMIT ' . ($limit + 1);
-is_literal($a); // false, but might need some discussion.
 </code>
-This uses a similar definition of [[https://wiki.php.net/rfc/sql_injection_protection#safeconst|SafeConst]] from Matt Tait's RFC, but it doesn't need to accept Integer or FloatingPoint variables as safe (unless it makes the implementation easier), nor should this proposal effect any existing functions.
+Note that there is no way to manually mark a string as a literal (i.e. no equivalent to //untaint()//); as soon as the value has been manipulated in any way, it is no longer marked as a literal.
-Thanks to [[https://chat.stackoverflow.com/transcript/message/51565346#51565346|NikiC]], it looks like we can reuse the GC_PROTECTED flag for strings.
+See the [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md|justification page]] as to why it's done this way.
-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".
+===== Comparison to Taint Tracking =====
-Unlike the Taint extension, there must **not** be an equivalent //untaint()// function, or support any kind of escaping.
+Some languages implement a "taint flag" which tracks whether values are considered "safe". There is a [[https://github.com/laruence/taint|Taint extension for PHP]] by Xinchen Hui, and [[https://wiki.php.net/rfc/taint|a previous RFC proposing it be added to the language]].
-===== Previous Work =====
+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.
-There is the [[https://github.com/laruence/taint|Taint extension]] by Xinchen Hui, but this approach explicitly allows escaping, which doesn't address all issues.
+This proposal avoids the complexity by addressing a different part of the problem: separating inputs supplied by the programmer, from inputs supplied by the user.
+===== Previous Work =====
-Google currently uses a [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md#go-implementation|similar approach in Go]] with the use of "compile time constants"; and there are [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md#javascript-implementation|discussions with it happening in JavaScript]].
+Google uses a [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md#go-implementation|similar approach in Go]] to identify "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.
-It might be possible to use static analysis, for example [[https://psalm.dev/|psalm]] (thanks [[https://news-web.php.net/php.internals/109192|Tyson Andre]]). But I can't find any which do these checks by default, they are likely to miss things that happen at runtime, and we can't expect all programmers to use static analysis (especially those who have just stated, who need this more than developers who know the concepts and just make the odd mistake).
+As noted by [[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).
-And there is the [[https://wiki.php.net/rfc/sql_injection_protection|Automatic SQL Injection Protection]] RFC by Matt Tait, where it was noted:
+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:
   * "unfiltered input can affect way more than only SQL" ([[https://news-web.php.net/php.internals/87355|Pierre Joye]]);
@@ Line 77: / Line 108: @@
   * 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 still need something to identify mistakes.
+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]]), and this is where //is_literal()// can be used to check that no mistakes are made.
+===== Usage =====
+By libraries:
+<code php>
+function literal_check($var) {
+  if (function_exists('is_literal') && !is_literal($var)) {
+    $level = 2; // Get from config, defaults to 1.
+    if ($level === 0) {
+      // Programmer aware, and is choosing to bypass this check.
+    } else if ($level === 1) {
+      trigger_error('Non-literal detected!', E_USER_NOTICE);
+    } else {
+      throw new Exception('Non-literal detected!');
+    }
+  }
+}
+function example($input) {
+  literal_check($input);
+  // ...
+}
+example('hello'); // OK
+example(strtoupper('hello')); // Exception thrown: the result of strtoupper is a new, non-literal string
+</code>
+Table and Fields in SQL, which cannot use parameters; for example //ORDER BY//:
+<code php>
+$order_fields = [
+    'name',
+    'created',
+    'admin',
+  ];
+$order_id = array_search(($_GET['sort'] ?? NULL), $order_fields);
+$sql = ' ORDER BY ' . $order_fields[$order_id];
+</code>
+Undefined number of parameters; for example //WHERE IN//:
+<code php>
+function where_in_sql($count) { // Should check for 0
+  $sql = '?';
+  for ($k = 1; $k < $count; $k++) {
+    $sql .= ',?';
+  }
+  return $sql;
+}
+$sql = 'WHERE id IN (' . where_in_sql(count($ids)) . ')';
+</code>
 ===== Backward Incompatible Changes =====
@@ Line 105: / Line 190: @@
 On [[https://github.com/craigfrancis/php-is-literal-rfc/issues|GitHub]]:
-  - Would this cause performance issues?
+  - Name it something else? [[https://news-web.php.net/php.internals/109197|Jakob Givoni]] suggested //is_from_literal()//.
-  - Can //array_fill()//+//implode()// pass though the "is_literal" flag for the "WHERE IN" case?
+  - Would this cause performance issues? A [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/tests/001.phpt|basic string concat test]], just focusing on string concat (worst case scenario), shows a 1.3% increase in processing time (1.341s to 1.358s = +0.017s).
-  - Should the function be named something else? ([[https://news-web.php.net/php.internals/109197|Jakob Givoni]] suggested //is_from_literal//).
+  - 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]].
-  - Systems/Frameworks that define certain variables (e.g. table name prefixes) without the use of a literal (e.g. ini/json/yaml files), 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 116: / Line 200: @@
 ===== Future Scope =====
-As noted by [[https://chat.stackoverflow.com/transcript/message/51573226#51573226|MarkR]], the benefit will come when it can be used by PDO and similar functions (//mysqli_query//, //preg_match//, etc).
+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.
-This check could be used to throw an exception, or generate an error/warning/notice, providing a way for PHP to teach new programmers, and/or completely block unsafe values in SQL, HTML, CLI, etc.
+**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).
-PHP could also have a mode where output (e.g. //echo '<html>'//) is blocked, and this can be bypassed (maybe via //ini_set//) when the HTML Templating Engine has created the correctly encoded output.
+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).
-And, for a bit of silliness, 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]] :-)
+**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 =====
@@ Line 134: / Line 220: @@
 ===== Implementation =====
-[[https://github.com/Danack/|Danack]] has [[https://github.com/php/php-src/compare/master...Danack:is_literal_attempt_two|started an implementation]].
+Joe Watkins has [[https://github.com/php/php-src/compare/master...krakjoe:literals|created an implementation]] which includes string concat. While the performance impact needs to be considered, this would provide the easiest solution for projects already using string concat for their parameterised SQL.
+Dan Ackroyd also [[https://github.com/php/php-src/compare/master...Danack:is_literal_attempt_two|started an implementation]], which uses functions like [[https://github.com/php/php-src/compare/master...Danack:is_literal_attempt_two#diff-2b0486443df74cd919c949f33f895eacf97c34b8490e7554e032e770ab11e4d8R2761|literal_combine()]] to avoid performance concerns.
 ===== References =====
@@ Line 143: / Line 231: @@
 N/A
+===== Thanks =====
+  - **Dan Ackroyd**, DanAck, for surprising me with the first implementation, and getting the whole thing started.
+  - **Joe Watkins**, krakjoe, for finding how to set the literal flag, and creating the implementation that supports string concat.
+  - **Rowan Tommins**, IMSoP, for re-writing this RFC to focus on the key features, and putting it in context of how it can be used by libraries.
+  - **Nikita Popov**, NikiC, for suggesting where the literal flag could be stored. Initially this was going to be the [[https://chat.stackoverflow.com/transcript/message/51565346#51565346|GC_PROTECTED flag for strings]], which allowed Dan to start the first implementation.
+  - **MarkR, **for alternative ideas, and noting that "interned strings in PHP have a flag" [[https://chat.stackoverflow.com/transcript/message/48927813#48927813|source]], which started the conversation on how this could be implemented.
+  - **Xinchen Hui**, who created the Taint Extension, allowing me to test the idea; and noting how Taint in PHP5 was complex, but "with PHP7's new zend_string, and string flags, the implementation will become easier" [[https://news-web.php.net/php.internals/87396|source]].

Differences

Page Tools