Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Aleksander Machniak]

On 23.06.2021 16:40, Sara Golemon wrote:
> Seriously. What about this looks spammy, I ask you?

I don't know the other, but this one has

X-Spam-Status: No, score=1.147 tagged_above=-999 required=4.5
tests=[BAYES_00=-1.9, GUARANTEED_100_PERCENT=2.699,
MAILING_LIST_MULTI=-1, RCVD_IN_BL_SPAMCOP_NET=1.347,
RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001,
RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001,
URIBL_BLOCKED=0.001] autolearn=no autolearn_force=no

-- 
Aleksander Machniak
Kolab Groupware Developer[https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]

PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Sara Golemon]

Resending this, because the mail daemon sent it back as spam, and we
shouldn't be running our own mail server any more than we should have been
running our own git server.
Seriously. What about this looks spammy, I ask you?

On Wed, Jun 23, 2021 at 9:36 AM Sara Golemon  wrote:

> On Mon, Jun 21, 2021 at 6:29 PM Craig Francis 
> wrote:
> > On Tue, 22 Jun 2021 at 12:18 am, Benjamin Morel <
> benjamin.mo...@gmail.com> wrote:
> > > On Tue, 22 Jun 2021 at 01:06, Derick Rethans  wrote:
> > >> On 21 June 2021 23:37:56 BST, Yasuo Ohgaki 
> wrote:
> > >> >
> > >> >The name "is_trusted" is misleading.
> > >> >Literal is nothing but literal.
> > >>
> > >> I agree with this. The name is_trusted is going to be the same naming
> > >> mistake as "safe mode" was. Developers will put their trust in it
> that it
> > >> is 100% guaranteed safe.
> > >
> > >
> > > FWIW, agreed, too. Trusted is vague and may imply some false sense of
> > > security. Literal is literally what it says on the tin.
> > >
> >
>
> Yasuo's contrived "faking it" eval example aside, I 100% agree with
> Derick, Benjamin, Levi,
>  and everyone saying that `is_trusted` is quite possibly the worst name
> that could be
> chosen for this. ((Okay, they didn't say that, but I am))
>
> Why not call it `is_safe` and really embrace the bad idea that was
> safe_mode properly.
> No. Nope. Hard no.
>
> > I can follow up properly tomorrow, but by popular request we do support
> > integers as well (could be seen as stretching the definition of
> “literal” a
> > bit).
> >
>
> Is a hard coded integer not a literal value?
> Nothing about the name 'literal' speaks to type, only to provenance.
>
> > Then someone said they wanted to check if an integer was a literal too -
> > but because of technical limitations, it now allows any integer,
> > regardless of where it came from, to be treated as a literal.
> >
>
> Oh, I missed this.  Yeah. No.
> If the provenance of an integer is unknown then it is neither literal nor
> trusted.
> It's a grenade and precisely the kind of thing that breaks any attempt at
> safety.
> Why can this not be tracked?  Is there no space in the zval to track
> IS_LITERAL
> as a flag rather than (and I'm making guesses about the current
> implementation here)
> storing it off the zend_string?  As a zval flag, it becomes theoretically
> applicable
> to any type (though for obvious reasons resources and objects have a hard
> time
> demonstrating literality).
>
> > That said, I’m really glad that the only issue we seem to have is the
> name.
> >
>
> As I've said off-list more than once, I'm not a fan of this feature in
> general as
> it provides a false sense of security, so no... the name is not the only
> issue.
> Using a loaded word like 'trusted' just makes the false-security
> implication worse.
>
> Take all that as you will because I'm probably still -1 on the feature
> overall
> regardless of the name, so my opinion on the name probably doesn't matter
> in that respect, but for me at least, it turns my probable -1 into a
> definite -1.
>
> -Sara
>

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Rowan Tommins]

On 22 June 2021 10:09:50 BST, Mike Schinkel  wrote:
>For my inspiration take a look at Trusted Types API in Javascript:
>
>https://developer.mozilla.org/en-US/docs/Web/API/Trusted_Types_API
>


There is an extremely important difference here: there is no single type in 
that system called "TrustedString", there are separate types for each context 
("injection sink"). The W3C article calls this out explicitly:

> Note: This allows the authors to specify the intention when creating a given 
> value, and the user agents to introduce checks based on the type of such 
> value to preserve the authors' intent. For example, if authors intend a value 
> to be used as an HTML snippet, an attempt to load a script from that value 
> would fail.

This is not just an implementation detail, it's an absolutely essential part of 
the concept.



So let me add my name to the chorus saying that is_trusted() is a bad name, and 
the added features that led to its selection make this feature worse not better.



Most of a "trusted types" implementation can be written in pure PHP, because 
all you need is an object with a private string property and some appropriate 
constructors. 

The one part you can't do is trust strings provided in source differently from 
strings provided by the user, and the original proposal provided a 
straightforward mechanism for that purpose.

There is no reason why is_literal itself needs to know about "trusted value 
objects", or have a long list of special cases to construct a string that is 
"not actually a literal but we can't think of any way to exploit it". That can 
all be handled by the userland code:

* Create a class called TrustedSql
* Accept a parameter of type string|TrustedSql. If the parameter is a string, 
reject it if is_literal returns false
* Or, if a pseudo-type is provided as well, just accept 
literal_string|TrustedSql
* If the user wants to provide dynamic SQL, they need to construct a TrustedSql 
object using whatever mechanism the library wants to provide. That can include 
an audited sprintf pattern, imploding arrays of integers, whatever turns out to 
be useful.


I think the engine should leave the complexities of defining "trusted" to APIs 
specialising in a particular "injection sink", and provide the low-level 
building block they need, which is the original simple is_literal function.


Regards,

-- 
Rowan Tommins
[IMSoP]

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Pierre Joye]

Hello Craig,

Very well written RFC, good job!

Others have said it already, but here are my thoughts. Many moons ago,
I was on this way as well and the filter extension came out. As it
fits for some projects, the actual gains were very far, to say the
least, from what I would have expected.

Since quite some time, and that thinking is intensified with the
support of native annotation, is that such a thing does not fit into
the core language(s). It will never be "trustable" 100%, which defeats
its main purpose or existance.

The large majority of apps or frameworks out there provide their own
interface to deal with external data input. Advanced ones like Symfony
can provide you an Entity where a parameter is the ID of that entity,
handling all safety checks. Others will provide a Request
implementation with getters as specific types, etc. This allows it to
be tightly linked to the actual usage or logic. It is impossible to
even agree on such an interface in the core.

As well intended as it looks, I think input data filtering is better
implemented in userland. And we may keep ourselves from reintroducing
trusted or safe mode, no matter where. I hope I don't sound too
negative, I am really convinced this is a bad idea and introducing it
again in 8.x will hunt the core for a decade to  come. :)

Best,

On Tue, Jun 22, 2021 at 3:25 AM Craig Francis  wrote:
>
> On Sat, 12 Jun 2021 at 18:00, Craig Francis 
> wrote:
>
> > I'd like to start the discussion on the is_literal() RFC:
> > https://wiki.php.net/rfc/is_literal
> >
>
>
> To recap,
>
> - We have chosen the name is_trusted(), based 18 votes for, vs 3 against.
>
> - Integers are now included, which will help adoption:
>
> https://wiki.php.net/rfc/is_literal
>
> (Joe’s currently updating the implementation to have the new name, but all
> the functionality is there).
>
> I’m glad this RFC has been well received; and thank you for all the
> feedback, I really think it‘s benefitting the implementation.
>
> Craig



-- 
Pierre

@pierrejoye | http://www.libgd.org

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Moritz Friedrich]

> Am 22.06.2021 um 20:38 schrieb Stephen Reay :
> 
> It took me about a minute to think of this:
> 
> "select * from customer_purchases where {$column} = :value”. 
> 
> The developer inadvertently passes the same “trusted value” in as the 
> `$column` substitute and the value parameter. It must be safe because we ran 
> it through `is_trusted`!
> 
> The query now executes as:
> 
> "select * from customer_purchases where 12345 = 12345”
> 
> 
> You cannot magically make all dynamically generated queries safe - they tried 
> that about a quarter of a century ago. Hint: it did not end well - and 
> explicitly allowing some user input is just mind boggling given the stated 
> goals.

There is a difference between “safe”, as in “safe to insert anywhere in a 
query”, and “trusted”, as in “trusted to not originate from user input”. 

Untrusted input is well understood and documented as a concept. PHP provides 
enough foot guns to choose from if you like taking function names too 
literally. 

This discussion focuses way too much on possible abuse by application 
developers, while it is actually a low-level feature for library authors which 
I trust very well to use it appropriately, and with opt-in configuration.

Regards
Moritz


-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Stephen Reay]

Sent from my iPhone

> On 23 Jun 2021, at 03:08, Lauri Kenttä  wrote:
> 
> On 2021-06-22 21:38, Stephen Reay wrote:
 On 22 Jun 2021, at 21:38, Craig Francis  wrote:
>>> If you can point me to an example where including integers in this has
>>> introduced a security vulnerability then please do, and I mean it, that’s
>>> what this process is for, I genuinely want people to come forward with them
>>> so we can refine this.
>> It took me about a minute to think of this:
>> "select * from customer_purchases where {$column} = :value”.
>> The developer inadvertently passes the same “trusted value” in as the
>> `$column` substitute and the value parameter. It must be safe because
>> we ran it through `is_trusted`!
>> The query now executes as:
>> "select * from customer_purchases where 12345 = 12345”
>> You cannot magically make all dynamically generated queries safe -
>> they tried that about a quarter of a century ago. Hint: it did not end
>> well - and explicitly allowing some user input is just mind boggling
>> given the stated goals.
> 
> 
> Your example is interesting and kind of valid. Looks like there is a bug in 
> the code: $column is sometimes an user-defined integer and sometimes a valid 
> literal column name, clearly this should not happen. (If it was supposed to 
> be integer, you would pass it as a parameter just like :value, right?)
> 
> Is this RFC about preventing bugs (accidentally used wrong variable) or 
> preventing bad code (user input intentional but used the wrong way)? I 
> thought it was more the about bad code. Bugs can live even in literal queries 
> as well as outside queries, where is_literal/is_trusted can't reach them.
> 
> Another line of thought: One possible approach would be to accept only 
> explicit integer casts (sprintf %d and %u, and a new function like 
> implode_ints() and/or strval_int()) and otherwise only accept strings. This 
> would avoid the accidental case where $x is supposed to be a trusted string 
> but is an untrusted integer instead, like the given example.
> 
> -- 
> Lauri Kenttä
> 

Yes it very deliberately shows a case where developer error can lead to 
incorrect behaviour, because of a bug. I don’t think it’s a stretch to say that 
not validating the dynamic elements used to generate a query would also be 
considered a bug caused by developer error.

Given that this is meant to be a security feature, an approach of “well this is 
probably fine” seems like a pretty cavalier approach to start from, especially 
given that there are existing solutions to prevent injections of integer values.

Rather than people keep repeating “integers aren’t injections”, perhaps someone 
can show just one example why you’d need to use a literal integer as an 
identifier in sql (ie a table name etc) and thus can’t use parameterisation.

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Lauri Kenttä]

On 2021-06-22 21:38, Stephen Reay wrote:
On 22 Jun 2021, at 21:38, Craig Francis  
wrote:


If you can point me to an example where including integers in this has
introduced a security vulnerability then please do, and I mean it, 
that’s
what this process is for, I genuinely want people to come forward with 
them

so we can refine this.



It took me about a minute to think of this:

 "select * from customer_purchases where {$column} = :value”.

The developer inadvertently passes the same “trusted value” in as the
`$column` substitute and the value parameter. It must be safe because
we ran it through `is_trusted`!

 The query now executes as:

 "select * from customer_purchases where 12345 = 12345”


You cannot magically make all dynamically generated queries safe -
they tried that about a quarter of a century ago. Hint: it did not end
well - and explicitly allowing some user input is just mind boggling
given the stated goals.



Your example is interesting and kind of valid. Looks like there is a bug 
in the code: $column is sometimes an user-defined integer and sometimes 
a valid literal column name, clearly this should not happen. (If it was 
supposed to be integer, you would pass it as a parameter just like 
:value, right?)


Is this RFC about preventing bugs (accidentally used wrong variable) or 
preventing bad code (user input intentional but used the wrong way)? I 
thought it was more the about bad code. Bugs can live even in literal 
queries as well as outside queries, where is_literal/is_trusted can't 
reach them.


Another line of thought: One possible approach would be to accept only 
explicit integer casts (sprintf %d and %u, and a new function like 
implode_ints() and/or strval_int()) and otherwise only accept strings. 
This would avoid the accidental case where $x is supposed to be a 
trusted string but is an untrusted integer instead, like the given 
example.


--
Lauri Kenttä

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Stephen Reay]

> On 22 Jun 2021, at 21:38, Craig Francis  wrote:
> 
> If you can point me to an example where including integers in this has
> introduced a security vulnerability then please do, and I mean it, that’s
> what this process is for, I genuinely want people to come forward with them
> so we can refine this.


It took me about a minute to think of this:

 "select * from customer_purchases where {$column} = :value”. 

The developer inadvertently passes the same “trusted value” in as the `$column` 
substitute and the value parameter. It must be safe because we ran it through 
`is_trusted`!

 The query now executes as:

 "select * from customer_purchases where 12345 = 12345”


You cannot magically make all dynamically generated queries safe - they tried 
that about a quarter of a century ago. Hint: it did not end well - and 
explicitly allowing some user input is just mind boggling given the stated 
goals.

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Craig Francis]

On Tue, 22 Jun 2021 at 3:05 pm, Stephen Reay 
wrote:

>
> On 22 Jun 2021, at 20:13, Craig Francis  wrote:
>
> On Tue, 22 Jun 2021 at 09:59, Stephen Reay 
> wrote:
>
>> So I just want to make sure I understand the progression on this so far.
>> It started out with people wanting a way to check that a string was a
>> literal string, in code somewhere, and does not come from user input. Ok
>> makes sense. The name makes sense too.
>>
>
>
>
> The primary reason was never just to define literal strings, the intention
> has always been to create a practical, implementable solution to address
> the issue of Injection Vulnerabilities (SQl/HTML/CLI/etc).
>
>
> Preventing injection vulnerabilities may be your *goal* but I’m talking
> about the *intended behaviour of this one function*.
>



I understand this is not the one true perfect solution, but that solution
does not exist without tanking performance and never being accepted solely
on that basis alone.

If you can point me to an example where including integers in this has
introduced a security vulnerability then please do, and I mean it, that’s
what this process is for, I genuinely want people to come forward with them
so we can refine this.

The only thing that you could suggest as an alternative is to not include
integers at all. That is not realistic for the reasons I’ve stated not just
here, but in replies to others, and the RFC itself. So if we can’t do that,
and we can’t mark developer-integers for reasons mentioned before, what
else is there? That we just don’t do it??

Torpedoing the idea because it is not 100% idealistically perfect, and
ignoring the fact that it, (as it stands right now with integers and all),
**Prevents Injection Vulnerabilities** - the most common security
vulnerability that dominates the top of the leaderboards again and again -
and does that without adding in any new security vulnerabilities, is just
nonsensical.

Craig

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Stephen Reay]

> On 22 Jun 2021, at 20:13, Craig Francis  wrote:
> 
> On Tue, 22 Jun 2021 at 09:59, Stephen Reay  > wrote:
> So I just want to make sure I understand the progression on this so far. It 
> started out with people wanting a way to check that a string was a literal 
> string, in code somewhere, and does not come from user input. Ok makes sense. 
> The name makes sense too.
> 
> 
> 
> The primary reason was never just to define literal strings, the intention 
> has always been to create a practical, implementable solution to address the 
> issue of Injection Vulnerabilities (SQl/HTML/CLI/etc).
> 

Preventing injection vulnerabilities may be your goal but I’m talking about the 
intended behaviour of this one function. Your original email says this:

>> Distinguishing strings from a trusted developer from strings that may be 
>> attacker controlled



If you feel that somehow doesn’t mean the same as "check that a string was a 
literal string, in code somewhere, and does not come from user input”, then we 
need to crack open a dictionary and work out which words one of us doesn’t know 
the meaning of.




> The name `is_literal()` has always just been a placeholder, it came up when I 
> first started looking at this problem because that was the most obvious thing 
> I knew we could anchor around. (Unfortunately I think it was easy to make 
> assumptions based solely on that name, rather than focussing on the issue it 
> is meant to address).
> 
> So, we cannot look for literals only - while it was part of the solution, it 
> was very much incomplete. Bearing in mind, there is considerable amount of 
> existing code and tutorials out there which include integers in their 
> SQL/HTML/CLI/etc, and they are perfectly safe in doing so. Making a solution 
> which does not support integers is not going to be adopted/used because the 
> task of rewriting and changing everything, for no benefit, will not be 
> considered by developers.
> 

There is a considerable amount of existing code that includes strings in SQL, 
HTML without danger too. Plenty of string values are fine, and plenty of 
integer values are fine. That doesn’t mean we should just blindly trust a value 
that came from the user, just because it’s a number.
The saying goes “never trust user input” not “never trust user input unless 
it’s a number”. 


> Likewise, a lot of code already builds SQL/HTML/CLI/etc strings via 
> concatenation and sprintf(), and forcing everyone to use a query builder is 
> likely to cause most people to not even consider using this.
> 

If they won’t adopt an existing solution to the problem why would they adopt 
this?
You’ve said very recently that this is not intended to be used directly by most 
developers, and instead used within libraries and frameworks. It seems a little 
weird to then make concessions that will defeat the stated goal, in the name of 
adoption. 


> It's all well thinking of one thing that might theoretically/idealistically 
> solve the issue, but it also needs to have a plan on how this will be 
> practically implemented and used by developers (which this has done).
>  


Having a plan for how to implement something doesn’t help much when the thing 
you’re implementing deliberately ignores a specific type of ‘untrusted’ input..

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Craig Francis]

On Tue, 22 Jun 2021 at 09:59, Stephen Reay  wrote:

> So I just want to make sure I understand the progression on this so far.
> It started out with people wanting a way to check that a string was a
> literal string, in code somewhere, and does not come from user input. Ok
> makes sense. The name makes sense too.
>



The primary reason was never just to define literal strings, the intention
has always been to create a practical, implementable solution to address
the issue of Injection Vulnerabilities (SQl/HTML/CLI/etc).

The name `is_literal()` has always just been a placeholder, it came up when
I first started looking at this problem because that was the most obvious
thing I knew we could anchor around. (Unfortunately I think it was easy to
make assumptions based solely on that name, rather than focussing on the
issue it is meant to address).

So, we cannot look for literals only - while it was part of the solution,
it was very much incomplete. Bearing in mind, there is considerable amount
of existing code and tutorials out there which include integers in their
SQL/HTML/CLI/etc, and they are perfectly safe in doing so. Making a
solution which does not support integers is not going to be adopted/used
because the task of rewriting and changing everything, for no benefit, will
not be considered by developers.

Likewise, a lot of code already builds SQL/HTML/CLI/etc strings via
concatenation and sprintf(), and forcing everyone to use a query builder is
likely to cause most people to not even consider using this.

It's all well thinking of one thing that might theoretically/idealistically
solve the issue, but it also needs to have a plan on how this will be
practically implemented and used by developers (which this has done).

Craig

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Craig Francis]

On Tue, 22 Jun 2021 at 11:31 am, Hans Henrik Bergan 
wrote:

> How is one supposed to use this? like
>
> if(!is_trusted($val)){
> $val = htmlentities($str, ENT_QUOTES | ENT_HTML401 | ENT_SUBSTITUTE |
> ENT_DISALLOWED, 'UTF-8', true);
> }
> echo "$val";




No, if anything that’s the opposite, and almost Taint Checking.

While this is covered in the RFC (https://wiki.php.net/rfc/is_literal) and
will be best read in context, in summary:

The developer does not use this function, instead you rely on libraries to
do that work for you. In this case you would use a HTML Templating Library
(which knows about all the complexities of HTML encoding), and you simply
provide the trusted string ‘?‘ and the values separately.

The Libraries will then use is_trusted(), with something like this:

https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification/example.php?ts=4

Craig

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Hans Henrik Bergan]

How is one supposed to use this? like

if(!is_trusted($val)){
$val = htmlentities($str, ENT_QUOTES | ENT_HTML401 | ENT_SUBSTITUTE |
ENT_DISALLOWED, 'UTF-8', true);
}
echo "$val";
(...)
if(!is_trusted($val)){
$val = $mysqli->real_escape_string($val);
}
$mysqli->query("INSERT INTO tbl VALUES('$val');");


like that? (my first impression is that this sounds stupid, but i haven't
given it enough thought to be sure)



On Tue, 22 Jun 2021 at 11:48, Lauri Kenttä  wrote:

> On 2021-06-21 23:25, Craig Francis wrote:
> >
> > - Integers are now included, which will help adoption:
> >
> > https://wiki.php.net/rfc/is_literal
> >
>
> Thanks for the great improvements!
>
> sprintf seems to have some issues, though.
>
> Currently you're checking the parameter types, not the formats.
> The parameter type matters only when coercing to a string (%s).
> Otherwise sprintf should consider the format, not the parameter.
>
> Example:
>  function test($s) { var_dump($s, is_trusted($s)); }
> setlocale(LC_ALL, "de_DE.UTF-8");
> test(sprintf("SET c = %c, f = %f, e = %e", 0x27, 1234, 1234));
> test(sprintf("SET d = %d, x = %x, b = %b", 1e2, 1e2, 1e2));
> test(sprintf("SET weird_d = %''*d", 4, 1));
> test(sprintf("SET s = '%s', int to string should be ok", 123));
> ?>
>
> Currently:
> string(43) "SET c = ', f = 1234,00, e = 1.234000e+3"
> bool(true)
> string(32) "SET d = 100, x = 64, b = 1100100"
> bool(false)
> string(18) "SET weird_d = '''1"
> bool(true)
> string(41) "SET s = '123', int to string should be ok"
> bool(true)
>
> Obviously the results with ints and floats should be the opposite.
>
> If you really want to allow %c, so be it, but I'd disallow it on the
> grounds that (1) it's probably not used in secure strings (usage data,
> anyone?), and thus (2) it could easily be a misspelled %d (for example,
> '%c' instead of '%d' could silently produce an empty result in a query),
> and (3) you're allowing a simple workaround with %s and chr() which
> makes the intent more obvious.
>
> In general, as this is supposed to be a security feature, allowing
> multiple ways for uninformed people to produce "trusted" but actually
> very unsafe values doesn't look like a good idea. I'm not sure if
> allowing trusted single characters to be created through chr or %c
> serves any useful purpose, but I can imagine people using either one
> without realizing that they can create any character, including \0 or '
> or " or non-UTF-8. Better to leave only chr(), one less thing to worry
> about.
>
> Custom padding is a weird edge case, maybe just disallow that too?
>
> As you said yourself, it's not easy to prove anything safe. ;)
>
> --
> Lauri Kenttä
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Lauri Kenttä]

On 2021-06-21 23:25, Craig Francis wrote:


- Integers are now included, which will help adoption:

https://wiki.php.net/rfc/is_literal



Thanks for the great improvements!

sprintf seems to have some issues, though.

Currently you're checking the parameter types, not the formats.
The parameter type matters only when coercing to a string (%s).
Otherwise sprintf should consider the format, not the parameter.

Example:


Currently:
string(43) "SET c = ', f = 1234,00, e = 1.234000e+3"
bool(true)
string(32) "SET d = 100, x = 64, b = 1100100"
bool(false)
string(18) "SET weird_d = '''1"
bool(true)
string(41) "SET s = '123', int to string should be ok"
bool(true)

Obviously the results with ints and floats should be the opposite.

If you really want to allow %c, so be it, but I'd disallow it on the 
grounds that (1) it's probably not used in secure strings (usage data, 
anyone?), and thus (2) it could easily be a misspelled %d (for example, 
'%c' instead of '%d' could silently produce an empty result in a query), 
and (3) you're allowing a simple workaround with %s and chr() which 
makes the intent more obvious.


In general, as this is supposed to be a security feature, allowing 
multiple ways for uninformed people to produce "trusted" but actually 
very unsafe values doesn't look like a good idea. I'm not sure if 
allowing trusted single characters to be created through chr or %c 
serves any useful purpose, but I can imagine people using either one 
without realizing that they can create any character, including \0 or ' 
or " or non-UTF-8. Better to leave only chr(), one less thing to worry 
about.


Custom padding is a weird edge case, maybe just disallow that too?

As you said yourself, it's not easy to prove anything safe. ;)

--
Lauri Kenttä

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Mike Schinkel]

> On Jun 21, 2021, at 7:18 PM, Benjamin Morel  wrote:
> 
> On Tue, 22 Jun 2021 at 01:06, Derick Rethans  wrote:
> 
>> On 21 June 2021 23:37:56 BST, Yasuo Ohgaki  wrote:
>>> 
>>> The name "is_trusted" is misleading.
>>> Literal is nothing but literal.
>> 
>> I agree with this. The name is_trusted is going to be the same naming
>> mistake as "safe mode" was. Developers will put their trust in it that it
>> is 100% guaranteed safe.
> 
> 
> FWIW, agreed, too. Trusted is vague and may imply some false sense of
> security. Literal is literally what it says on the tin.


I proposed the name change originally.

For my inspiration take a look at Trusted Types API in Javascript:

https://developer.mozilla.org/en-US/docs/Web/API/Trusted_Types_API 


And also Trusted Types API from the W3C

https://w3c.github.io/webappsec-trusted-types/dist/spec/ 


The reason is_trusted() would be better than is_literal() is so that the 
function could in the future handle other trusted types that are definitely not 
literal and allow most code that using is_trusted() to continue to work.  

So, for example, in the future we could add a new TrustedInterface and if an 
object implemented the trusted interface and a __ToString() method it could be 
considered trusted by code that uses the is_trusted() to guard against 
untrusted code.

Alternately if in the future PHP added a TrustedAttribute we could use on the 
__ToString() method to declare an object that could be used when a trusted 
string is required.

-

Conversely, I am *strongly* opposed to is_literal() as proposed.  The current 
RFC proposes half a solution without first envisioning what the other half of 
the solution will look like.

If an is_literal() RFC were passed I would envision the following things 
happening:

1. Bloggers would start blogging about how all PHP users should use 
is_literal() when accepting SQL and HTML code, but won't provide any of the 
nuances about special-cases because many won't understand them, they will just 
see a new PHP feature to hype.

2. PHP listing tools will jump on is_literal() and champion its use.  Just look 
at Psalm.

3. Coding standards teams in large organizations will see that the listing 
tools are flagging non-literals and wil start requiring all SQL and HTML etc. 
code to be literals. And since they already bar the use of eval() there will be 
no way to work around the requirement when you actually know what you are 
doing. 

And because they are in large organizations, they will be divorced from the 
developers in the trenches who 95% of the time will have no problem with the 
requirement, but 5% of the time will simply not be able to achieve their 
development needs. Unfortunately these trench developers will not have enough 
power in the organization to get the coding standards teams to even list to 
their issues, especially if they are an agency or contractor.

4. Library developers will similarly adopt is_literal() as a requirement to use 
their library.  While the most popular ones will likely be enlightened enough 
they will address edge cases. many in the long tail will not.  So lots of the 
long-tail PHP libraries out there — and especially the ones only needed by a 
small audience so there won't be alternatives — will not address exceptions. 
And for any of many reasons, many of those library developers will not be 
responsive to those who submit PRs to provide workarounds.

THIS is the future I do not want to see, and if is_trusted() is renamed back to 
is_literal() this is the future I predict. Maybe it won't happen, but I 
personally do not want to risk it.

The biggest problem with the RFC is it does not address how to bypass someone 
else's requirement for use of is_literal() when you *know* what you are doing 
and you absolutely need to.  Craig and I have had several long discussions 
about this, and he said he did not want to allow that because someone might 
misuse it (Craig, if I am paraphrasing incorrectly, please do correct me here.) 
 

And I completely understand Craig's misgivings, but then he would not address 
mine.  His only solution to get around the special-case needs that *will* exist 
is to use eval(). which is a lot like saying: "So you are in pain?  Why not try 
Meth?"  

Craig shared this talk from Google which inspired him:

https://www.youtube.com/watch?v=ccfEu-Jj0as 


I would highly recommend watching this before voting on Craig's RFC.

The speaker talks about how they used "CompileTimeConstants" to reduce security 
bugs, BUT:

1. They used a @CompileTimeConstant attribute and a static checker to guard 
against unsafe strings instead of a function baked into Java. So convention and 
process over language enforcement.

https://errorprone.info/bugpattern/CompileTimeConstant 

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Stephen Reay]

> On 22 Jun 2021, at 15:58, Stephen Reay  wrote:
> 
> 
> 
>> On 22 Jun 2021, at 06:28, Craig Francis > > wrote:
>> 
>> On Tue, 22 Jun 2021 at 12:18 am, Benjamin Morel >  > >>
>> wrote:
>> 
>>> On Tue, 22 Jun 2021 at 01:06, Derick Rethans  wrote:
>>> 
 On 21 June 2021 23:37:56 BST, Yasuo Ohgaki  wrote:
> 
> The name "is_trusted" is misleading.
> Literal is nothing but literal.
 
 I agree with this. The name is_trusted is going to be the same naming
 mistake as "safe mode" was. Developers will put their trust in it that it
 is 100% guaranteed safe.
>>> 
>>> 
>>> FWIW, agreed, too. Trusted is vague and may imply some false sense of
>>> security. Literal is literally what it says on the tin.
>>> 
>> 
>> 
>> I can follow up properly tomorrow, but by popular request we do support
>> integers as well (could be seen as stretching the definition of “literal” a
>> bit).
>> 
>> And we did ask for suggestions last week, which ended up with a vote (as I
>> couldn’t decide).
>> 
>> That said, I’m really glad that the only issue we seem to have is the name.
>> 
>> Craig
> 
> So I just want to make sure I understand the progression on this so far.
> 
> 
> It started out with people wanting a way to check that a string was a literal 
> string, in code somewhere, and does not come from user input. Ok makes sense. 
> The name makes sense too.
> 
> Then someone said they wanted to check if an integer was a literal too - but 
> because of technical limitations, it now allows any integer, regardless of 
> where it came from, to be treated as a literal.
> 
> Then because it’s not actually checking for literals, people thought the name 
> “trusted” made more sense?
> 
> 
> That nobody thinks “any user supplied integer must be surely safe” is kind of 
> hilarious, and sad at the same time.
> 
> Knowing that a string is literal would be very helpful. Knowing that the 
> string potentially still contains user input, in spite of the one thing it 
> claims to do, is not just unhelpful, it makes the entire thing useless.
> 
> 
> I can’t vote, but this whole thing would be a No from me unless it was the 
> original scope - a variable is a literal defined in code somewhere. If there 
> are technical limitations with some types, then leave them off the list of 
> what it will check.

s/nobody/anybody/

I blame a lack of caffeine.

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Stephen Reay]

> On 22 Jun 2021, at 06:28, Craig Francis  wrote:
> 
> On Tue, 22 Jun 2021 at 12:18 am, Benjamin Morel  >
> wrote:
> 
>> On Tue, 22 Jun 2021 at 01:06, Derick Rethans  wrote:
>> 
>>> On 21 June 2021 23:37:56 BST, Yasuo Ohgaki  wrote:
 
 The name "is_trusted" is misleading.
 Literal is nothing but literal.
>>> 
>>> I agree with this. The name is_trusted is going to be the same naming
>>> mistake as "safe mode" was. Developers will put their trust in it that it
>>> is 100% guaranteed safe.
>> 
>> 
>> FWIW, agreed, too. Trusted is vague and may imply some false sense of
>> security. Literal is literally what it says on the tin.
>> 
> 
> 
> I can follow up properly tomorrow, but by popular request we do support
> integers as well (could be seen as stretching the definition of “literal” a
> bit).
> 
> And we did ask for suggestions last week, which ended up with a vote (as I
> couldn’t decide).
> 
> That said, I’m really glad that the only issue we seem to have is the name.
> 
> Craig

So I just want to make sure I understand the progression on this so far.


It started out with people wanting a way to check that a string was a literal 
string, in code somewhere, and does not come from user input. Ok makes sense. 
The name makes sense too.

Then someone said they wanted to check if an integer was a literal too - but 
because of technical limitations, it now allows any integer, regardless of 
where it came from, to be treated as a literal.

Then because it’s not actually checking for literals, people thought the name 
“trusted” made more sense?


That nobody thinks “any user supplied integer must be surely safe” is kind of 
hilarious, and sad at the same time.

Knowing that a string is literal would be very helpful. Knowing that the string 
potentially still contains user input, in spite of the one thing it claims to 
do, is not just unhelpful, it makes the entire thing useless.


I can’t vote, but this whole thing would be a No from me unless it was the 
original scope - a variable is a literal defined in code somewhere. If there 
are technical limitations with some types, then leave them off the list of what 
it will check.

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Levi Morrison via internals]

On Mon, Jun 21, 2021 at 2:25 PM Craig Francis  wrote:
>
> On Sat, 12 Jun 2021 at 18:00, Craig Francis 
> wrote:
>
> > I'd like to start the discussion on the is_literal() RFC:
> > https://wiki.php.net/rfc/is_literal
> >
>
>
> To recap,
>
> - We have chosen the name is_trusted(), based 18 votes for, vs 3 against.

I guess I missed the vote on this one; I'm definitely against the name change.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Craig Francis]

On Tue, 22 Jun 2021 at 12:18 am, Benjamin Morel 
wrote:

> On Tue, 22 Jun 2021 at 01:06, Derick Rethans  wrote:
>
>> On 21 June 2021 23:37:56 BST, Yasuo Ohgaki  wrote:
>> >
>> >The name "is_trusted" is misleading.
>> >Literal is nothing but literal.
>>
>> I agree with this. The name is_trusted is going to be the same naming
>> mistake as "safe mode" was. Developers will put their trust in it that it
>> is 100% guaranteed safe.
>
>
> FWIW, agreed, too. Trusted is vague and may imply some false sense of
> security. Literal is literally what it says on the tin.
>


I can follow up properly tomorrow, but by popular request we do support
integers as well (could be seen as stretching the definition of “literal” a
bit).

And we did ask for suggestions last week, which ended up with a vote (as I
couldn’t decide).

That said, I’m really glad that the only issue we seem to have is the name.

Craig

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Benjamin Morel]

On Tue, 22 Jun 2021 at 01:06, Derick Rethans  wrote:

> On 21 June 2021 23:37:56 BST, Yasuo Ohgaki  wrote:
> >
> >The name "is_trusted" is misleading.
> >Literal is nothing but literal.
>
> I agree with this. The name is_trusted is going to be the same naming
> mistake as "safe mode" was. Developers will put their trust in it that it
> is 100% guaranteed safe.


FWIW, agreed, too. Trusted is vague and may imply some false sense of
security. Literal is literally what it says on the tin.

— Benjamin

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Derick Rethans]

On 21 June 2021 23:37:56 BST, Yasuo Ohgaki  wrote:
>
>The name "is_trusted" is misleading.
>Literal is nothing but literal.

I agree with this. The name is_trusted is going to be the same naming mistake 
as "safe mode" was. Developers will put their trust in it that it is 100% 
guaranteed safe. 

cheers, 
Derick 

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Craig Francis]

On Mon, 21 Jun 2021 at 11:38 pm, Yasuo Ohgaki  wrote:

> Hi,
>
> The name "is_trusted" is misleading.
> Literal is nothing but literal.
>
> 
>  eval('$var= '. $_GET['a'] );
>
> if (is_trusted($var)) echo $var;
> ?>
> 
>
> Literals cannot always be trusted.
>



That’s explained in the RFC, under “Limitations” and “Faking it”…

“That said, we do not pretend there aren't ways around this (e.g. using
var_export), but doing so is clearly the developer doing something wrong.
We want to provide safety rails, but there is nothing stopping the
developer from jumping over them if that's their choice.”

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Yasuo Ohgaki]

Hi,

The name "is_trusted" is misleading.
Literal is nothing but literal.





Literals cannot always be trusted.

--
Yasuo Ohgaki
yohg...@ohgaki.net


On Tue, Jun 22, 2021 at 5:25 AM Craig Francis 
wrote:

> On Sat, 12 Jun 2021 at 18:00, Craig Francis 
> wrote:
>
> > I'd like to start the discussion on the is_literal() RFC:
> > https://wiki.php.net/rfc/is_literal
> >
>
>
> To recap,
>
> - We have chosen the name is_trusted(), based 18 votes for, vs 3 against.
>
> - Integers are now included, which will help adoption:
>
> https://wiki.php.net/rfc/is_literal
>
> (Joe’s currently updating the implementation to have the new name, but all
> the functionality is there).
>
> I’m glad this RFC has been well received; and thank you for all the
> feedback, I really think it‘s benefitting the implementation.
>
> Craig
>

Re: [PHP-DEV] [RFC] is_trusted - was is_literal

[Craig Francis]

On Sat, 12 Jun 2021 at 18:00, Craig Francis 
wrote:

> I'd like to start the discussion on the is_literal() RFC:
> https://wiki.php.net/rfc/is_literal
>


To recap,

- We have chosen the name is_trusted(), based 18 votes for, vs 3 against.

- Integers are now included, which will help adoption:

https://wiki.php.net/rfc/is_literal

(Joe’s currently updating the implementation to have the new name, but all
the functionality is there).

I’m glad this RFC has been well received; and thank you for all the
feedback, I really think it‘s benefitting the implementation.

Craig