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/03/21 17:38] – Remove edit link craigfrancisrfc:is_literal [2021/05/03 19:38] – Don't write off the concat version (inc more perf stats) craigfrancis
Line 1: Line 1:
 ====== PHP RFC: Is Literal Check ====== ====== PHP RFC: Is Literal Check ======
  
-  * Version: 0.1+  * Version: 0.6
   * Date: 2020-03-21   * Date: 2020-03-21
 +  * Updated: 2021-04-30
   * Author: Craig Francis, craig#at#craigfrancis.co.uk   * Author: Craig Francis, craig#at#craigfrancis.co.uk
   * Status: Draft   * Status: Draft
   * First Published at: https://wiki.php.net/rfc/is_literal   * First Published at: https://wiki.php.net/rfc/is_literal
 +  * GitHub Repo: https://github.com/craigfrancis/php-is-literal-rfc
  
 ===== Introduction ===== ===== Introduction =====
  
-Add an //is_literal()// functionso developers/frameworks can be sure they are working with a safe value - one created from one or more literals, defined within PHP scripts.+A new function, //is_literal(string $string)//, to identify variables that have been created from a programmer defined string.
  
-This allows developers/frameworks, at runtime, to warn or block SQL Injection, Command Line Injection, and many cases of HTML Injection.+This takes the concept of "taint checking" and makes it simpler and stricter.
  
-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]]).+It does not allow a variable to be marked as untainted, and it does not allow escaping (important).
  
-This will also allow systems/frameworks to decide if they want to **block****educate** (via notice)or **ignore** these issues (to avoid the "don't nanny" concern raised by [[https://news-web.php.net/php.internals/87383|Lester Caine]]).+For exampletake database library that supports parametrised queries at the driver leveltoday a programmer could use either of these:
  
-===== Related JavaScript Implementation =====+<code php> 
 +$db->query('SELECT * FROM users WHERE id ?', [$_GET['id']]);
  
-This proposal is taking some ideas from TC39, where a similar idea is being discussed for JavaScript, to support the introduction of Trusted Types.+$db->query('SELECT * FROM users WHERE id = ' $_GET['id']); // INSECURE 
 +</code>
  
-https://github.com/tc39/proposal-array-is-template-object\\ +If the library only accepted a literal SQL string (written by the programmer), and simply rejected the second example (not written as a literal), the library can provide an "inherently safe API".
-https://github.com/mikewest/tc39-proposal-literals+
  
-They are looking at "Distinguishing strings from a trusted developerfrom strings that may be attacker controlled".+This definition of an "inherently safe API" comes from Christoph Kern, who did 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.
  
-===== Taint Checking =====+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.
  
-Xinchen Hui has done some amazing work with the Taint extension:+===== Why =====
  
-https://github.com/laruence/taint+The [[https://owasp.org/www-project-top-ten/|OWASP Top 10]] lists common vulnerabilities sorted by prevalence, exploitability, detectability, and impact. Each ranked out of 3.
  
-Unfortunately this approach does not address all issuesmainly 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:+**A1: Injection** - common prevalence (2)easy for attackers to detect/exploit (3), severe impact (3).
  
-  $sql = 'DELETE FROM table WHERE id = ' . mysqli_real_escape_string($db, $_GET['id'])+**A7: XSS** - widespread prevalence (3), easy for attackers to detect/exploit (3), moderate impact (2).
-   +
-  // delete.php?id=id +
-   +
-  // DELETE FROM table WHERE id = id+
  
-  $html = '<img src=' . htmlentities($_GET['url']. ' />'; +And these two have always been listed: 2003 (A6/A4), 2004 (A6/A4), 2007 (A2/A1), 2010 (A1/A2), 2013 (A1/A3), 2017 (A1/A7).
-   +
-  // 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),+It's because these mistakes are very easy to make, and hard to identify - is_literal() directly addresses this problem.
  
-===== Previous RFC =====+===== Examples =====
  
-Matt Tait suggested [[https://wiki.php.net/rfc/sql_injection_protection||Automatic SQL Injection Protection]].+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 stringThis is intended for use with literals and placeholders, but does not protect against this simple mistake:
  
-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]]).+<code php> 
 +// INSECURE 
 +$qb->select('u') 
 +   ->from('User', 'u') 
 +   ->where('u.id = ' $_GET['id']) 
 +</code>
  
-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]]).+The definition of the //where()// method could check with //is_literal()// and throw an exceptionadvising the programmer to replace it with a safer use of placeholders:
  
-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 databecause 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]]).+<code php> 
 +$qb->select('u'
 +   ->from('User', 'u') 
 +   ->where('u.id = :identifier'
 +   ->setParameter('identifier', $_GET['id'])
 +</code>
  
-I also agree that "SQL injection is almost solved problem [by usingprepared statements" ([[https://news-web.php.net/php.internals/87400|Scott Arciszewski]]), but we do need something that can identify mistakesideally at runtime.+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> 
 + 
 +===== Failed Solutions ===== 
 + 
 +==== Education ==== 
 + 
 +Developer training has not worked, it simply does not scale (people start programming every day), and learning about every single issue is difficult. 
 + 
 +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. 
 + 
 +We cannot keep saying they 'need to be careful', and relying on them to never make a mistake. 
 + 
 +==== Escaping ==== 
 + 
 +Escaping is hard, and error prone. 
 + 
 +We have a list of common [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification.md#common-mistakes|escaping mistakes]]. 
 + 
 +Developers should use parameterised queries (e.g. SQL), or a well tested library that knows how to escape values based on their context (e.g. HTML). 
 + 
 +==== Taint Checking ==== 
 + 
 +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]] by Wietse Venema. 
 + 
 +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. 
 + 
 +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. 
 + 
 +==== Static Analysis ==== 
 + 
 +While I agree with [[https://news-web.php.net/php.internals/109192|Tyson Andre]], it is highly recommended to use Static Analysis. 
 + 
 +But they nearly always focus on other issues (type checking, basic logic flaws, code formatting, etc)
 + 
 +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]]. 
 + 
 +For a quick example, psalm, 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: 
 + 
 +<code php> 
 +$db = new mysqli('...'); 
 + 
 +$id = (string) ($_GET['id'] ?? 'id'); // Keep the type checker happy. 
 + 
 +$db->prepare('SELECT * FROM users WHERE id = ' . $db->real_escape_string($id)); 
 +</code> 
 + 
 +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 did, you're back to escaping being an issue). 
 + 
 +But the biggest problem is that Static Analysis is simply not used by most developersespecially those who are new to programming (usage tends to be higher by those writing well tested libraries).
  
 ===== Proposal ===== ===== Proposal =====
  
-Add an //is_literal()// function to check if a given variable has only been created by Literal(s).+This RFC proposes adding four functions:
  
-This uses 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.+  * //is_literal(string $string): bool// to check if a variable represents value written into the source code or not. 
 +  * //literal_implode(string $glue, array $pieces)string// - implode an array of literals, with a literal. 
 +  * //literal_combine(string $piece, string ...$pieces): string// - allow concatenating literal strings. 
 +  * //literal_sprintf(string $formatstring ...$values): string// - a version of sprintf that uses literals.
  
-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 literal is defined as a value (string) which has been written by the programmerThe value may be passed between functionsas long as it is not modified in any way.
  
-Unlike the Taint extension, there is no need to provide an equivalent //untaint()// function.+<code php> 
 +is_literal('Example')// true
  
-===== Examples =====+$a 'Hello'; 
 +$b 'World';
  
-==== SQL InjectionBasic ====+is_literal($a); // true 
 +is_literal($a . $b); // TBCdetails below.
  
-A simple example:+$c = literal_combine($a, $b); 
 +is_literal($c); // true
  
-  $sql = 'SELECT * FROM table WHERE id = ?'; +is_literal($_GET['id'])// false 
-   +is_literal('WHERE id ' . intval($_GET['id'])); // false 
-  $result $db->exec($sql, [$id]);+is_literal(rand(0, 10)); // false 
 +is_literal(sprintf('LIMIT %d', 3)); // false 
 +</code>
  
-Checked in the framework by:+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.
  
-  class db { +===== Previous Work ===== 
-   + 
-    public function exec($sql, $parameters = []) { +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]]. 
-   + 
-      if (!is_literal($sql)) { +Google also uses [[https://errorprone.info/|Error Prone]] in Java to augment the compiler's type analysis, where [[https://errorprone.info/bugpattern/CompileTimeConstant|@CompileTimeConstant]] ensures method parameters can only use "compile-time constant expressions" (this isn't a complete solution either). 
-        throw new Exception('SQL must be 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 a regular expression to match and return known-good values (where regular expressions are easy to get wrong). 
 + 
 +[[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]]). 
 + 
 +As noted above, there is the [[https://github.com/laruence/taint|Taint extension for PHP]] by Xinchen Hui. 
 + 
 +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]]); 
 +  * 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]]), and this is where //is_literal()// can be used to check that no mistakes are made when using prepared statements. 
 + 
 +===== Usage ===== 
 + 
 +By libraries: 
 + 
 +<code php> 
 +class db 
 +  protected $level = 2; // Probably should default to 1 at first. 
 +  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);
 +    // ...
 +  }
 +}
  
-It will also work 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_real_escape_string($db, $_GET['id']); +
-   +
-    is_literal($sql); // Returns false+
  
-==== 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];+
  
-==== SQL Injection, WHERE IN ====+Undefined number of parameters; for example //WHERE IN//:
  
-Most SQL strings can be a concatenations of literal values, but //WHERE IN (?,?,?)// need 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>
  
-So there //might// need to be a special case for //array_fill()//+//implode()// or //str_repeat()//+//substr()// to create something like '?,?,?'+===== Considerations =====
  
-  $in_sql implode(',', array_fill(0, count($ids), '?')); +==== Naming ====
-   +
-  // or +
-   +
-  $in_sql substr(str_repeat('?,', count($ids)), 0, -1);+
  
-To be used with:+Literal string is the standard name for strings in source code. See [[https://www.google.com/search?q=what+is+literal+string+in+php|Google]].
  
-  $sql = 'SELECT * FROM table WHERE id IN (' $in_sql ')';+> 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...
  
-==== SQL Injection, ORM Usage ====+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".
  
-[[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:+==== Supporting Int/Float/Boolean values====
  
-  $users = $queryBuilder +When converting to stringthey aren't guaranteed (and often don'thave the exact same value they have in source code.
-    ->select('u'+
-    ->from('User', 'u') +
-    ->where('u.id = ' . $_GET['id']) +
-    ->getQuery() +
-    ->getResult(); +
-   +
-  // example.php?id=u.id+
  
-Where this mistake could be identified by:+For example, //TRUE// and //true// when cast to string give "1".
  
-  public function where($predicates) +It's also very low value feature, where there might not be space for a flag to be added.
-  { +
-      if (!is_literal($predicates)) { +
-          throw new Exception('Can only accept literal'); +
-      } +
-      ... +
-  }+
  
-[[https://redbeanphp.com/index.php?p=/finding|RedBean]] could check //$sql// is a literal:+==== Supporting Concatenation ====
  
-  $users = R::find('user', 'id = ' $_GET['id']);+This is the big question.
  
-[[http://propelorm.org/Propel/reference/model-criteria.html#relational-api|PropelORM]] could check //$clause// is a literal:+Máté Kocsis has done some [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/tests/results/with-concat/kocsismate.pdf|primary testing on supporting string concat]], 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]].
  
-  $users = UserQuery::create()->where('id = ' $_GET['id'])->find();+In my own [[https://github.com/craigfrancis/php-is-literal-rfc/tree/main/tests|simplistic testing]], where I included a basic version that did not support string concatThe [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/tests/results/with-concat/local.pdf|results]] found:
  
-==== SQL InjectionORM Internal ====+    Laravel Demo App: +0.30% withvs +0.18% without concat. 
 +    Symfony Demo App: +0.06% with, vs +0.06% without concat. 
 +    My Concat Test:   +4.36% with, vs +2.23% without concat.
  
-The //is_literal()// function could be used by ORM developers, so they can be sure they have created an SQL string out of literals.+In my basic test, I used a RAM Disk, and disabled the processors Turbo Boost. With the Demo Apps, I used ///sapi/cgi/php-cgi "-T10"// to get the timings (so would include the compilation), and ///sapi/cli/php// for [[https://github.com/craigfrancis/php-is-literal-rfc/blob/main/tests/001.phpt|My Concat Test]].
  
-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]].+There is still a small impact without concat because the //concat_function()// in "zend_operators.c" uses //zend_string_extend()// (where the literal flag needs to be removed)And in "zend_vm_def.h", it has a similar version; and supports a quick concat with an empty string, which doesn't create a new variable (x2) and would need it's flag removed as well.
  
-==== CLI Injection ====+Technically string concat isn't needed for most libraries, like an ORM or Query Builder, where their methods nearly always take a small literal string. But it would make adoption of is_literal() easier for existing projects that are currently using string concat for their SQL, HTML Templates, etc.
  
-Rather than using functions such as:+And supporting runtime concat would make the literal check easier to understand, as it would be consistent (e.g. compiler vs runtime concat, where the compiler can concat two strings to create a single literal that has the literal flag set).
  
-  * //exec()// +The non-concat version would use //literal_combine()// or //literal_implode()// as special functions to avoid most of the work during runtime contact. Where Dan Ackroyd notes that these functions would make it easier to identify exactly where mistakes are made, rather than it being picked up at the end of a potentially long script, after multiple string concatenations, e.g.
-  * //shell_exec()// +
-  * //system()// +
-  * //passthru()//+
  
-Frameworks (or PHP) could introduce something similar to //pcntl_exec()//, where arguments are provided separately.+<code php> 
 +$sortOrder = 'ASC';
  
-Ortake a verified literal for the command, and use parameters for the arguments (like SQL):+// 300 lines of codeor multiple function calls
  
-  $output parameterised_exec('grep ? /path/to/file | wc -l', [ +$sql .= ' ORDER BY name . $sortOrder;
-      'example', +
-    ]);+
  
-Rough implementation:+// 300 lines of code, or multiple function calls
  
-  function parameterised_exec($cmd, $args = []) { +$db->query($sql); 
-   +</code>
-    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 ====+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:
  
-Template engines should receive variables separately from the raw HTML.+<code php> 
 +$sql = literal_combine($sql, ' ORDER BY name ', $sortOrder); 
 +</code>
  
-Often the engine will get the HTML from static files:+==== Performance ====
  
-  $html = file_get_contents('/path/to/template.html');+TBC
  
-But small snippets of HTML are often easier to define as a literal within the PHP script:+See the section above.
  
-  $template_html +==== Values from INI/JSON/YAML ====
-    <p>Hello <span id="username"></span></p> +
-    <p><a>Website</a></p>';+
  
-Where the variables are supplied separatelyin this example I'm using XPaths:+As 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 feature (depending on where they use the is_literal check).
  
-  $values +==== Existing String Functions ====
-      '//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 variablesthe templating engine can accept and apply the supplied variables for the relevant contextfor example:+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 mistakes.
  
-  function template_parse($html$values) { +For any use-case where dynamic strings are requiredit would be better to build those strings with an appropriate query builderor by using //literal_combine()/////literal_implode()//.
-   +
-    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 ===== ===== Backward Incompatible Changes =====
  
-Not sure+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?+PHP 8.1
  
 ===== RFC Impact ===== ===== RFC Impact =====
Line 329: Line 322:
 ==== To SAPIs ==== ==== To SAPIs ====
  
-Not sure+None known
  
 ==== To Existing Extensions ==== ==== To Existing Extensions ====
Line 341: Line 334:
 ===== Open Issues ===== ===== Open Issues =====
  
-  - Can //array_fill()//+//implode()// or //str_repeat()//+//substr()// pass though the "is_literal" flag for the "WHERE IN" case? +None
-  - 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]].+
  
 ===== Unaffected PHP Functionality ===== ===== Unaffected PHP Functionality =====
  
-Not sure+None known
  
 ===== Future Scope ===== ===== Future Scope =====
  
-Certain functions (mysqli_query, preg_match, etc) might use this information to generate error/warning/notice.+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. 
 + 
 +**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). 
 + 
 +For example, 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), MarkR would like a //is_figurative()// function (functionality to be confirmed).
  
 ===== Proposed Voting Choices ===== ===== Proposed Voting Choices =====
  
-Not sure+Accept the RFC. Yes/No
  
 ===== Patches and Tests ===== ===== Patches and Tests =====
  
-volunteer is needed to help with implementation.+N/A
  
 ===== Implementation ===== ===== Implementation =====
  
-N/A+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 supports string concat at runtime.
  
 ===== References ===== ===== References =====
  
-- https://wiki.php.net/rfc/sql_injection_protection+N/A
  
 ===== Rejected Features ===== ===== Rejected Features =====
  
 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 supports string concat.
 +  - **Máté Kocsis**, mate-kocsis, for setting up and doing the performance testing.
 +  - **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