Some questions about literal-string

[VincentLanglet]

Since I tried to work on stub about literal-string, I have some questions @craigfrancis.
And since the is_literal is not working so far, I cannot try it.

https://phpstan.org/r/499a50f0-3efa-4cee-ab55-24b2244ca0cb
In this example, I expect phpstan to report that the assert(is_literal($text)); is always true.

This can create some issue: If a library doesn't use a level of phpstan high enough (example with level 4: https://phpstan.org/r/cb2b9b60-5f48-4966-a893-f6fbb1b8a390) or if the setter is called in another library (so you cannot trust the value), the literal-string is considered as always true but isn't.
It could give a false feeling of security and people could remove some is_literal check which would have been useful.

Another question I have with the literal-string, is about how it should be used by the developper.
For instance, if I have written something like
https://phpstan.org/r/458e9c61-a1be-4d48-8def-ed5c03cb29f8
I feel like my code is not so robust since a refacto of You class like
https://phpstan.org/r/e50d8628-4894-4dfd-a361-50e3fbf73bff
could change my code so easily because I end in the else part instead of the if one.

With this example of how easily code could break when using some is_literal check, I don't feel safe using this method.

[craigfrancis]

Hi @VincentLanglet,

First some background...

Unfortunately the PHP RFC for the is_literal() function failed. I will have another go, as I didn't receive any feedback that suggested it was wrong; e.g. a couple of people didn't want it to support string concatenation (on the basis that it could make mistakes easier to find; I disagree, I think tooling should do that, and this limitation would only make is_literal() harder to adopt), some think injection problems should be solved with documentation/education (I disagree, as that hasn't worked in the last ~20 years, and mistakes still happen), a few think this should only be done with static analysis (which I've been working on, but there are complications, e.g. I don't think we can get every single developer using static analysis; and those that don't probably need this the most), some felt the implementation could be improved (later they admitted to mis-reading the performance stats), and a couple of people thought the vote was too close to the 8.1 deadline to be comfortable voting for it.

I'd also note the is_literal() function has a slightly different way of working; it works at runtime, via a flag that's set on every string variable when they are created. Static Analysis, while it's much better at checking every source for every variable (e.g. $_GET['sort'] ?? 'name'), it relies on every bit of code being being checked by the relevant author. For example. a library author can say @return literal-string, and that can/should be tested by the library author via static analysis; but lets say they don't, or they ignore a mistake; developers using that library, when they use static analysis, because it's not checking anything inside the library, it just assumes the @return is correct (done for speed, and because issues in the library code are not for the user of the library to fix).

You can test the is_literal() implementation (created by Joe Watkins), by compiling PHP with this patch.

---

Ok, now to reply to your excellent points...

With your first example, The main issue is that assert() fails because the is_literal() function does not exist, and returns an error. But you're right, while PHPStan does complain when $notLiteralString is provided to the object via setText(), that is the end of checking that value, so when it comes to using getText(), PHPStan will simply assume that all the relevant checks have been done earlier, and $text will be seen as a literal-string.

Yep, you are correct. If the library has a method with @return literal-string, it will simply assume that's what will be returned. It requires every developer involved (all library authors, and the developers using the libraries) to run static analysis for their bit of code, at a high enough level, to not ignore these errors, and to not include them in their baseline. This is why the runtime is_literal() approach is the most reliable for everyone (it would be guaranteed by the PHP engine), but static analysis is much better at checking every possibility (i.e. all variable sources), and it's better at debugging (e.g. it can work backwards to identify the source of the problem).

As to developers using is_literal() and the literal-string type, while they are different, they are very similar and should work well together. In your example, try adding /** @return literal-string */ to your canTrustMe() method. That will tell PHPStan; 1, when this method is being checked it should always return a literal-string; and 2, that anyone using this method will be given a literal-string.

I've got a few examples, but my Basic SQL library shows how a library could use is_literal(); where it calls its own literal_check() method for any sensitive value it receives (works well with recursive array structures); it also allows developers using this library to choose the protection_level they want. I've also got a copy of this example with type information to show both approaches in use (I use the first one to focus on the runtime approach).

[craigfrancis]

I consider somehow that this method will be useful but need a lot of thought process before.

Both approaches are useful - Adding is_literal() to the language itself would ensure it's available to everyone; keeping in mind that my mission is to completely remove Injection Vulnerabilities from the OWASP Top 10 - it's the type of vulnerability that should (eventually) be completely prevented (details).

When you say "a lot of thought process", if you're thinking about library authors thinking about how their methods work, they should be keeping user values separate from sensitive strings already (the difficulties I've had with adding these checks is more about how static analysis tools work today).

IMHO sprintf has to be supported. For the 'sprintf('WHERE name = %s', $literalString) example.

Sorry, we did try this, but it started getting complicated, both in how it affected performance, and the security guarantees become much harder to prove. For example sprintf('%.3f', 1.23) with locale 'de_DE.UTF-8', returns '1,230' (a problem in the SQL context).

And maybe with corrects usecase, the is_literal check should support a lot of more manipulations. Like strtoupper, strtolower, ... it should return a literal-string.

Same again, we could create a massive list of functions, but the more you add, the bigger the performance impact, and the harder it is to say it's going to be secure (i.e. what are all of the inputs? how can these functions affect the output for the different contexts? what should actually go on the list? can user defined functions affect the flag? what about mistakes? etc).

I discussed this with quite a few security people (probably the main discussion was with Krzysztof Kotowicz about how Google did this in Go Safe HTML, and SafeHtml in the JavaScript Closure-Library), and we all came to the conclusion it's much easier to say a literal string must be defined in the source code, and (for the convenience of not needing to use a string builder) the only modification can be string concatenation.

Technically there is a way to intentionally create a vulnerability with string concatenation; where an evil developer takes unsafe user value, splits it into single 8 bit characters, and uses their own internal array of literal characters to build up a new string. Fortunately this cannot be done by accident, so the core principal is fine (i.e. it provides very good "guard rails" to ensure mistakes cannot be made).

The more I think about it, the more I think it's better with static analysis.

You're right, especially with debugging and checking, but even the JetBrains survey (which doesen't exactly reflect typical developers) suggests only 33% of developers use static analysis. Unfortunately static analysis is an optional thing (so many developer simply skip it), and as noted above, there are difficulties.

First the developer that will need this may not use the latest PHP version and even if they are using it, if they don't care about static analysis, they will not care either about adding some is_literal calls in their code.

They won't, it's for libraries to add (it's the libraries which know how to check and handle the inputs).

And you're right, static analysis is generally good for staying up to date (ish, the Symfony Twig developers wait a year before using new static analysis features), but it's also easy for libraries to use function_exists('is_literal') to skip the check.

Secondly, and my main concern about this method. It's easy to add a dynamicReturnType to phpstan library and create a new release. It's not that easy to change the is_literal behavior and creating a new php release.

By keeping the implementation really simple (string concatenation only), we won't need to worry about the behaviour changing; it's a very simple boolean flag that's only set to true for strings defined in the source code, and when concatenating them together. Due to the nature of these flags, they are off by default, so any other way of creating a new string, no matter how complicated, will not have the literal flag switched on.

Also we might ends with some code where phpstan is able to consider it as literal-string thanks to phpdoc/dynamicReturnType/... but which is too complex to process by the is_literal method to consider the same.

It's not a problem, the use of dynamic return types is for static analysis to better represent the dynamic nature of PHP; whereas the is_literal() method would be part of the language, and it's only a few locations where the flag can be switched on.

[VincentLanglet]

It's not a problem, the use of dynamic return types is for static analysis to better represent the dynamic nature of PHP; whereas the is_literal() method would be part of the language, and it's only a few locations where the flag can be switched on.

But it would say that the fact a string is considered as a literal-string by phpstan or psalm, doesn't imply that is_literal will return true.

IMHO sprintf has to be supported. For the 'sprintf('WHERE name = %s', $literalString) example.

Sorry, we did try this, but it started getting complicated, both in how it affected performance, and the security guarantees become much harder to prove. For example sprintf('%.3f', 1.23) with locale 'de_DE.UTF-8', returns '1,230' (a problem in the SQL context).

And maybe with corrects usecase, the is_literal check should support a lot of more manipulations. Like strtoupper, strtolower, ... it should return a literal-string.

Same again, we could create a massive list of functions, but the more you add, the bigger the performance impact, and the harder it is to say it's going to be secure

Let's say the alias of my query is accessible with a getAlias method from a doctrine library.
And I want to write select($this->getAlias() . '.myField').

If the phpdoc of getAlias is @return string, phpstan will complain.
If I add the check

if (!is_literal($query->getAlias)) {
     throw new \Exception();
}

this will solve the phpstan error, but my code may stop to work because the getAlias method is using sprintf method.
And if I write a stub to say that getAlias is returning a literal-string, this is not true. Because is_literal($query->getAlias()) === false.

I feel like with the current definition of literal-string, adding @param literal-string is way too restrictive then and provide situations without solutions.

I really like the idea of the literal-string feature. But currently I see so many false-positive on my project that I prefer to ignore all the literal-string errors in the baseline which was the opposite of the goal of the RFC.

[craigfrancis]

I'm trying to avoid false positives as much as possible, and I'm trying to work out how best to get this provably secure technique to work with static analysis tools, and existing libraries (I've only just got started).

My intention is to get the literal-string check working in phpstan-doctrine, then go back to the Doctrine developers to get them to use it (I made a mistake when discussing it with one of their core developers, where I said the final SQL could be checked as a literal, to give a perfect guarantee that no injection vulnerabilities could happen, but they thought it would require too many changes; I should have focused on Doctrine only checking its inputs, and assuming there are no mistakes in the library itself). If I can convince them, they might need to make some small changes to their code (so their checks pass), and that's why we (unable to change the library code itself) might need to lie with @return values (just for now).

As to sprintf(), you have to acknowledge it's not a function that simply concatenates strings, it already has the ability to coerce values, pad strings, change number precision, format numbers based on the locale... and who knows what its future holds (maybe the $format string could specify a function to do custom formatting, that might be useful; but hopefully not be like preg_replace /e modifier, fortunately removed in PHP 7.0).

Also, when you say getAlias() uses sprintf(), I cannot see that in Doctrine ORM?

[VincentLanglet]

Also, when you say getAlias() uses sprintf(), I cannot see that in Doctrine ORM?

It's not used so far. I was just trying to give an example about how easy a literal-string could stop being a literal-string, without introducing any security flaw.

[craigfrancis]

There are a few cases where a value looses the literal flag, but I think that's a good thing, even if they are probably safe.

I appreciate a list of exceptions might make adoption easier, but it moves away from the existing definition of what a "literal" in programming is, and I worry that any one of the exceptions could create a flaw (like how taint checking has the flaw in assuming an escaped value is safe)... it also risks developers falling into the trap of thinking this is safe vs unsafe for everything, which isn't the case; e.g. exec_param('rm -rf ?', $path) is completely safe from injection vulnerabilities, but has a different problem if $path can be any value from the user, e.g. '/' :-)

During the RFC discussion period, Scott Arciszewski (who knows a little bit about cryptography and security) suggested an idea they called is_noble() to support integers/floats (these cannot have a flag to say if they are developer defined or not, but they are almost certainly safe in regards to Injection Vulnerabilities), this approach and name was rejected by pretty much everyone (we tried so hard, but people just thought it was a messy concept)... if you wanted, you could put together a new type which had a list of exceptions. Personally I think it would become too complicated and risky, but maybe others would appreciate it being more relaxed?