rfc:is_literal

Differences

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

Link to this comparison view

Both sides previous revisionPrevious revision
Next revision
Previous revision
Next revisionBoth sides next revision
rfc:is_literal [2020/12/26 11:41] – New examples, with a focus on escaping craigfrancisrfc:is_literal [2021/05/02 16:38] – Add performance stats, and notes from Dan craigfrancis
Line 1: Line 1:
 ====== PHP RFC: Is Literal Check ====== ====== PHP RFC: Is Literal Check ======
  
-  * Version: 0.2+  * Version: 0.6
   * Date: 2020-03-21   * Date: 2020-03-21
-  * Updated: 2020-12-22+  * Updated: 2021-04-30
   * Author: Craig Francis, craig#at#craigfrancis.co.uk   * Author: Craig Francis, craig#at#craigfrancis.co.uk
   * Status: Draft   * Status: Draft
Line 11: Line 11:
 ===== Introduction ===== ===== Introduction =====
  
-Add an //is_literal()// functionso developers/frameworks can check if given variable is **safe**.+A new function, //is_literal(string $string)//, to identify variables that have been created from programmer defined string.
  
-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 takes the concept of "taint checking" and makes it simpler and stricter.
  
-This simple check can be used to warn or completely block SQL Injection, Command Line Injection, and many cases of HTML Injection (aka XSS).+It does not allow a variable to be marked as untainted, and it does not allowing escaping (important).
  
-===== The Problem =====+For example, a database library that supports parametrised queries at the driver level, today a programmer could use either of these:
  
-Escaping strings for SQL, HTML, Commands, etc is **very** error prone.+<code php> 
 +$db->query('SELECT FROM users WHERE id = ' $_GET['id']); // INSECURE
  
-The vast majority of programmers should never do this (mistakes will be made).+$db->query('SELECT * FROM users WHERE id = ?', [$_GET['id']])
 +</code>
  
-Unsafe values (often user supplied) **must** be kept separate (e.g. parameterised SQL), or be processed by something that understands the context (e.g. a HTML Templating Engine).+By rejecting the SQL that was not written as a literal (first example), and only accepting a literal string (written by the programmer), the library can provide an "inherently safe API".
  
-This is primarily for security reasonsbut it can also cause data to be damaged (e.gASCII/UTF-8 issues).+This definition of an "inherently safe API" comes from Christoph Kernwho did a talk in 2016 about [[https://www.youtube.com/watch?v=ccfEu-Jj0as|Preventing Security Bugs through Software Design]] (also at [[https://www.usenix.org/conference/usenixsecurity15/symposium-program/presentation/kern|USENIX Security 2015]]), which covers how this is used at Google. The idea is that we "Don't Blame the Developer, Blame the API"; where we need to put the burden on libraries (written once, used by many) to ensure that it's impossible for the developer to make these mistakes.
  
-Take these mistakes, where the value has come from the user:+By adding a way for libraries to check if the strings they receive came from the developer (from trusted PHP source code), it allows the library to check they are being used in a safe way.
  
-  echo "<img src=" . $url . " alt='' />";+===== Why =====
  
-Flawed, and unfortunately very common, a classic XSS vulnerability.+The [[https://owasp.org/www-project-top-ten/|OWASP Top 10]] lists common vulnerabilities sorted by prevalenceexploitability, detectability, and impact.
  
-  echo "<img src=" . htmlentities($url) . " alt='' />";+**Injection vulnerabilities** remain at the top of the list (common prevalence, easy for attackers to detect/exploit, severe impact)and **XSS** comes in at 7 (widespread prevalence, easy for attackers to detect/exploit, moderate impact).
  
-Flawed because the attribute value is not quotede.g. //$url = '/ onerror=alert(1)'//+This is because injection mistakes are very easy to makeand hard to identify - is_literal() addresses this.
  
-  echo "<img src='" . htmlentities($url) . "' alt='' />";+===== Examples =====
  
-Flawed because //htmlentities()// does not encode single quotes by default, e.g. //$url = "/' onerror='alert(1)"//+The [[https://www.doctrine-project.org/projects/doctrine-orm/en/current/reference/query-builder.html#high-level-api-methods|Doctrine Query Builder]] allows a custom WHERE clause to be provided as a string. This is intended for use with literals and placeholders, but does not protect against this simple mistake:
  
-  echo '<a href="' . htmlentities($url) . '">Link</a>';+<code php> 
 +// INSECURE 
 +$qb->select('u'
 +   ->from('User', 'u'
 +   ->where('u.id = ' . $_GET['id']) 
 +</code>
  
-Flawed because a link can include JavaScript, e.g. //$url = 'javascript:alert(1)'//+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:
  
-  <script+<code php
-    var url "<?= addslashes($url?>"+$qb->select('u'
-  </script>+   ->from('User', 'u'
 +   ->where('u.id :identifier'
 +   ->setParameter('identifier', $_GET['id']); 
 +</code>
  
-Flawed because //addslashes()// is not HTML context aware, e.g. //$url = '</script><script>alert(1)</script>'//+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:
  
-  echo '<a href="/path/?name=' . htmlentities($name. '">Link</a>';+<code php> 
 +// INSECURE 
 +echo $twig->createTemplate('<p>Hi ' . $_GET['name'. '</p>')->render(); 
 +</code>
  
-Flawed because //urlencode()// has not been used, e.g. //$name = 'A&B'//+If //createTemplate()// checked with //is_literal()//, the programmer could be advised to write this instead:
  
-  <p><?= htmlentities($url?></p>+<code php> 
 +echo $twig->createTemplate('<p>Hi {{ name }}</p>')->render(['name' => $_GET['name']])
 +</code>
  
-Flawed because the encoding is not guaranteed to be UTF-8 (or ISO-8859-1 before PHP 5.4), so the value could be corrupted.+===== Failed Solutions =====
  
-Also flawed because some browsers (e.g. IE 11), if the charset isn't defined (header or meta tag), could guess the output as UTF-7, e.g. //$url '+ADw-script+AD4-alert(1)+ADw-+AC8-script+AD4-'//+==== Education ====
  
-  example.html: +Developer training has not worked, it simply does not scale (people start programming every day), and learning about every single issue is difficult.
-      <img src={{ url }} alt='' /> +
-   +
-  $loader = new \Twig\Loader\FilesystemLoader('./templates/')+
-  $twig = new \Twig\Environment($loader['autoescape' => 'name']); +
-   +
-  echo $twig->render('example.html', ['url' => $url]);+
  
-Flawed because Twig is not context aware (in this case, an unquoted HTML attribute), e.g. //$url = '/ onerror=alert(1)'//+Keeping in mind that programmers will frequently do just enough to complete their task (busy), where they often copy/paste a solution to their problem they find online (risky), modify it for their needs (risky), then move on.
  
-  $sql = 'SELECT 1 FROM user WHERE id=' . $mysqli->escape_string($id);+We cannot keep saying they 'need to be careful', and relying on them to never make a mistake.
  
-Flawed because the value has not been quoted, e.g. //$id 'id', or '1 OR 1=1'//+==== Escaping ====
  
-  $sql = 'SELECT 1 FROM user WHERE id="' $mysqli->escape_string($id) . '"';+Escaping is hard, and error prone.
  
-Flawed if 'sql_mode' includes //NO_BACKSLASH_ESCAPES//, e.g. //$id = '2" or "1"="1'//+We have a list of common [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md#common-mistakes|escaping mistakes]].
  
-  $sql = 'INSERT INTO user (nameVALUES ("' $mysqli->escape_string($name) '")';+Developers should use parameterised queries (e.g. SQL), or a well tested library that knows how to escape values based on their context (e.gHTML).
  
-Flawed if 'SET NAMES latin1' has been used, and escape_string() uses 'utf8'.+==== Taint Checking ====
  
-  $parameters = "-f$email"+Some languages implement a "taint flag" which tracks whether values are considered "safe".
-   +
-  // $parameters = '-f' . escapeshellarg($email); +
-   +
-  mail('a@example.com', 'Subject', 'Message', NULL, $parameters);+
  
-Flawed because it's not possible to safely escape values in //$additional_parameters// for //mail()//, e.g. //$email = 'b@example.com -X/www/example.php'//+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 Solutions =====+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.
  
-[[https://github.com/laruence/taint|Taint extension]] by Xinchen Huibut this approach explicitly allows escaping, which doesn't address the issues listed above.+This proposal avoids the complexity by addressing a different part of the problemseparating inputs supplied by the programmerfrom inputs supplied by the user.
  
-[[https://wiki.php.net/rfc/sql_injection_protection|Automatic SQL Injection Protection]] by Matt Tait, where it was noted:+==== Static Analysis ====
  
-  * "unfiltered input can affect way more than only SQL" ([[https://news-web.php.net/php.internals/87355|Pierre Joye]]); +While I agree with [[https://news-web.php.net/php.internals/109192|Tyson Andre]], it is highly recommended to use Static Analysis.
-  * this amount of work isn't ideal for "just for one use case" ([[https://news-web.php.net/php.internals/87647|Julien Pauli]]); +
-  * 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]]).+
  
-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.+But they nearly always focus on other issues (type checkingbasic logic flaws, code formatting, etc).
  
-===== Related JavaScript Implementation =====+Those that attempt to address injection vulnerabilities, do so via Taint Checking (see above), and are [[https://github.com/vimeo/psalm/commit/2122e4a1756dac68a83ec3f5abfbc60331630781|often incomplete]].
  
-This RFC is taking some ideas from TC39, where similar idea is being discussed for JavaScriptto support the introduction of Trusted Types.+For quick examplepsalm, even in its strictest errorLevel (1), and/or running //--taint-analysis//, will not notice the missing quote marks in this SQL, and will incorrectly assume this is perfectly safe:
  
-https://github.com/tc39/proposal-array-is-template-object\\ +<code php> 
-https://github.com/mikewest/tc39-proposal-literals+$db = new mysqli('...');
  
-They are looking at "Distinguishing strings from a trusted developer, from strings that may be attacker controlled".+$id = (string) ($_GET['id'] ?? 'id'); // Keep the type checker happy.
  
-===== Solution =====+$db->prepare('SELECT * FROM users WHERE id ' . $db->real_escape_string($id)); 
 +</code>
  
-Literals are safe valuesdefined within the PHP scriptsfor example:+When psalm comes to taint checking the usage of a library (like Doctrine), it assumes all methods are safe, because none of them note the sinks (and even if they didyou're back to escaping being an issue).
  
-  $a = 'Example'; +But the biggest problem is that Static Analysis is simply not used by most developers, especially those who are new to programming (usage tends to be higher by those writing well tested libraries).
-  is_literal($a); // true +
-   +
-  $a = 'Example ' . $a . ', ' . 5; +
-  is_literal($a); // true +
-   +
-  $a = 'Example ' . $_GET['id']; +
-  is_literal($a); // false +
-   +
-  $a = 'Example ' . time(); +
-  is_literal($a); // false +
-   +
-  $a = sprintf('LIMIT %d', 3); +
-  is_literal($a); // false +
-   +
-  $c = count($ids); +
-  $a = 'WHERE id IN (' . implode(',', array_fill(0, $c, '?')) . ')'; +
-  is_literal($a); // true, the odd one that involves functions. +
-   +
-  $limit = 10; +
-  $a = 'LIMIT ' . ($limit + 1)+
-  is_literal($a); // false, but might need some discussion.+
  
-This uses a similar definition of [[https://wiki.php.net/rfc/sql_injection_protection#safeconst|SafeConst]] from Matt Tait's RFC, but it does not need to accept Integer or FloatingPoint variables as safe (unless it makes the implementation easier), nor should this proposal effect any existing functions.+===== Proposal =====
  
-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".+This RFC proposes adding three functions:
  
-And thanks to [[https://chat.stackoverflow.com/transcript/message/48927813#48927813|Mark R]]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".+  * //is_literal(string $string)bool// to check if a variable represents a value written into the source code or not. 
 +  * //literal_combine(string $piecestring $pieces): string// to allow concatenating literal strings
 +  * //literal_implode(string $gluearray $pieces): string// to implode an array of literals, with a literal.
  
-Commands can be checked to ensure they are "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]]).+A literal is defined as value (string) which has been written by the programmerThe value may be passed between functions, as long as it is not modified in any way.
  
-This approach allows all 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]]).+<code php> 
 +is_literal('Example')// true
  
-Unlike the Taint extension, there must **not** be an equivalent //untaint()// function, or support any kind of escaping.+$a = 'Hello'; 
 +$b = 'World';
  
-==== Solution: SQL Injection ====+is_literal($a); // true 
 +is_literal($a . $b); // false, no concat at run time (details below).
  
-Database abstractions (e.g. ORMswill be able to ensure they are provided with strings that are safe.+is_literal(literal_combine($a, $b)); // true
  
-[[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 //->where($predicates)// is a literal:+is_literal($_GET['id']); // false 
 +is_literal('WHERE id = ' intval($_GET['id'])); // false 
 +is_literal(rand(0, 10)); // false 
 +is_literal(sprintf('LIMIT %d', 3)); // false, should use parameters 
 +</code>
  
-  $users = $queryBuilder +There is no way to manually mark a string as a literal (i.eno equivalent to //untaint()//); as soon as the value has been manipulated in any way, it is no longer marked as a literal.
-    ->select('u'+
-    ->from('User', 'u'+
-    ->where('u.id = ' $_GET['id']) +
-    ->getQuery() +
-    ->getResult(); +
-   +
-  // example.php?id=u.id+
  
-This mistake can be easily identified by:+*Technical detailStrings that are concatenated in place at compile time are treated as a literal.*
  
-  public function where($predicates) +===== Previous Work =====
-  { +
-      if (function_exists('is_literal') && !is_literal($predicates)) { +
-          throw new Exception('->where() can only accept a literal'); +
-      } +
-      ... +
-  }+
  
-[[https://redbeanphp.com/index.php?p=/finding|RedBean]] could check //$sql// is a literal:+Google uses "compile time constants" in Go, which isn't as good as a run time solution (e.g. the //WHERE IN// issue), but it works, and is used by [[https://blogtitle.github.io/go-safe-html/|go-safe-html]] and [[https://github.com/google/go-safeweb/tree/master/safesql|go-safesql]].
  
-  $users = R::find('user', 'id = ' . $_GET['id']);+Google also uses [[https://errorprone.info/|Error Prone]] in Java to augment the compiler's type analysiswhere [[https://errorprone.info/bugpattern/CompileTimeConstant|@CompileTimeConstant]] ensures method parameters can only use "compile-time constant expressions" (this isn't a complete solution either).
  
-[[http://propelorm.org/Propel/reference/model-criteria.html#relational-api|PropelORM]] could check //$clause// is a literal:+Perl has a [[https://perldoc.perl.org/perlsec#Taint-mode|Taint Mode]], via the -T flag, where all input is marked as "tainted", and cannot be used by some methods (like commands that modify files), unless you use regular expression to match and return known-good values (where regular expressions are easy to get wrong).
  
-  $users = UserQuery::create()->where('id = ' . $_GET['id'])->find();+[[https://github.com/tc39/proposal-array-is-template-object|JavaScript might get isTemplateObject]] to "Distinguishing strings from a trusted developer from strings that may be attacker controlled" (intended to be [[https://github.com/mikewest/tc39-proposal-literals|used with Trusted Types]]).
  
-The //is_literal()// function could also be used internally by ORM developersso they can be sure they have created their SQL strings 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]].+As noted abovethere is the [[https://github.com/laruence/taint|Taint extension for PHP]] by Xinchen Hui.
  
-==== Solution: SQL Injection, Basic ====+And there is the [[https://wiki.php.net/rfc/sql_injection_protection|Automatic SQL Injection Protection]] RFC by Matt Taitwhere 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:
  
-A simple example:+  * "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]]); 
 +  * 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]]).
  
-  $sql = 'SELECT * FROM table WHERE id = ?'; +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 when using prepared statements.
-   +
-  $result = $db->exec($sql, [$id]);+
  
-Checked in the framework by:+===== Usage =====
  
-  class db { +By libraries: 
-   + 
-    public function exec($sql, $parameters []) { +<code php> 
-   +class db { 
-      if (!is_literal($sql)) { +  protected $level = 2; // Probably should default to 1 at first. 
-        throw new Exception('SQL must be a literal.');+  function literal_check($var) { 
 +    if (function_exists('is_literal') && !is_literal($var)) { 
 +      if ($this->level ==0) { 
 +        // Programmer aware, and is choosing to bypass this check. 
 +      } else if ($this->level === 1
 +        trigger_error('Non-literal detected!', E_USER_WARNING)
 +      } else 
 +        throw new Exception('Non-literal detected!');
       }       }
-   
-      $statement = $this->pdo->prepare($sql); 
-      $statement->execute($parameters); 
-      return $statement->fetchAll(); 
-   
     }     }
-   
   }   }
 +  function unsafe_disable_injection_protection() {
 +    $this->level = 0;
 +  }
 +  function where($sql, $parameters = []) {
 +    $this->literal_check($sql);
 +    // ...
 +  }
 +}
  
-This also works with string concatenation:+$db->where('id = ?'); // OK 
 +$db->where('id = ' . $_GET['id']); // Exception thrown 
 +</code>
  
-  define('TABLE''example'); +Table and Fields in SQLwhich cannot use parameters; for example //ORDER BY//:
-   +
-  $sql = 'SELECT * FROM ' . TABLE . ' WHERE id = ?'; +
-   +
-    is_literal($sql); // Returns true +
-   +
-  $sql .= ' AND id = ' . $mysqli->escape_string($_GET['id']); +
-   +
-    is_literal($sql); // Returns false+
  
-==== Solution: SQL InjectionORDER BY ====+<code php> 
 +$order_fields 
 +    'name', 
 +    'created', 
 +    'admin', 
 +  ];
  
-To ensure //ORDER BY// can be set via the userbut only use acceptable values:+$order_id = array_search(($_GET['sort'] ?? NULL)$order_fields);
  
-  $order_fields = [ +$sql literal_combine(' ORDER BY '$order_fields[$order_id]); 
-      'name', +</code>
-      'created', +
-      'admin', +
-    ]; +
-   +
-  $order_id array_search(($_GET['sort'] ?? NULL), $order_fields); +
-   +
-  $sql = ' ORDER BY ' $order_fields[$order_id];+
  
-==== Solution: SQL Injection, WHERE IN ====+Undefined number of parameters; for example //WHERE IN//:
  
-Most SQL strings can be a simple concatenations of literal values, but //WHERE IN (?,?,?)// needs to use a variable number of literal placeholders.+<code php> 
 +function where_in_sql($count) { // Should check for 0 
 +  $sql = []; 
 +  for ($k = 0; $k < $count; $k++) { 
 +    $sql[] = '?'; 
 +  } 
 +  return literal_implode(',', $sql); 
 +
 +$sql = literal_combine('WHERE id IN ('where_in_sql(count($ids))')'); 
 +</code>
  
-There needs to be a special case for //array_fill()//+//implode()//, so the //is_literal// state can be preserved, allowing us to create the safe literal string '?,?,?':+===== Considerations =====
  
-  $in_sql implode(',', array_fill(0, count($ids), '?')); +==== Naming ====
-   +
-  $sql 'SELECT * FROM table WHERE id IN (' . $in_sql . ')';+
  
-==== SolutionCLI Injection ====+Literal string is the standard name for strings in source code. See https://www.google.com/search?q=what+is+literal+string+in+php
  
-Rather than using functions such as:+> A string literal is the notation for representing a string value within the text of a computer program. In PHP, strings can be created with single quotes, double quotes or using the heredoc or the nowdoc syntax. ... The heredoc preserves the line breaks and other whitespace (including indentation) in the text.
  
-  * //exec()// +Alternatives suggestions have included //is_from_literal()// from [[https://news-web.php.net/php.internals/109197|Jakob Givoni]]. I think //is_safe_string()// might be asking for trouble. Other terms have included "compile time constants" and "code string".
-  * //shell_exec()// +
-  * //system()// +
-  * //passthru()//+
  
-Frameworks (or PHP) could introduce something similar to //pcntl_exec()//, where arguments are provided separately.+==== Supporting Int/Float/Boolean values====
  
-Or, take a safe literal for the command, and use parameters for the arguments (like SQL does):+When converting to stringthey aren't guaranteed (and often don't) have the exact same value they have in source code.
  
-  $output = parameterised_exec('grep ? /path/to/file | wc -l', [ +For example, //TRUE// and //true// when cast to string give "1".
-      'example', +
-    ]);+
  
-Rough implementation:+It's also a very low value feature, where there might not be space for a flag to be added.
  
-  function parameterised_exec($cmd, $args []) { +==== Supporting Concatenation ====
-   +
-    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); +
-   +
-  }+
  
-==== Solution: HTML Injection ====+Unfortunately early testing suggests there will be too much of a performance impact, and is why //literal_combine()// or //literal_implode()// exists.
  
-Template engines should receive variables separately from the raw HTML.+It isn't needed for most libraries, like an ORM or Query Builder, where their methods nearly always take a small literal string.
  
-Often the engine will get the HTML from static files (safe):+It was considered because it would have made it easier for existing projects currently using string concatenation to adopt.
  
-  $html = file_get_contents('/path/to/template.html');+Joe Watkins has created a version that does support string concatenation, which we might re-consider at a later date (Joe's implementation also sets the literal flag, and was used as the basis for this implementation).
  
-But small snippets of HTML are often easier to define as a literal within the PHP script:+Máté Kocsis did the [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/tests/results/with-concat/kocsismate.pdf|primary testing on the string concat version]], and found a 0.124% performance hit for the Laravel Demo app, 0.161% for Symfony, and a more severe -3.719% when running this [[https://github.com/kocsismate/php-version-benchmarks/blob/main/app/zend/concat.php#L25|concat test]].
  
-  $template_html = ' +In my own [[https://github.com/craigfrancis/php-is-literal-rfc/tree/main/tests|simplistic testing]], the [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/tests/results/with-concat/local.pdf|results]] found 0.42% performance hit for the Laravel Demo app, 0.40% for Symfony, and 1.81% when running my [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/tests/001.phpt|concat test]] via //./cli/php//.
-    <p>Hello <span id="username"></span></p> +
-    <p><a>Website</a></p>';+
  
-Where the variables are supplied separatelyin this example I'm using XPath:+Dan Ackroyd also notes that the use of //literal_combine()// or //literal_implode()// will make it easier to identify exactly where mistakes are maderather than it being picked up at the end of a potentially long script, after multiple string concatenations, e.g.
  
-  $values = [ +<code php
-      '//span[@id="username"]' =[ +$sortOrder = 'ASC';
-          NULL      ='Name', // The textContent +
-          'class'   => 'admin', +
-          'data-id' => '123', +
-        ], +
-      '//a' => [ +
-          'href' => 'https://example.com', +
-        ], +
-    ]; +
-   +
-  echo template_parse($template_html, $values);+
  
-The templating engine can then accept and apply the supplied variables for the relevant context.+// 300 lines of code, or multiple function calls
  
-As a simple example, this can be done with:+$sql .= ' ORDER BY name ' . $sortOrder;
  
-  function template_parse($html, $values) { +// 300 lines of code, or multiple function calls 
-   + 
-    if (!is_literal($html)) { +$db->query($sql); 
-      throw new Exception('Invalid Template HTML.'); +</code
-    } + 
-   +If a developer changed the literal //'ASC'// to //$_GET['order']//, the error raised by //$db->query()// would not be clear where the mistake was made. Whereas using //literal_combine()// highlights exactly where the issue happened: 
-    $dom = new DomDocument(); + 
-    $dom->loadHTML('<?xml encoding="UTF-8">'$html); +<code php> 
-   +$sql literal_combine($sql, ORDER BY name ', $sortOrder); 
-    $xpath = new DOMXPath($dom); +</code> 
-   + 
-    foreach ($values as $query =$attributes) { +==== Performance ==== 
-   + 
-      if (!is_literal($query)) { +The proposed implementation has
-        throw new Exception('Invalid Template XPath.'); + 
-      } +    [TODO] 
-   + 
-      foreach ($xpath->query($queryas $element{ +Alsosee the section abovewhere string concatenation support would have introduced a ~1% performance hit. 
-        foreach ($attributes as $attribute => $value) { + 
-   +==== Values from INI/JSON/YAML ==== 
-          if (!is_literal($attribute)) { + 
-            throw new Exception('Invalid Template Attribute.'); +As noted by [[https://news-web.php.net/php.internals/87667|Dennis Birkholz]], Systems/Frameworks that define certain variables (e.g. table name prefixeswithout the use of a literal (e.g. ini/json/yaml files)might need to make some changes to use this feature (depending on where they use the is_literal check). 
-          } + 
-   +==== Existing String Functions ===
-          if ($attribute) { + 
-            $safe false; +Trying to determine if the //is_literal// flag should be passed through functions like //str_repeat()//, or //substr()// etc is difficult. Having a security feature be difficult to reason aboutgives a much higher chance of making a mistake. 
-            if ($attribute == 'href') { + 
-              if (preg_match('/^https?:\/\//', $value)) { +For any use-case where dynamic strings are required, it would be better to build those strings with an appropriate query builder, or by using //literal_combine()/////literal_implode()//.
-                $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 ===== ===== Backward Incompatible Changes =====
  
-None+No known BC breaks, except for code-bases that already contain userland functions //is_literal()//, //literal_implode()// or //literal_combine()//.
  
 ===== Proposed PHP Version(s) ===== ===== Proposed PHP Version(s) =====
  
-PHP 8.1?+PHP 8.1
  
 ===== RFC Impact ===== ===== RFC Impact =====
Line 403: Line 314:
 ==== To SAPIs ==== ==== To SAPIs ====
  
-Not sure+None known
  
 ==== To Existing Extensions ==== ==== To Existing Extensions ====
Line 415: Line 326:
 ===== Open Issues ===== ===== Open Issues =====
  
-On [[https://github.com/craigfrancis/php-is-literal-rfc/issues|GitHub]]:+None
  
-  - Would this cause performance issues? Presumably not as bad a type checking. +===== Unaffected PHP Functionality =====
-  - Can //array_fill()//+//implode()// pass though the "is_literal" flag for the "WHERE IN" case? +
-  - Should the function be named //is_from_literal()//? (suggestion from [[https://news-web.php.net/php.internals/109197|Jakob Givoni]]) +
-  - Systems/Frameworks that define certain variables (e.g. table name prefixes) without the use of a literal (e.g. ini/json/yaml files), so 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]].+
  
-===== Alternatives =====+None known
  
-  - The current Taint Extension (notes above) +===== Future Scope =====
-  - Using static analysis (not at runtime), 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 (if they even try), and we can't expect all programmers to use static analysis (especially those who have just stated).+
  
-===== Unaffected PHP Functionality =====+As noted by 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.
  
-Not sure+**Phase 2** could introduce a way for programmers to specify certain PHP function/method arguments can only accept literals, and/or specific value-objects their project trusts (this idea comes from [[https://web.dev/trusted-types/|Trusted Types]] in JavaScript).
  
-===== Future Scope =====+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).
  
-Certain functions (//mysqli_query////preg_match//etccould use this information to generate a error/warning/notice.+**Phase 3** could set a default of 'only literals' for all of the relevant PHP function argumentsso developers are given a warningand 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).
  
-PHP could also have mode where output (e.g. //echo '<html>'//is blockedand this can be bypassed (maybe via //ini_set//) when the HTML Templating Engine has created the correctly encoded output.+And, for bit of silliness (Spaß ist verboten), MarkR would like a //is_figurative()// function (functionality to be confirmed).
  
 ===== Proposed Voting Choices ===== ===== Proposed Voting Choices =====
  
-N/A+Accept the RFC. Yes/No
  
 ===== Patches and Tests ===== ===== Patches and Tests =====
  
-volunteer is needed to help with implementation.+N/A
  
 ===== Implementation ===== ===== Implementation =====
 +
 +Dan Ackroyd has [[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.
 +
 +Joe Watkins has [[https://github.com/php/php-src/compare/master...krakjoe:literals|created an implementation]] which includes string concat. As noted above, the performance impact is probably too much for it to be accepted.
 +
 +===== References =====
  
 N/A N/A
Line 452: Line 365:
  
 N/A N/A
 +
 +===== Thanks =====
 +
 +  - **Dan Ackroyd**, DanAck, for starting the first implementation (which made this a reality), and followup on the version that uses functions instead of string concat.
 +  - **Joe Watkins**, krakjoe, for finding how to set the literal flag (tricky), and creating the implementation that does support 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 "GC_PROTECTED flag for strings", which allowed Dan to start the first implementation.
 +  - **Mark Randall**, MarkR, for alternative ideas, and noting that "interned strings in PHP have a flag", 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]].
  
rfc/is_literal.txt · Last modified: 2022/02/14 00:36 by craigfrancis