Comments (5)
@craigfrancis do you agree that these are cases we should report errors for, as they hide errors in wrong escaping?
do other php apis have the same problem?
I checked PDO, but PDO::quote
already wrapps the returned value in single-quotes, so at least the mentioned examples shouldn't be a problem for PDO.
from phpstan-dba.
I think I have seen similar code like this in my life somewhere:
wrong (sql injection vulnerable):
$query = sprintf('SELECT CountryCode FROM City WHERE name="%s"',
addslashes($mysqli, $city)); // invalid escaping method
$query = sprintf('SELECT CountryCode FROM City WHERE name="%s"',
addcslashes($city)); // invalid escaping method
from phpstan-dba.
Yep, missing quotes is a common problem... a code base with many thousands of values being escaped, and all it takes is 1 mistake.
The mysqli_real_escape_string()
function is dangerous, developers learn about escaping, and they use it, and think they are now safe... it's one of the main reasons why I keep pushing developers to use database abstractions, or parameterised queries (both still need to use the literal-string
type to check mistakes haven't been made).
It's even more fun when developers try escaping field names, e.g. $sql .= ' ORDER BY `' . $db->real_escape_string($field) . '`'
(and yes, I've seen it without the backtick quotes as well).
As to similar API's...
- PDO, yes, you're correct,
PDO::quote
is much safer because it adds the quotes itself, but I will note the manual says "you are strongly recommended to use PDO::prepare() to prepare SQL statements with bound parameters". - PostgreSQL, the
pg_escape_string()
has the missing quotes issue, and you also need to use single quotes as well (double quotes don't get escaped, source), that's why you should at least usepg_escape_literal()
... or, please, just use parameterised queries :-) - MSSQL, the
mssql_*
functions were removed in PHP 7, but I seem to remember it didn't have a string escaping function, I believe they were trying to get everyone to use parameterised queries. Unfortunately I've seen developers make their own, fromstr_replace('"', '""', str_replace("'", "''", $string))
to using a hex bytestring. - SQLSRV, the replacement for MSSQL, that also pushes developers to use prepared queries.
- HTML encoding has a similar issue
"<img src=" . htmlentities($url) . " alt='' />"
, where $url could be set to'/ onerror=alert(1)'
(and yes, it does not considerjavascript:
urls in<a href="">
). - Fortunately
escapeshellarg()
does add single quotes. - And when I find
addslashes()
being used incorrectly, I should put a part of those invoice payments into a holiday fund, and go on a lot more holidays in it's (dis)honor.
And when you say you have seen similar code before, we kinda discussed this last year, ref issue 316.
In your examples, a name=%s
is most likely going to be picked up fairly quickly (usually an SQL parse error), I tend to find this mistake takes the form of id=%s
, because a number is normally being passed to it, and that works ™.
Extra bonus point for parameterised queries... the developer defined SQL is sent to the database first, the query execution plan is created from it (sometimes that can be cached)... then the user values are accepted/used; this means the user values cannot have any affect on the query execution plan, so even if there was an implementation issue, it's far too late to change it... in contrast, (escaped) user values are mixed in with the SQL, and separating them again is tricky.
from phpstan-dba.
thanks. I agree that prepared statements is a must have. still there is a lot of vulnerable code out there, where I want to point out security issues.
I will add a "phpstan-tip" to the rule that escaping should be prevented. you had a website somewhere, where all the problems and solutions are described in detail, right?
from phpstan-dba.
That works.
As to the website, yep, although it's more of a quick summary (so people are more likely to read): https://eiv.dev/
It does link to pages on my GitHub repo with some typical examples:
https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification/escaping.php
https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification/mistakes.php
And yes, there is a lot of vulnerable code out there, and why I'm so glad you're tackling it from another angle... hopefully, with enough people working on this problem, we can stop boring injection vulnerabilities. and I can move on to more interesting problems :-)
from phpstan-dba.
Related Issues (20)
- AST: inconsistent behavior with functions and aliases HOT 4
- Analyzing multiple connections HOT 6
- Right vs left join HOT 2
- AST: generic operator support HOT 2
- pgsql support for AST HOT 2
- give up sql based narrowing in case sqlftw cannot parse the query
- pgsql support for uuid type
- Support for (PDO->prepare())->execute() HOT 6
- generic type mysqli_result error with phpstan v1.10.36 and v1.10.37 HOT 2
- Support for custom API's for type inference HOT 1
- Check driver differences
- dibi support DATE_FORMAT
- Psalm support
- Using with doctrine/dbal HOT 8
- CI: Separate Testing of Doctrine 3.x and 4.x
- Issue with update to 0.2.80: Query expects 0 placeholder, but 1 value is given HOT 3
- Enabling `staabm/phpstan-dba` significantly increases garbage collection activity HOT 9
- Querying the column information fails when sql mode is set to ANSI_QUOTES HOT 1
- Uncaught UnexpectedValueException: RecursiveDirectoryIterator::__construct(//proc/tty/driver) HOT 2
- Internal error: Unexpected expression type iterable<int>
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from phpstan-dba.