Injection vulnerabilities

3 years ago by Craig Francis — view source

unread

Hi Internals,

Just something for you to think about, to start the conversation (as the
8.1 deadline is fast approaching).

Injection Vulnerabilities remain on the OWASP Top 10 List, same with XSS.

https://owasp.org/www-project-top-ten/2017/

These mistakes happen when user supplied values are included in command
strings - be that SQL, HTML, OS Commands (e.g. shell_exec), and dare I say
it eval().

It's easy to make these mistakes, especially when using a library (API)
that hides the implementation details.

I've got loads of examples (one of the joys of auditing), but have a think
about these 3 for now...

1, Let's consider an ORM (I'm using CakePHP this time). Parts of the
where() array must be defined by the programmer. Both of these examples
work, but the first one has a simple mistake that has introduced an SQL
Injection vulnerability:

$articles->find()->where(['id != ' . $_GET['id']]);

$articles->find()->where(['id != ' => $_GET['id']]);

https://book.cakephp.org/3/en/orm/query-builder.html

2, Maybe the programmer is writing the SQL themselves, and passes it on
to a basic database abstraction; but they don't realise the SQL string
should be entirely written by the programmer, using parameterised queries*:

$sql = 'SELECT * FROM table WHERE id IN (' . implode(',', $_POST['select'])
. ')';

Yes, I know, things like table names cannot use parameters, but they have
to be handled carefully as well, and will be covered.

3, How about a small HTML snippet, which should also be written entirely
by the programmer:

$html = "<img src=" . htmlentities($url) . " alt='' />";

Because escaping is hard (and sometimes completely forgotten), user values
should be added to HTML via a context-aware Templating Engine/Library (so
every one of the programmers using that well tested library don't have to
worry about making these mistakes all the time).

In this case, the src attribute value isn't quoted, and that kinda works,
but it can be exploited with $url = '/ onerror=alert(1)';

Just have a think about these issues, and note how they are all affected by
the same problem.

Craig

3 years ago by Craig Francis — view source

unread

Hi Internals,

To follow up on yesterdays post [1]...

We could try to teach programmers to never make a mistake (yep, you can
stop laughing).

Take the previous example:

$html = "<img src=" . htmlentities($url) . " alt='' />";

We might be able to teach everyone to always quote their attributes (or use
a different form of encoding):

$html = "<img src='" . htmlentities($url) . "' alt='' />";

And before PHP 8.1 [2] is released, teach them htmlentities doesn't encode
single quotes by default.

And teach them about dangerous things like '<a href="?">', due to
'javascript:' URLs... etc, etc.

The first part to solving this, use the Parameterised Queries idea from SQL

the programmer writes their HTML string, and keeps their HTML completely
separate from the user values. To combine, they provide both to a HTML
templating engine, that knows how to do appropriate escaping.

For example:

html("<img src=? alt='' />", [$url]);

Which can be done today, but it doesen't stop injection mistakes from
happening (the second part of this problem).

It's still trivial for a programmer to mistakenly include (inject) user
values into that first argument:

html("<img src='$url' alt='' />");

And note how this mistake is exactly same as the other examples, and doing
this with Laravel [3]:

DB::select('select * from users where active = ' . $_GET['active']);
// INSECURE

DB::select('select * from users where active = ?', [$_GET['active']]);

Craig

[1] https://externals.io/message/114540
[2]
https://github.com/php/php-src/commit/50eca61f68815005f3b0f808578cc1ce3b4297f0
[3] https://laravel.com/docs/8.x/database#running-a-select-query

3 years ago by Craig Francis — view source

unread

Fine, I'll finish with this...

We know that HTML [3] and SQL [4] should be written by the programmer, with
user data being handled separately.

The same applies to OS Commands:

$command = 'rm -rf ?';

Because we're using parameters (to escape the user values properly), we
don't need to consider injection vulnerabilities (yay).

But we must NEVER say these strings, written by a programmer, are "safe".

For example, while the use of parameters in $command does avoid an
injection vulnerability, it's still a big problem if a path to something
that shouldn't be deleted is used (insert classic rm -rf / joke here).

So we cannot say anything is "safe", but we can note something valuable
about a variable - was it written by the programmer.

Or, in other words, was this variable created from a literal (a string
defined in the PHP script).

Now you might say that we should use static analysis to identify
mistakes... yeah, most programmers do not (it's an extra step they can't be
bothered to do)... and even then, Psalm and PHPStan currently focus on
other issues (type checking, basic logic flaws, code formatting, etc)...
that said, Psalm does have a Taint Checking feature (run separately, and I
bet you're not using it), but Taint Checking is also flawed:

$sql = 'SELECT * FROM users WHERE id = ' . $db->real_escape_string($id);

$html = "<img src=" . htmlentities($url) . " alt='' />";

$html = "<a href='" . htmlentities($url) . "'>...";

The first two need the values to be quoted, but would be considered
"untained".

The third example, well htmlentities, before PHP 8.1 (thank you very much)
does not escape single quotes by default... not does it consider
'javascript:' URLs.

We need something that libraries will (in the future) be able to use to
protect themselves against these mistakes... by all programmers, especially
those who aren't using static analysis.

Craig

3 years ago by Guilliam Xavier — view source

unread

On Fri, May 21, 2021 at 11:21 PM Craig Francis craig@craigfrancis.co.uk
wrote:

[...]

We need something that libraries will (in the future) be able to use to
protect themselves against these mistakes... by all programmers, especially
those who aren't using static analysis.

Hi,

Not sure what kind of answer you expect... Are you suggesting to provide
one or both of:

a way to forbid "dynamic" strings (or at least detect them)?
"safe" HTML, SQL and OS-command builder/generator/executor APIs (that
would internally restrict/validate their "static" parts and quote/escape
the dynamic parameters)?

Regards,

--
Guilliam Xavier