# [RFC] Name issue - is_literal/is_trusted

- Thread: https://externals.io/message/115037
- Original archive: https://news-web.php.net/php.internals/115037
- Started by Craig Francis on 2021-06-22T18:11:05+00:00
- Messages: 39
- RFC: Is literal (Declined) — https://externals.io/rfc/is_literal.md
- Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL)
- Next page: https://externals.io/message/115037.md?page=2
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #115037 — Craig Francis — 2021-06-22T18:11:05+00:00

URL: https://externals.io/message/115037#115037

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

---

## #115040 — Mel Dafert — 2021-06-22T19:38:34+00:00

URL: https://externals.io/message/115037#115040 · In reply to: #115037

>- `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.

---

## #115041 — Craig Francis — 2021-06-22T19:47:12+00:00

URL: https://externals.io/message/115037#115041 · In reply to: #115040

> 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

---

## #115042 — Derick Rethans — 2021-06-22T19:50:45+00:00

URL: https://externals.io/message/115037#115042 · In reply to: #115037

>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

---

## #115044 — Kamil Tekiela — 2021-06-22T20:35:04+00:00

URL: https://externals.io/message/115037#115044 · In reply to: #115042

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

---

## #115057 — Guilliam Xavier — 2021-06-23T10:27:18+00:00

URL: https://externals.io/message/115037#115057 · In reply to: #115037

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

---

## #115059 — Hossein Baghayi — 2021-06-23T13:05:16+00:00

URL: https://externals.io/message/115037#115059 · In reply to: #115037

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:

> 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

---

## #115060 — Larry Garfield — 2021-06-23T13:37:11+00:00

URL: https://externals.io/message/115037#115060 · In reply to: #115059

> 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

---

## #115061 — Craig Francis — 2021-06-23T14:10:57+00:00

URL: https://externals.io/message/115037#115061 · In reply to: #115057

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.

---

## #115067 — Craig Francis — 2021-06-23T14:54:30+00:00

URL: https://externals.io/message/115037#115067 · In reply to: #115060

> 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

---

## #115075 — Scott Arciszewski — 2021-06-23T18:21:39+00:00

URL: https://externals.io/message/115037#115075 · In reply to: #115067

>
> 
>
> > […]
>
>
> 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

---

## #115087 — Bruce Weirdan — 2021-06-24T00:09:31+00:00

URL: https://externals.io/message/115037#115087 · In reply to: #115075

> - 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

---

## #115089 — Craig Francis — 2021-06-24T00:26:12+00:00

URL: https://externals.io/message/115037#115089 · In reply to: #115087

> > […]
>
> 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

---

## #115090 — Scott Arciszewski — 2021-06-24T00:41:46+00:00

URL: https://externals.io/message/115037#115090 · In reply to: #115087

>
> > […]
>
> 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.

---

## #115091 — Bruce Weirdan — 2021-06-24T01:22:53+00:00

URL: https://externals.io/message/115037#115091 · In reply to: #115090

> 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

---

## #115092 — Scott Arciszewski — 2021-06-24T01:30:58+00:00

URL: https://externals.io/message/115037#115092 · In reply to: #115091

> On Thu, Jun 24, 2021 at 3:41 AM Scott Arciszewski <scott@paragonie.com>
> wrote:
> > […]
>
> 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.
>
> > […]
>
> 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.

---

## #115093 — Stephen Reay — 2021-06-24T06:10:03+00:00

URL: https://externals.io/message/115037#115093 · In reply to: #115092

> 
> 
> 
> 
> > […]
> 
> 
> 
> 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

---

## #115096 — Scott Arciszewski — 2021-06-24T07:14:23+00:00

URL: https://externals.io/message/115037#115096 · In reply to: #115093

> Hi Scott,
>
> I wrote that example where an integer could be dangerous.

I don't disagree that it's an example where an integer could be dangerous.

Danger is too broad to have a meaningful discussion about. You can, of
course, always do dangerous things if you're determined or creative
enough.

> 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.

Agreed.

> 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 have a strong opinion on this.

> 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.

They're effectively synonyms.

> 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?

Yes, let me try to explain this more clearly.

If you can inject code, it's a code injection vulnerability. SQL
injection is a specific type of code injection vulnerability. If you
can trick the SQL into doing something invalid *without* injecting
code, and you call it "SQL injection", that's a category error.

Supplying an integer in the place of a column name is a bug. The bug
can even be dangerous. The bug can EVEN be a security problem for the
system.

But calling it SQL injection is categorically incorrect. Changing the
behavior of a SQL command **without injecting additional code** is not
SQL injection.

If we're trying to stop all forms of misuse and insecurity that can
result from string concatenation, cool. But be very clear about that.

> I did a little research, and it turns out Wikipedia (https://en.wikipedia.org/wiki/SQL_injection#Technical_implementations), Cloudflare (https://www.cloudflare.com/en-au/learning/security/threats/sql-injection/), and OWASP (https://owasp.org/www-community/attacks/SQL_Injection#example-2) 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?

If you **inject a `1=1` clause where one didn't exist before**, that's
injection. Notice the introduction of an OR operator in the examples
you cited.

My classification of the original example as "Not Injection" has
nothing to do with the fact that numbers are being compared with
numbers. Rather, there was no code injection.

> 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).

Snark is unnecessary.

> 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.

Why not two functions then?

- is_noble_string() -- more restrictive
- is_noble() -- YOLO

> Cheers
>
> Stephen

---

## #115097 — Scott Arciszewski — 2021-06-24T07:29:08+00:00

URL: https://externals.io/message/115037#115097 · In reply to: #115093

>
>
>
> 
>
> 
>
> 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), Cloudflare (https://www.cloudflare.com/en-au/learning/security/threats/sql-injection/), and OWASP (https://owasp.org/www-community/attacks/SQL_Injection#example-2) 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).
>
>
>
> 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
>

One other thing. What do you think the result of this code will be?
How many rows do you think it will spit out?

```
<?php
$db = new PDO('sqlite::memory:');

// These are the only reasonable settings to ever use:
$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

// Let's create a table:
$db->exec("CREATE TABLE foo (username TEXT, pwhash TEXT, email TEXT)");

// Dummy rows:
$db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('scott',
'lolno', 'scott@paragonie.com')");
$db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('robyn',
'fake!', 'robyn@paragonie.com')");

// Okay, now let's do a prepared statement, with an integer field
$int = 12345;

$stmt = $db->prepare("SELECT * FROM foo WHERE {$int} = ?");
$stmt->execute([12345]);
var_dump($stmt->fetchAll(PDO::FETCH_ASSOC));
```

---

## #115100 — Guilliam Xavier — 2021-06-24T08:34:18+00:00

URL: https://externals.io/message/115037#115100 · In reply to: #115096

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

> On Thu, Jun 24, 2021 at 2:10 AM Stephen Reay <php-lists@koalephant.com>
> wrote:
>
> > […]
> 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.
>
> Why not two functions then?
>
> - is_noble_string() -- more restrictive
> - is_noble() -- YOLO
>

I was going to ask basically the same [with different names] a few days ago
("why can't we have both?"), but then remembered
https://externals.io/message/114835#114951 , esp. the end:

"""
And to support having 2 functions, we would need 2 flags on strings. These
flags are limited, and managing 2 flags would affect performance.
"""

Regards,

-- 
Guilliam Xavier

---

## #115101 — Scott Arciszewski — 2021-06-24T09:19:17+00:00

URL: https://externals.io/message/115037#115101 · In reply to: #115100

On Thu, Jun 24, 2021 at 4:34 AM Guilliam Xavier
<guilliam.xavier@gmail.com> wrote:
>
>
> 
> > […]
>
>
> I was going to ask basically the same [with different names] a few days ago ("why can't we have both?"), but then remembered https://externals.io/message/114835#114951 , esp. the end:
>
> """
> And to support having 2 functions, we would need 2 flags on strings. These
> flags are limited, and managing 2 flags would affect performance.
> """
>
> Regards,
>
> --
> Guilliam Xavier

Thanks for the reference to that part of the discussion that I missed.

Aside:

I encourage everyone to look at EasyDB (especially EasyStatement) for
handling `WHERE x IN (a, b, c, ...)` statements in SQL.
https://github.com/paragonie/easydb

Additionally, Ionizer is useful for input filtering and asserting type
safety: https://github.com/paragonie/ionizer

If you're doing dynamic, on-the-fly SQL query generation (based, in
part, on user input), these are two framework-agnostic tools that can
help make your code safer against code injection and other attacks.

---

## #115102 — Stephen Reay — 2021-06-24T09:22:35+00:00

URL: https://externals.io/message/115037#115102 · In reply to: #115096

> 
> 
> 
> > […]
> 
> I don't disagree that it's an example where an integer could be dangerous.
> 
> Danger is too broad to have a meaningful discussion about. You can, of
> course, always do dangerous things if you're determined or creative
> enough.
> 
> > […]
> 
> Agreed.
> 
> > […]
> 
> I don't have a strong opinion on this.
> 
> > […]
> 
> They're effectively synonyms.

I mean, they’re not - you can prepare a statement with zero parameters in it. 
But my point was why you even mentioned parameterised queries OR prepared statements at all. You cannot parameterise the column name, hence the entire reason this RFC exists. The entire point is to make sure that the bits that you _need_ to insert variables, are something the developer wrote, not some input from the user.

If that isn’t the purpose of the RFC, then it describes its actual goal really poorly.

> 
> > […]
> 
> Yes, let me try to explain this more clearly.
> 
> If you can inject code, it's a code injection vulnerability. SQL
> injection is a specific type of code injection vulnerability. If you
> can trick the SQL into doing something invalid *without* injecting
> code, and you call it "SQL injection", that's a category error.
> 
> Supplying an integer in the place of a column name is a bug. The bug
> can even be dangerous. The bug can EVEN be a security problem for the
> system.
> 
> But calling it SQL injection is categorically incorrect. Changing the
> behavior of a SQL command **without injecting additional code** is not
> SQL injection.
> 
> If we're trying to stop all forms of misuse and insecurity that can
> result from string concatenation, cool. But be very clear about that.
> 
> > […]
> 
> If you **inject a `1=1` clause where one didn't exist before**, that's
> injection. Notice the introduction of an OR operator in the examples
> you cited.

Please, explain to us all, how `where foo=‘bar’ OR 1=1` is functionally different than `where 123=123` because the developer screwed the pooch in some specific condition and passed the ID twice, rather than the column name and the id, respectively.

> 
> My classification of the original example as "Not Injection" has
> nothing to do with the fact that numbers are being compared with
> numbers. Rather, there was no code injection.
> 
> > […]
> Snark is unnecessary.

You call it snark, I call it ironic hypocrisy.

> 
> > […]
> 
> Why not two functions then?
> 
> - is_noble_string() -- more restrictive
> - is_noble() -- YOLO

I mean sure, if someone wants to vote on the two choices or whatever, I’ll go get the popcorn, but I still haven’t seen any answers that make sense to this question:  Why integers at all, though? 


> 
> > […]
> 
> --
> 
> To unsubscribe, visit: https://www.php.net/unsub.php

---

## #115104 — Stephen Reay — 2021-06-24T09:55:40+00:00

URL: https://externals.io/message/115037#115104 · In reply to: #115097

> 
> 
> 
> > […]
> 
> One other thing. What do you think the result of this code will be?
> How many rows do you think it will spit out?
> 
> ```
> <?php
> $db = new PDO('sqlite::memory:');
> 
> // These are the only reasonable settings to ever use:
> $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
> $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
> 
> // Let's create a table:
> $db->exec("CREATE TABLE foo (username TEXT, pwhash TEXT, email TEXT)");
> 
> // Dummy rows:
> $db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('scott',
> 'lolno', 'scott@paragonie.com')");
> $db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('robyn',
> 'fake!', 'robyn@paragonie.com')");
> 
> // Okay, now let's do a prepared statement, with an integer field
> $int = 12345;
> 
> $stmt = $db->prepare("SELECT * FROM foo WHERE {$int} = ?");
> $stmt->execute([12345]);
> var_dump($stmt->fetchAll(PDO::FETCH_ASSOC));
> ```
> 


Two things: 
(a) try that same query, unaltered on mysql. It won’t be 0 rows. I’m pretty sure its also not 0 on Postgres.

(b) try binding the parameter as an integer and running it on SQLite. It won’t be zero rows. Actually I can even show you that one: https://3v4l.org/VL7UC#focus=8.0.7 <https://3v4l.org/VL7UC#focus=8.0.7>


So what do we call the function now: is_trusted_string_or_integer_but_dont_dare_use_the_int_as_an_int_or_with_mysql_at_all() ?


And, this horse is practically glue based on how much I’m beating it, but still I have to keep asking: Why integers at all? Who is using “2” or some other digit-only identifier as a table or column name, but also _doesn’t_ define that name as a string?

---

## #115105 — Kamil Tekiela — 2021-06-24T10:07:09+00:00

URL: https://externals.io/message/115037#115105 · In reply to: #115104

Hi Stephen,

I believe the idea was for dynamically generate table names, or numbered
tables/columns. E.g.

function getTable(string $table){
    // is_literal check here
}

$number = (int) $_GET['tableno'];
if($number < 0 || $number > 10) {
    throw new Exception("Invalid number");
}

$tablename = 'table_'.$number;
getTable($tablename);

The number is concatenated to the table name.

--Kamil

---

## #115106 — Craig Francis — 2021-06-24T10:16:08+00:00

URL: https://externals.io/message/115037#115106 · In reply to: #115104

> but still I have to keep asking: Why integers at all?
>


While I'm not a fan of this approach, there is a lot of existing code and
tutorials that use:

$sql = 'WHERE id IN (' . implode(',', array_map('intval', $ids)) . ')';

$sql = sprintf('SELECT * FROM table WHERE id = %d;', intval($id));

Craig

---

Next page: https://externals.io/message/115037.md?page=2