Everything

php

Home » Project » Newsgroups » php.internals » [RFC] Name issue - is_literal/is_trusted »

[RFC] Name issue - is_literal/is_trusted

php.internals

Craig Francis

5 years ago

Hi Internals,

As the name `is_trusted()` seems to be causing contention, I think more
than the alternative option would. Since we want to get this right, and we
still have time before the feature freeze, this might be worth re-looking
at. Particularly if you are one of those unsure about it, read on.



The name `is_trusted()` was chosen by a community vote over several days.
While I’m of a similar opinion that "trusted" might be misleading for some
in the strength of its word, I do not want to simply override 18 of the 21
people, who I assume read the RFC, asked questions to clarify on the
mailing list, understood how it works, and have chosen that name.

However, clearly some people missed the vote and its discussion time, and
some voted but then perhaps wanted clarifying on what the RFC was fully
about later. If we say that's about five people, then assuming there is a
larger audience who reads but does not post (as the voting numbers
indicated) then I'm inclined to guesstimate that maybe that means 3x the
number of people share those feelings. And with that number it starts to
feel like maybe we should double-check here.

While a one-word name is always going to be misunderstood by some people,
we want to be as clear as possible.

The Function:
- Is a security-based function that prevents Injection Vulnerabilities in
PHP.
- Flags strings written by the developer, including when concatenated.
- Also accepts integer values, which as purely numerical cannot contain
code/dangerous characters. (Due to technical limitations within PHP, it's
not possible for these to be flagged as user or developer in the codebase
itself without performance issues).

(RFC for full breakdown: https://wiki.php.net/rfc/is_literal)

Ideally we want a one-word name that suggests this as best we can - one
word to be consistent with other `is_*()` functions.

- `is_literal()` was my original placeholder name. However, given that it's
not just literal strings, but also supports concatenation and integers I
felt it may be misleading with the definition of 'literal' stretched so far
it might get confusing, and is why I didn't include my original name for it
in the poll. However, if you feel it would be more accurate my mind isn't
fixed on it.

- `is_known()` - suggested by Joe, who created the implementation, was one
of two options in the original vote, and was based on the principle that
the value be 'known' to the developer to be free from external code and be
within a 'known' understanding of the values that should be going in it.

- `is_trusted()` - suggested by Moritz and separately by Mike, was the
second option in the original vote, and was based on the idea that what is
returned can be 'trusted' to be free from external code.

I suggest that people who are serious in their feelings about this, offer
the name that they would prefer (including potentially making one
themselves that fits the RFC content and style mentioned above) so we can
assess whether the current name needs a second look.

Thanks,
Craig

Mel Dafert

5 years ago

>- `is_trusted()` - suggested by Moritz and separately by Mike, was the
>second option in the original vote, and was based on the idea that what is
>returned can be 'trusted' to be free from external code.

Another idea - `is_internal()`, since it is not external code, and internal would be the
opposite of external.
Not quite sure how helpful/realistic this one is, but I wanted to bring it up either
way.

Craig Francis

5 years ago

On Tue, 22 Jun 2021 at 20:38, Mel Dafert <mel@dafert.at> wrote:

> Another idea - `is_internal()`, since it is not external code, and
> internal would be the
> opposite of external.

Unfortunately, because we cannot record internal vs external integers (too
big of a change to how integers are stored), we are currently allowing all
integers, as that helps adoption, without affecting security.

Craig

Derick Rethans

5 years ago

On 22 June 2021 19:11:05 BST, Craig Francis <craig@craigfrancis.co.uk> wrote:

>Hi Internals,
>
>As the name `is_trusted()` seems to be causing contention, I think more
>than the alternative option would. Since we want to get this right, and
>we
>still have time before the feature freeze, this might be worth
>re-looking
>at. Particularly if you are one of those unsure about it, read on.

Just changing the name to something not misleading isn't going to change my opinion about voting for this. It's grown into a concoction of different things than the original is_literal, that just checks a single string for its literalness.

cheers,
Derick

Kamil Tekiela

5 years ago

FWIW, I would prefer is_literal, but without integers in scope. Modern code
is often type hinted. I would expect that a lot of libraries would accept
only strings and then check whether it's a literal string. I don't think
accepting integers in scope increases security or improves ease of use.
However, I am more concerned about other things that have been mentioned in
this thread and the overall disagreement. I'd rather not rush this change.
Also, I am concerned about the performance of applications that are heavy
on string processing. Although I haven't tested this yet myself.

--Kamil

Guilliam Xavier

5 years ago

On Tue, Jun 22, 2021 at 8:11 PM Craig Francis <craig@craigfrancis.co.uk>
wrote:

>
> The Function:
> - Is a security-based function that prevents Injection Vulnerabilities in
> PHP.
> - Flags strings written by the developer, including when concatenated.
> - Also accepts integer values, which as purely numerical cannot contain
> code/dangerous characters. (Due to technical limitations within PHP, it's
> not possible for these to be flagged as user or developer in the codebase
> itself without performance issues).
>

- `is_safe_from_injections()`?
- `is_secure_against_injections()`?
- `can_be_trusted_to_not_contain_injection_vulnerabilities()`? (okay not
this one)

Alternatively, if integers are too controversial, how about reverting the
implementation to `is_literal()` but provide a function like
`to_literal(int $int): string` (or just a "polyfill" for userland, could be
a one-liner `implode(array_map(fn ($c) =>
['0','1','2','3','4','5','6','7','8','9','-'=>'-'][$c],
str_split((string)$int)))`), so that those `implode(',', [1,2,3])` could
use `implode(',', array_map('to_literal', [1,2,3]))`?

Regards,

--
Guilliam Xavier

Craig Francis

5 years ago

On Wed, 23 Jun 2021 at 11:27 am, Guilliam Xavier <guilliam.xavier@gmail.com>
wrote:

> Alternatively, if integers are too controversial, how about reverting the
> implementation to `is_literal()`
>

Starting to look like that, yeah.

Hossein Baghayi

5 years ago

Hello,
What about `is_vulnerable`? Its behaviour would be the inverse of
is_literal.
I mean we don't have to avoid the other side of the coin.

On Tue, 22 Jun 2021 at 22:41, Craig Francis <craig@craigfrancis.co.uk>
wrote:

Larry Garfield

5 years ago

On Wed, Jun 23, 2021, at 8:05 AM, Hossein Baghayi wrote:

> Hello,
> What about `is_vulnerable`? Its behaviour would be the inverse of
> is_literal.
> I mean we don't have to avoid the other side of the coin.

That has the same core problem as is_trusted.  It's making a claim about the probable security status of a value, which I promise you, you will not get right 100% of the time.  is_literal, is asserting only that the value came from the source code originally, not from user input.  That is something you can assert one way or another and be guaranteed correct.  What the *implications* are for what you can then do with it are an entirely separate, and highly squishy and use-case-specific, question.

I'm still very torn on is_literal; I fear that the people who would benefit from it are the very people that don't use the tools that would leverage it (DBALs et al), and so the net benefit will be small, but misuse of it will make DBALs weaker and less able to handle the highly-dynamic cases that I am used to working with.  I may be convinced of is_literal if the major DBAL authors back it, but I'm still not sure.

I am definitely -1 on is_trusted or any other claim of fit-for-purpose, rather than a claim of origin.  That's guaranteed to be incorrect often enough that it makes things worse rather than better.

--Larry Garfield

Craig Francis

5 years ago

On Wed, 23 Jun 2021 at 14:37, Larry Garfield <larry@garfieldtech.com> wrote:

> I'm still very torn on is_literal; I fear that the people who would
> benefit from it are the very people that don't use the tools that would
> leverage it (DBALs et al), and so the net benefit will be small.
>

This RFC will not help those who aren’t using libraries (DBALs), that’s
something we could look at in the future (I have a suggestion in the Future
Scope section, but whatever that involves, it will need to use this flag,
so it would need to be in place first).

But - and why I’m here - my experience is that it’s still a big issue for
those who *do* use libraries. It frequently comes up at software
agencies/companies that maintain their code (built with free
libraries/frameworks), and employ junior developers (i.e. the cheapest),
who make many "quick edits" (time is money), and in doing so introduce the
issues the RFC covers (and not to say we more experienced coders don’t
occasionally make mistakes too!). While non-library users are the main
cause, the library users are still a big part of why Injection
Vulnerabilities remain at the top of the OWASP Top 10.

Craig

Scott Arciszewski

5 years ago

On Wed, Jun 23, 2021 at 10:54 AM Craig Francis <craig@craigfrancis.co.uk> wrote:

>
> On Wed, 23 Jun 2021 at 14:37, Larry Garfield <larry@garfieldtech.com> wrote:
>
> > I'm still very torn on is_literal; I fear that the people who would
> > benefit from it are the very people that don't use the tools that would
> > leverage it (DBALs et al), and so the net benefit will be small.
> >
>
>
> This RFC will not help those who aren’t using libraries (DBALs), that’s
> something we could look at in the future (I have a suggestion in the Future
> Scope section, but whatever that involves, it will need to use this flag,
> so it would need to be in place first).
>
> But - and why I’m here - my experience is that it’s still a big issue for
> those who *do* use libraries. It frequently comes up at software
> agencies/companies that maintain their code (built with free
> libraries/frameworks), and employ junior developers (i.e. the cheapest),
> who make many "quick edits" (time is money), and in doing so introduce the
> issues the RFC covers (and not to say we more experienced coders don’t
> occasionally make mistakes too!). While non-library users are the main
> cause, the library users are still a big part of why Injection
> Vulnerabilities remain at the top of the OWASP Top 10.
>
> Craig

Hi,

I was asked for my thoughts on this RFC (and its naming) from a
security engineering perspective.

The old is_literal() isn't correct if it includes non-string values
(i.e. integers).
The new is_trusted() is potentially misleading, especially to people
who don't read the docs.

My knee-jerk reaction was simply, "Why not is_untainted()?" but that
invokes the imagery of taint-checking, which the RFC explicitly
doesn't implement. A better name might be is_noble(), where we get to
define the concept of noble inputs (name inspired by the Noble gases
from Chemistry).

The main reason I don't like is_trusted() is that everyone's threat
model and risk tolerance is different, and trust is too nebulous a
concept for a built-in function. But also, it's really easy to jump
the guard-rail: https://3v4l.org/4GM8Q#focus=rfc.literals

One concern that Joe Watkins asked about is: Is it reasonable to cover
both integers and strings that are not influenceable by potential
external attackers (n.b. ones that can't already overwrite your source
code)? Outside the chr()/pack()/sprintf()/etc. technique demonstrated
above, there's no risk of injection inherent to concatenating a
trusted string with an untrusted integer.

Injection attacks (SQL injection, LDAP injection, XSS, etc.) are, at
their core, an instance of type confusion between data and code. In
order for the injection to *do* anything, it needs to be in the same
input domain as the language the code is written in. Try as you might,
there is no integer that will, upon concatenation with a string,
produce a control character for HTML (i.e. `>`) or SQL (i.e. `'`).

Therefore, if the concern is Injection attacks, integer inputs do not
need to be tracked to provide a security gain.

This is only true for integers, not all numeric types. I haven't
investigated the safety of floats in every possible context, and the
`e`, `+`, and `.` characters could be problematic in corner cases.

TL;DR
- Why not is_noble()?
- String + int concatenation isn't an injection risk.

Cheers,
Scott

Weirdan

5 years ago

> - String + int concatenation isn't an injection risk.

I think this demonstrates it very well could be:
https://externals.io/message/114988#115038

--
Best regards,
Bruce Weirdan                                     mailto:weirdan@gmail.com

Craig Francis

5 years ago

On Thu, 24 Jun 2021 at 1:09 am, Bruce Weirdan <weirdan@gmail.com> wrote:

> > - String + int concatenation isn't an injection risk.
>
> I think this demonstrates it very well could be:
> https://externals.io/message/114988#115038

That’s the developer choosing to use a variable, and it’s no different than
the developer using a library to add the value via proper quoting/escaping.

Craig

Scott Arciszewski

5 years ago

On Wed, Jun 23, 2021 at 8:09 PM Bruce Weirdan <weirdan@gmail.com> wrote:

>
> > - String + int concatenation isn't an injection risk.
>
> I think this demonstrates it very well could be:
> https://externals.io/message/114988#115038
>
> --
>   Best regards,
>       Bruce Weirdan                                     mailto:weirdan@gmail.com

Respectfully, the example you linked is **not** an example of an
Injection vulnerability. The failure condition of this query is
"return all rows from the table already being queried", not "return
arbitrary data the attacker selects from any table that the
application can read".

Being able to arbitrarily select a column is a bad design (and you
should feel bad, as per the meme, if you let this happen in
production), but it differs from Injection vulnerabilities in one
fundamental way: The attacker cannot change the structure of the SQL
query being executed.

Here's an example of an injection vulnerability:

`$pdo->prepare("SELECT b, c, d, e FROM table WHERE a = '$foo'");`

If you set $foo to `' UNION SELECT NULL, NULL, NULL, pwhash FROM
accounts WHERE username = 'Admin`, you'll leak contents from *another
table* in the SQL result. This is the danger posted by
string-to-string concatenation, and what we mean by SQL Injection.

This doesn't have to stop all dumb things that PHP developers can do.
It's enough to only stop the catastrophically dumb things (especially
if we don't call the function `is_trusted()`).

You can still invent scenarios where int-to-string concatenation
results in buggy behavior, but it isn't the game-over security
vulnerability that string-to-string concatenation is. And that's the
entire point.

Weirdan

5 years ago

On Thu, Jun 24, 2021 at 3:41 AM Scott Arciszewski <scott@paragonie.com> wrote:

> The failure condition of this query is
> "return all rows from the table already being queried", not "return
> arbitrary data the attacker selects from any table that the
> application can read".

Imagine that was a DELETE rather than SELECT and the failure condition
becomes 'the table is emptied'.
It may have less disastrous consequences (depending on how good your
backup / restore procedures are) compared to arbitrary reads you
demonstrated, but it is still, quite clearly, a glaring security hole
caused by user input in SQL query - AKA SQL injection in layman's
terms.

> it differs from Injection vulnerabilities in one
> fundamental way: The attacker cannot change the structure of the SQL
> query being executed.

I would say replacing a column name with value is changing the
structure of SQL query, and, basically, in exactly the way you
describe SQL injection: confusing the code (column name) with data.

I wholeheartedly welcome this RFC as it was originally proposed:
is_literal() doing exactly what it says on the tin, without any
security claims. But it has gone far from there real quick and now
people can't even name the thing.

--
Best regards,
Bruce Weirdan                                     mailto:weirdan@gmail.com

Scott Arciszewski

5 years ago

On Wed, Jun 23, 2021, 9:23 PM Bruce Weirdan <weirdan@gmail.com> wrote:

> On Thu, Jun 24, 2021 at 3:41 AM Scott Arciszewski <scott@paragonie.com>
> wrote:
> > The failure condition of this query is
> > "return all rows from the table already being queried", not "return
> > arbitrary data the attacker selects from any table that the
> > application can read".
>
> Imagine that was a DELETE rather than SELECT and the failure condition
> becomes 'the table is emptied'.
> It may have less disastrous consequences (depending on how good your
> backup / restore procedures are) compared to arbitrary reads you
> demonstrated, but it is still, quite clearly, a glaring security hole
> caused by user input in SQL query - AKA SQL injection in layman's
> terms.
>
> > it differs from Injection vulnerabilities in one
> > fundamental way: The attacker cannot change the structure of the SQL
> > query being executed.
>
> I would say replacing a column name with value is changing the
> structure of SQL query, and, basically, in exactly the way you
> describe SQL injection: confusing the code (column name) with data.
>
> I wholeheartedly welcome this RFC as it was originally proposed:
> is_literal() doing exactly what it says on the tin, without any
> security claims. But it has gone far from there real quick and now
> people can't even name the thing.
>
>
> --
>   Best regards,
>       Bruce Weirdan                                     mailto:
> weirdan@gmail.com

We can agree that it is a bug. We don't agree on the definition of SQL
injection.

Changing a column name to a number (which prepared statements shouldn't
allow in the first place) is a bug. This changes the effect of the command,
but the *structure* of the query remains unchanged.

Stephen Reay

5 years ago

> On 24 Jun 2021, at 08:30, Scott Arciszewski <scott@paragonie.com> wrote:
>
> On Wed, Jun 23, 2021, 9:23 PM Bruce Weirdan <weirdan@gmail.com <mailto:weirdan@gmail.com>> wrote:
>
>> On Thu, Jun 24, 2021 at 3:41 AM Scott Arciszewski <scott@paragonie.com>
>> wrote:
>>> The failure condition of this query is
>>> "return all rows from the table already being queried", not "return
>>> arbitrary data the attacker selects from any table that the
>>> application can read".
>>
>> Imagine that was a DELETE rather than SELECT and the failure condition
>> becomes 'the table is emptied'.
>> It may have less disastrous consequences (depending on how good your
>> backup / restore procedures are) compared to arbitrary reads you
>> demonstrated, but it is still, quite clearly, a glaring security hole
>> caused by user input in SQL query - AKA SQL injection in layman's
>> terms.
>>
>>> it differs from Injection vulnerabilities in one
>>> fundamental way: The attacker cannot change the structure of the SQL
>>> query being executed.
>>
>> I would say replacing a column name with value is changing the
>> structure of SQL query, and, basically, in exactly the way you
>> describe SQL injection: confusing the code (column name) with data.
>>
>> I wholeheartedly welcome this RFC as it was originally proposed:
>> is_literal() doing exactly what it says on the tin, without any
>> security claims. But it has gone far from there real quick and now
>> people can't even name the thing.
>>
>>
>> --
>>  Best regards,
>>      Bruce Weirdan                                     mailto:
>> weirdan@gmail.com
>
>
>
> We can agree that it is a bug. We don't agree on the definition of SQL
> injection.
>
> Changing a column name to a number (which prepared statements shouldn't
> allow in the first place) is a bug. This changes the effect of the command,
> but the *structure* of the query remains unchanged.

Hi Scott,

I wrote that example where an integer could be dangerous.

So firstly - just to clarify, because some replies seemed to be confused on the topic, as was literally mentioned in the original comment, it is definitely not correct behaviour - it’s a developer mistake that might work some of the time, and thus go unnoticed in testing. If you can show me a developer who’s never inadvertently passed the wrong parameter in some condition, I’ll show you an imaginary developer.

Additionally - pointing out that this is a "developer error” doesn’t help your case. Using non-parameterised queries should already be a “developer error” for anyone who can walk and breathe at the same time - and yet that usage is being actively encouraged if this function supports integers. I’ve still seen zero responses about legitimate reasons this needs to support integers - giving people a shitty way to build an IN() clause is not legitimate. Parameterisation exists, and works.

I don’t even understand why you mentioned prepared statements (I guess you meant using parameterised queries?) - the column name inherently can’t be parameterised - hence having to use a string substitution in the query.

That part was weird and confusing, but not as odd as your claim that altering the query, so that it causes the where clause to become moot, is not an SQL Injection? REALLY? That’s your claim?

I did a little research, and it turns out Wikipedia (https://en.wikipedia.org/wiki/SQL_injection#Technical_implementations <https://en.wikipedia.org/wiki/SQL_injection>), Cloudflare (https://www.cloudflare.com/en-au/learning/security/threats/sql-injection/ <https://www.cloudflare.com/en-au/learning/security/threats/sql-injection/>), and OWASP (https://owasp.org/www-community/attacks/SQL_Injection#example-2 <https://owasp.org/www-community/attacks/SQL_Injection>) all have examples with a `1=1` type query manipulation. Do you want to write and tell them that they’re all wrong, or should I ask them to call you?

Also, while researching the specifics of what is considered an “SQL Injection” I came across an article, that talks specifically about the dangers of allowing user input (i.e. the thing `is_trusted` is meant to prevent) as a column or table identifier. It’s from this little organisation, you may have heard of them: “Paragon Initiative” (https://paragonie.com/blog/2015/05/preventing-sql-injection-in-php-applications-easy-and-definitive-guide <https://paragonie.com/blog/2015/05/preventing-sql-injection-in-php-applications-easy-and-definitive-guide>).

I would absolutely make use of a function that tells me if the string given is in fact from something controlled by the developer. But once that same string can also include input from the request or the environment or whatever by nature of integers, the function becomes useless for the stated purpose.

Cheers

Stephen