Re: [PHP-DEV] SensitiveParameterValue serialization behavior

[kontakt]

> Am 26.02.2022 um 12:49 schrieb Dan Ackroyd :
> 
> On Thu, 24 Feb 2022 at 14:11, Tim Düsterhus, WoltLab GmbH
>  wrote:
>> 
>> I see two possible options to remediate this issue:
>> 
>> ---
>> 
>> 1. Disallow both serialization and unserialization.
>> 
>> This will make the serialization issue very obvious, but will require
>> adjustments to exception handlers that serialize the stack traces.
> 
> That sounds the best option. Yes, people who serialize/unserialize
> stack traces will need to take into account the new behaviour, but
> they will need to do that anyway, and it's better for something to
> fail early, when the data is serialized rather than it happen later
> when someone goes to use that data. Programs continuing when they have
> already failed is Bad™.
> 
>> Allow unserialization, but poison the unserialized object and
>> disallow calling ->getValue() on it.
> 
> That sounds like something to be instantly added to the list of PHP Sadness.
> 
>> I theoretically could add an additional public function isPoisoned(): bool 
>> as well.
> 
> To me, that sounds like a workaround needed by a bad design aka a hack.
> 
> Guilliam Xavier wrote:
>> to avoid potentially breaking existing code.
> 
> Technically, all existing code will continue to run, as no-one is
> currently using SensitiveParameter and so that code will continue to
> run. When people start using that feature, then yes they will need to
> make sure that any stack serializer is aware of the new feature. But
> that doesn't sound like a huge problem to me.

That is not technically true from what I understand the SensitiveParameter will 
be added to core functions in a second step, so this will make them appear in 
existing code immediately.

It would also require some iteration across the whole stack trace and all its 
parameters to sanitize. Code that would always look the same and have no extra 
purpose.

However i cant seem to wrap my head around serialising stack traces with 
parameters. Does someone have an open source example of this kind of code?

Still I am for the second option, because SensitiveParameter without a value is 
still enough information in an unserialized state. 
> 
> In general, I think we should only add surprising and awkward apis
> when there is a really strong reason for doing so, not because there
> might be a problem. If it's left as unserializable for now, people
> would have the opportunity for saying why it needs to be relaxed
> later, aka "no is temporary, yes is forever" for features.
> 
> cheers
> Dan
> Ack
> 
> -- 
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
> 

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

Re: [PHP-DEV] Allowing NULL for some internal functions

[Craig Francis]

On Mon, 28 Feb 2022 at 22:11, Christian Schneider 
wrote:

> Am 28.02.2022 um 22:05 schrieb Christoph M. Becker :
> > The BC break doesn't appear to be that serious after all.
>
> I'm not sure I get your point here: If you provide a user-land
> implementation of the previous behavior under a different name then the BC
> break cannot be serious?
>
> - Chris
>


Yeah, I'm not sure what the point is either... sorry, I thought the
suggestion was a joke, and replied off-list as such, but creating
replacement userland functions for all of these functions, and to update
all code to use those functions... maybe I'm missing something.

And because I'm back to tired pissed-off sarcasm masking depression (why
bother spending days on a solution that works for everyone when we can make
a hostile/elitist environment/language)... maybe we could simply suggest
that everyone affected by this should use strval() for everything?
Admittedly the following only does 1 parameter, with a single variable, but
at least this is one way to dismiss/ignore the problem:

sed -i -E
's/(htmlspecialchars)\((\$[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*)\)/\1(strval(\2))/i'
index.php

As to voting at:

https://quiz.craigfrancis.co.uk/

So far it's 10 votes to continue supporting null coercing for those not
using `strict_types=1` (as PHP always has always done with internal
functions, and non-strict comparison operators)... 10 votes to ignore the
problem and make a hard backwards compatibility break for PHP 9.0... and 4
votes to update some functions to explicitly allow null (going to round
two, you get 1 more vote each, and 2 which drop out because they either
didn't provide a second choice, or selected "Don't Mind")... also, while I
have counted them, 7 people didn't enter a name, or entered "N/A"... and
obviously this vote only helps me setup the RFC, it will be the RFC that
will be the official record of who voted for what.

Craig

Re: [PHP-DEV] Allowing NULL for some internal functions

[Craig Francis]

On Mon, 28 Feb 2022 at 17:35, Guilliam Xavier 
wrote:

> Call me devil's advocate, but is it too late to discuss revisiting past
> decisions and consider changing direction towards 1 for userland functions
>


Hi Guilliam,

tbh, for those who use `strict_types=1` nothing changes, so we can ignore
them... for everyone else, nothing would break if user functions did start
coercing NULL (if anything it just makes PHP more consistent)... whereas
the current direction would add a Type Error exception for developers who
aren't familiar with them, and those developers will just get annoyed at
having to convert those NULL values manually (see the complaints from
developers upgrading to PHP 8.1).

Craig

Re: [PHP-DEV] Allowing NULL for some internal functions

[Christian Schneider]

Am 28.02.2022 um 22:05 schrieb Christoph M. Becker :
> On 28.02.2022 at 21:51, Craig Francis wrote:
> 
>> And after all of this, no-one has come up with a way to find or address
>> this problem, e.g.
>> 
>> >  $nullable = ($_GET['a'] ?? NULL);
>>  echo htmlentities($nullable);
>> ?>
> 
>  function my_htmlentities(string|null $value) {
>return htmlentities($value ?? "");
> }
> 
> $nullable = ($_GET['a'] ?? NULL);
> echo my_htmlentities($nullable);
> ?>
> 
> The BC break doesn't appear to be that serious after all.

I'm not sure I get your point here: If you provide a user-land implementation 
of the previous behavior under a different name then the BC break cannot be 
serious?

- Chris

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

Re: [PHP-DEV] Allowing NULL for some internal functions

[Christoph M. Becker]

On 28.02.2022 at 21:51, Craig Francis wrote:

> And after all of this, no-one has come up with a way to find or address
> this problem, e.g.
>
>$nullable = ($_GET['a'] ?? NULL);
>   echo htmlentities($nullable);
> ?>



The BC break doesn't appear to be that serious after all.

--
Christoph M. Becker

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

Re: [PHP-DEV] Allowing NULL for some internal functions

[Craig Francis]

On Mon, 28 Feb 2022 at 17:42, Larry Garfield  wrote:

> Bringing internal functions into line with user-space was the correct
> move.  There may be internals functions that make sense to be nullable on
> their own right, on a case by case basis.  We can evaluate that case by
> case.
>


Thanks Larry,

I agree about bringing them into line, but I think our understanding of
NULL may be different... these are PHP scripts which have used NULL as a
distinct value for, well, forever? and many developers expect it to be
coerced like the other values.

And while some developers use strict_types=1 (like myself) because we like
the type to match up without values being coerced (I should be the one that
manually chooses to convert)... I don't think I should force that strict
coding style onto everyone, because there is nothing technically wrong with
passing NULL into functions like urlencode(), it just implies, iff you use
strict_types=1, that something may have gone wrong earlier.

As an aside, I often note that many developers who talk about how strict
their code is... obv not you, but many still use 'unsafe-inline' JavaScript
on their websites (mixing content), and don't use Trusted Types to disable
unsafe JS APIs, which I consider a much bigger security concern :-)

And after all of this, no-one has come up with a way to find or address
this problem, e.g.



./vendor/bin/psalm --init ./public/ 4
./vendor/bin/psalm
  No errors found!

./vendor/bin/phpstan analyse -l 9 ./public/
  [OK] No errors

./vendor/bin/phpcs -p public/ --standard=PHPCompatibility
  . 1 / 1 (100%)

./vendor/bin/rector process ./public/
  [OK] Rector is done!

Craig

Re: [PHP-DEV] Allowing NULL for some internal functions

[Craig Francis]

On Mon, 28 Feb 2022 at 16:41, Dik Takken  wrote:

> In my view, consistency between internal and userland functions brings a
> lot of value, and not only for the language itself.



Thanks Dik,

I agree that consistency is very important, and I do not want to stop
that... I just recognise that many scripts use NULL coercion a lot, as they
do with other variable types (when not using `strict_types=1`), and that's
being broken just for NULL (which creates a form of inconsistency vs
int/float/string/bool types).

I started with preferring 2, but I think 1 is the best route to keep the
spirit of the original RFC, while not breaking backwards compatibility.

Craig

Re: [PHP-DEV] Allowing NULL for some internal functions

[Larry Garfield]

On Mon, Feb 28, 2022, at 11:35 AM, Guilliam Xavier wrote:
> On Mon, Feb 28, 2022 at 5:41 PM Dik Takken  wrote:
>
>>
>> In my view, consistency between internal and userland functions brings a
>> lot of value, and not only for the language itself. As soon as internal
>> and userland become fully consistent it will become a lot easier to
>> write "internal" functions in PHP rather than C. Not only will that make
>> developing the standard library easier, it may also make the optimizer
>> and JIT compiler more effective. The more consistency the better.
>>
>
> Yes, and we see two possible ways to make them consistent w.r.t. handling
> of null argument passed into scalar parameter:
>
> 1. implicit coercion by default, error with strict_types=1
> 2. error (independent of strict_types)
>
> AFAIK, internal functions have been doing 1 since like forever, but PHP 7.0
> chose 2 for userland functions when introducing scalar parameter type
> declarations (see my previous message for history) and PHP 8.1 continued in
> that direction by deprecating 1 for internal functions (and planning to
> change them to 2 in PHP 9.0).
> Call me devil's advocate, but is it too late to discuss revisiting past
> decisions and consider changing direction towards 1 for userland functions
> (esp. in implications of BC impact)?

Under absolutely no circumstances should parameters become implicitly nullable. 
 That way lies madness.  And yes, coercing null to a magic zero-value is a form 
of implicit nullability.  Absolutely not, under any circumstances.

Bringing internal functions into line with user-space was the correct move.  
There may be internals functions that make sense to be nullable on their own 
right, on a case by case basis.  We can evaluate that case by case.  

--Larry Garfield

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

Re: [PHP-DEV] Allowing NULL for some internal functions

[Larry Garfield]

On Mon, Feb 28, 2022, at 5:28 AM, Mark Randall wrote:
> On 28/02/2022 01:46, Craig Francis wrote:
>> Personally I think `strict_types=1` is fine for my code, but I would never
>> want to force that style on everyone, because doing so would be fairly
>> hostile for a language that's popular and well known for being easy to
>> use/learn.
>
> Magically coercing things and hiding things under the hood is useful for 
> the first 10 minutes of learning.
>
> After that it just becomes a hindrance difficult because you have to 
> know all of the secret rules about type juggling.

This needs to be called out more.  "Easy to learn" doesn't come from being 
forgiving of sloppiness.  It comes from making the non-sloppy approach 
straightforward, obvious, and if someone does do something sloppy the error 
message(s) point people toward doing it the right way.

Forgiving of sloppiness is how you gt security errors, not ease of use.

--Larry Garfield

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

Re: [PHP-DEV] Allowing NULL for some internal functions

[Guilliam Xavier]

On Mon, Feb 28, 2022 at 5:41 PM Dik Takken  wrote:

>
> In my view, consistency between internal and userland functions brings a
> lot of value, and not only for the language itself. As soon as internal
> and userland become fully consistent it will become a lot easier to
> write "internal" functions in PHP rather than C. Not only will that make
> developing the standard library easier, it may also make the optimizer
> and JIT compiler more effective. The more consistency the better.
>

Yes, and we see two possible ways to make them consistent w.r.t. handling
of null argument passed into scalar parameter:

1. implicit coercion by default, error with strict_types=1
2. error (independent of strict_types)

AFAIK, internal functions have been doing 1 since like forever, but PHP 7.0
chose 2 for userland functions when introducing scalar parameter type
declarations (see my previous message for history) and PHP 8.1 continued in
that direction by deprecating 1 for internal functions (and planning to
change them to 2 in PHP 9.0).
Call me devil's advocate, but is it too late to discuss revisiting past
decisions and consider changing direction towards 1 for userland functions
(esp. in implications of BC impact)?

Regards,

-- 
Guilliam Xavier

Re: [PHP-DEV] Allowing NULL for some internal functions

[Dik Takken]

On 21-02-2022 10:04, Christoph M. Becker wrote:

That "inconsistency" had been introduced with PHP 7.0.0, i.e. right when
scalar type declarations have been introduced.  Passing a null to a
non-nullable parameter of a *userland* function throws a TypeError:
.  As of PHP 8.1.0, internal functions behave
the same as userland function in this regard.



In my view, consistency between internal and userland functions brings a 
lot of value, and not only for the language itself. As soon as internal 
and userland become fully consistent it will become a lot easier to 
write "internal" functions in PHP rather than C. Not only will that make 
developing the standard library easier, it may also make the optimizer 
and JIT compiler more effective. The more consistency the better.


Approaches 2 and 3 from the RFC are a step in the right direction.

Regards,
Dik Takken

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

Re: [PHP-DEV] SensitiveParameterValue serialization behavior

[Tim Düsterhus , WoltLab GmbH]

Hi Internals!

On 2/24/22 15:11, Tim Düsterhus, WoltLab GmbH wrote:

Please find the thread in the GitHub PR at:

https://github.com/php/php-src/pull/7921#discussion_r813743903

[…]

1. Disallow both serialization and unserialization.

This will make the serialization issue very obvious, but will require
adjustments to exception handlers that serialize the stack traces.


Thank you for voicing your opinion.

I've adjusted the implementation to "Disallow both serialization and 
unserialization":


https://github.com/php/php-src/pull/7921#discussion_r815976815

As per Dan's response disallowing serialization is the "safe" choice 
with regard to future changes and it neatly avoids the bikeshedding with 
regard to the exception class to use [1].


I'll make sure to update the RFC with an errata section later.

[1] https://externals.io/message/117022#117120

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duester...@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

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

Re: [PHP-DEV] SensitiveParameterValue serialization behavior

[Guilliam Xavier]

Hi again,

FWIW, Dan's and Claude's explanations (thanks!) and arguments made me
change my preference to option 1 (i.e. make SensitiveParameterValue not
serializable, period).

Best regards,

-- 
Guilliam Xavier

Re: [PHP-DEV] Allowing NULL for some internal functions

[Mark Randall]

On 28/02/2022 01:46, Craig Francis wrote:

Personally I think `strict_types=1` is fine for my code, but I would never
want to force that style on everyone, because doing so would be fairly
hostile for a language that's popular and well known for being easy to
use/learn.


Magically coercing things and hiding things under the hood is useful for 
the first 10 minutes of learning.


After that it just becomes a hindrance difficult because you have to 
know all of the secret rules about type juggling.



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