RFC [Discussion]: Redacting parameters in back traces

Dan Ackroyd

4 years ago

Hi Tim,

On Mon, 10 Jan 2022 at 14:05, Tim Düsterhus, WoltLab GmbH
<duesterhus@woltlab.com> wrote:

>
> this is a follow-up for my "Pre-RFC" email from last Friday, January, 7th.
>
> https://wiki.php.net/rfc/redact_parameters_in_back_traces
>

How do other languages handle this problem? Or how do they avoid it in
the first place?

From the RFC:

> Specifically the back trace collection should be updated to use an object of class
> \SensitiveParameter as the value for all parameters that are marked with the
> \SensitiveParameter attribute.

To me....these words are not clear. Does the following sentence say
the same thing?

"When the backtrace is generated, any parameter that has a
'SensitiveParameter' attribute will not have it's value stored in the
backtrace, but instead will be replaced with an SensitiveParameter
object.

If so, the RFC could be updated to be clearer.....if not, then the RFC
should be updated to be clearer.

Also, having parameters replaced with another type doesn't seem
obviously correct. There should probably be some words justifying why
that is the correct thing to do, rather than just replacing any values
with "****REDACTED***" or other simple behaviour.

> On shared web hosting, the customer might not be able to configure it.

My personal opinion is that shared web hosting shouldn't be a thing
that exists in 2022. And definitely shouldn't be used for anything
where secrets need to be maintained. Yeah shared hosts might have a DB
they can connect to, but those credentials should only be usuable from
the shared host to the DB.

cheers
Dan
Ack

Ryan Jentzsch

4 years ago

Answering the question: How do other languages handle this problem? Or how
do they avoid it in
the first place?

Python basically doesn't handle the problem at all and offers this advice: Be
sure to delete all debugging related code before code delivery!

See section [9.2.1 production code cannot contain any debug entry points]

https://www.fatalerrors.org/a/python-general-programming-specification-09-security-programming-specification.html

On Mon, Jan 10, 2022 at 10:01 AM Dan Ackroyd <Danack@basereality.com> wrote:

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Dan

On 1/10/22 6:01 PM, Dan Ackroyd wrote:

> How do other languages handle this problem? Or how do they avoid it in
> the first place?

Ryan already replied here, but I've also researched this:

- Java is unable to provide parameters in stack traces.
- In C you generally have a core dump which contains everything.
Parameters might or might not be available depending on optimization level.
- In Python parameters are not provided by default, but it appears to be
available via
https://docs.python.org/3/library/inspect.html#inspect.getargvalues.
- In JavaScript the '.stack' property is non-standard and the behavior
depends on the engine. Generally no parameters are provided.
- In Ruby parameters are not provided.

So basically all the other languages I researched do not provide
arguments within back traces. I might have missed something, as I am not
experienced in all of them.

>  From the RFC:
>> Specifically the back trace collection should be updated to use an object of class
>> \SensitiveParameter as the value for all parameters that are marked with the
>> \SensitiveParameter attribute.
>
> To me....these words are not clear. Does the following sentence say
> the same thing?
>
> "When the backtrace is generated, any parameter that has a
> 'SensitiveParameter' attribute will not have it's value stored in the
> backtrace, but instead will be replaced with an SensitiveParameter
> object.
>
> If so, the RFC could be updated to be clearer.....if not, then the RFC
> should be updated to be clearer.

Yes, your phrasing is correct. It's much better than mine, so I've taken it.

> Also, having parameters replaced with another type doesn't seem
> obviously correct. There should probably be some words justifying why
> that is the correct thing to do, rather than just replacing any values
> with "****REDACTED***" or other simple behaviour.

I updated the RFC with a new "Choice of replacement value" section:

https://wiki.php.net/rfc/redact_parameters_in_back_traces#choice_of_replacement_value

>> On shared web hosting, the customer might not be able to configure it.
>
> My personal opinion is that shared web hosting shouldn't be a thing
> that exists in 2022. And definitely shouldn't be used for anything
> where secrets need to be maintained. Yeah shared hosts might have a DB
> they can connect to, but those credentials should only be usuable from
> the shared host to the DB.

While I agree with regard to shared web hosting generally, I'd rather
see a customer using a shared web hosting than attempting to self-host
and running outdated vulnerable PHP versions.

Also while PDO is the prominent example at the beginning of the RFC,
other functions are also affected. As an example 'ldap_bind()' might
include a user's password when the directory is unavailable, possibly
logging the user's password within Sentry or whatever error collector is
in use.

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Dan Ackroyd

4 years ago

On Tue, 11 Jan 2022 at 09:11, Tim Düsterhus, WoltLab GmbH
<duesterhus@woltlab.com> wrote:

>
> So basically all the other languages I researched do not provide
> arguments within back traces.

Uh, that kind of suggests that providing arguments at all is a
mistake, and that removing could be the way to go. I mean other than
everyone complaining about the BC break.

An awkward question; why is this hard-coding of the behaviour
dependent on a new attribute the correct thing to do?

PHP allows people to set_error_handler() to process errors how they
like. Conceiveably, allowing users to set a custom function to redact
data from stack traces would allow users to inspect and redact the
parameters however they like. Why is adding a special attribute that
is recognised by the PHP engine itself the right thing to do?

To be clear, I can think of at least two reasons. But also, I think
every attribute that is proposed to be added to PHP core, needs to
have the reasons why it's the right thing to do listed. If nothing
else, it will help to reject attributes in the future if they don't
have the same strong justifications.

cheers
Dan
Ack

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Dan

On 2/7/22 17:36, Dan Ackroyd wrote:

>> So basically all the other languages I researched do not provide
>> arguments within back traces.
>
> Uh, that kind of suggests that providing arguments at all is a
> mistake, and that removing could be the way to go. I mean other than
> everyone complaining about the BC break.

Personally I wouldn't complain about the BC break per se, but about that
making debugging harder :-)

> An awkward question; why is this hard-coding of the behaviour
> dependent on a new attribute the correct thing to do?

Reading through the RFC I concede that this is not (yet) explicitly
spelled out. However the following sentence appears within the
"Proposal" section:

> To reliably apply this protection for all types of back traces and all types of exception and error handlers, the redaction should happen when collecting the parameter values during back trace generation.

"reliably apply [...] for all [...] error handlers".

I've explained that in more detail in the podcast with Derick
(https://phpinternals.news/97).

e.g. at the end of 1:05:

> But in any case, this exception handler will miss sensitive information because it needs to basically guess what parameters are sensitive values and which don't.

-

> PHP allows people to set_error_handler() to process errors how they
> like. Conceiveably, allowing users to set a custom function to redact
> data from stack traces would allow users to inspect and redact the
> parameters however they like. Why is adding a special attribute that
> is recognised by the PHP engine itself the right thing to do?

As explained above: Without having a standardized solution, redacting
those arguments results in guesswork for the exception handler. A
userland solution (e.g. standardized in PHP FIG) does not cut it,
because then native functions will not be protected (e.g. ldap_bind).

I have made the following change to the RFC to hopefully make this
clearer within the introduction:

https://wiki.php.net/rfc/redact_parameters_in_back_traces?do=diff&rev2%5B0%5D=1643972253&rev2%5B1%5D=1644252789&difftype=sidebyside

Please let me know if this is sufficiently clear, or if you'd like to
see this expanded further.

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Alexandru Pătrănescu

4 years ago

On Mon, Jan 10, 2022 at 4:05 PM Tim Düsterhus, WoltLab GmbH <
duesterhus@woltlab.com> wrote:

> Hi Internals!
>
> this is a follow-up for my "Pre-RFC" email from last Friday, January, 7th.
>
> Christoph Becker granted me RFC editing permissions and I've now written
> up our proposal as a proper RFC:
>
> https://wiki.php.net/rfc/redact_parameters_in_back_traces

Hey Tim,

As the trace in the exception is in the same format as the one generated
by debug_backtrace(),
do you intend to have the changes affecting debug_backtrace()
and debug_print_backtrace()?

Also, globally configuring generated exception traces to not include
parameters would solve the issue?
I mean, similarly with how DEBUG_BACKTRACE_IGNORE_ARGS works?

Regards,
Alex

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Alex

On 1/11/22 4:10 AM, Alexandru Pătrănescu wrote:

> As the trace in the exception is in the same format as the one generated
> by debug_backtrace(),
> do you intend to have the changes affecting debug_backtrace()
> and debug_print_backtrace()?

My proof of concept patch adjusts the internal
'debug_backtrace_get_args' function which the function that collects the
function parameters whenever a back trace is generated (Exceptions,
debug_backtrace, debug_print_backtrace).

I've added a new example showing debug_backtrace to the RFC.

> Also, globally configuring generated exception traces to not include
> parameters would solve the issue?
> I mean, similarly with how DEBUG_BACKTRACE_IGNORE_ARGS works?

This is already configurable with 'zend.exception_ignore_args'. There's
a section in the RFC why we believe this to not be a solution:

https://wiki.php.net/rfc/redact_parameters_in_back_traces#why_existing_features_are_insufficient

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Pierre Joye

4 years ago

Good morning Tim,

On Mon, Jan 10, 2022 at 9:06 PM Tim Düsterhus, WoltLab GmbH
<duesterhus@woltlab.com> wrote:

I am not sure it makes sense to make the code so verbose to prevent
users from showing sensitive data as it never stops (next
print_r/var_dump and userland version of them?).

Also sensitive data goes way beyond arguments, GDPR brings a lot of
issues here too. Userland packages like monolog provide filters or
custom output, I think that is where it should be handled.

As a side note, the RFC mentions that zend.exception_ignore_args may
not be configurable on some shared hosters, it is INI_ALL, so even in
the code could change it, any time, back and forth:

<?php
function foo($a) {
var_dump($a);
throw new Exception('Thrown');
}
foo('adas');
ini_set('zend.exception_ignore_args', true);
foo('adas');

$ php t.php

string(4) "adas"
PHP Fatal error:  Uncaught Exception: Thrown in /home/pierre/t.php:4
Stack trace:
#0 /home/pierre/t.php(6): foo()
#1 {main}
thrown in /home/pierre/t.php on line 4

best,

--
Pierre

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

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Pierre

On 1/11/22 4:48 AM, Pierre Joye wrote:

> Also sensitive data goes way beyond arguments, GDPR brings a lot of
> issues here too. Userland packages like monolog provide filters or
> custom output, I think that is where it should be handled.

I believe that the author of a function is in the best position to
decide whether a specific argument generally holds sensitive data or
not. This avoids every exception handler / logger / … having to check
what function parameters hold sensitive data and scrubbing them,
possibly missing some.

Of course these exception handlers / loggers will still need to take
care of any other data they are getting from the request context. But in
that case the affected values (e.g. the user object) often need to be
explicitly passed into the handler, because they are application specific.

> As a side note, the RFC mentions that zend.exception_ignore_args may
> not be configurable on some shared hosters, it is INI_ALL, so even in
> the code could change it, any time, back and forth:

I've seen all kinds of broken configurations / broken builds at shared
web hosting over time, where things that generally work, do not for some
reason.

But good point indeed, I've removed that list item and only left the
other one.

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Pierre Joye

4 years ago

Hi Tim,

On Tue, Jan 11, 2022 at 4:40 PM Tim Düsterhus, WoltLab GmbH
<duesterhus@woltlab.com> wrote:

>
> Hi Pierre
>
> On 1/11/22 4:48 AM, Pierre Joye wrote:
> > Also sensitive data goes way beyond arguments, GDPR brings a lot of
> > issues here too. Userland packages like monolog provide filters or
> > custom output, I think that is where it should be handled.
>
> I believe that the author of a function is in the best position to
> decide whether a specific argument generally holds sensitive data or
> not. This avoids every exception handler / logger / … having to check
> what function parameters hold sensitive data and scrubbing them,
> possibly missing some.

You are fully correct here.

My thoughts differ as I think there are better tools to handle this
than PHP itself, as well as all other cases where sensitive data may
be exposed (a lot more than backtrace, which is handled already using
the ini setting).
Things like query errors, etc. There is no way we can handle them all
and this is really the developer responsibility to handle this data in
a safe manner. We do our best at the engine level for critical data
from an engine point of view. Application level critical data are the
developers responsibility (config, code, etc.).

Best,

--
Pierre

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

Michael Morris

4 years ago

On Mon, Jan 10, 2022 at 8:05 AM Tim Düsterhus, WoltLab GmbH <
duesterhus@woltlab.com> wrote:

> Hi Internals!
>
> this is a follow-up for my "Pre-RFC" email from last Friday, January, 7th.
>
> Christoph Becker granted me RFC editing permissions and I've now written
> up our proposal as a proper RFC:
>
> https://wiki.php.net/rfc/redact_parameters_in_back_traces
>
> I recommend also taking a look at my previous email:
>
> https://externals.io/message/116847
>
> It contains some additional context that did not really fit within the
> language of a "neutral" RFC that will remain as the permanent record.
>
> - As indicated within the RFC and my previous email we still need a more
> experienced developer for the final implementation, as I have next to no
> experience with PHP's implementation.
>
> Specifically adding this attribute to existing functions is not clear to
> me. It is probably required to update the stub parser/generator to add
> support for attributes? If someone creates an example implementation for
> one function, I'll likely be able to apply this to other functions myself.
> - The RFC Impact to Opcache is not clear to me. I don't believe there is
> any, but I am not sure. So if someone knows, I'm happy to update that
> section.
>
>
>

If someone can inject a debug_backtrace into your code and get it executed
you have bigger problems than a parameter being exposed. And if you
configure your prod servers to be all chatty Kathy to the world on error,
you need to learn how to do better. A change to the language is not in
order here.

Jordan LeDoux

4 years ago

On Mon, Jan 10, 2022 at 9:37 PM Michael Morris <tendoaki@gmail.com> wrote:

>
> If someone can inject a debug_backtrace into your code and get it executed
> you have bigger problems than a parameter being exposed. And if you
> configure your prod servers to be all chatty Kathy to the world on error,
> you need to learn how to do better. A change to the language is not in
> order here.
>

These things can also be logged as well. This isn't a security concern only
in the sense of the backtrace being displayed on a webpage output or
something. There are legal requirements in many jurisdictions about how
data can be retained and where. It is entirely possible that something
could be accidentally logged that would inadvertently violate a local
regulation for handling of customer data.

Jordan

Lynn

4 years ago

On Mon, Jan 10, 2022 at 3:05 PM Tim Düsterhus, WoltLab GmbH <
duesterhus@woltlab.com> wrote:

> Hi Internals!
>
> this is a follow-up for my "Pre-RFC" email from last Friday, January, 7th.
>
> Christoph Becker granted me RFC editing permissions and I've now written
> up our proposal as a proper RFC:
>
> https://wiki.php.net/rfc/redact_parameters_in_back_traces
>
> I recommend also taking a look at my previous email:
>
> https://externals.io/message/116847

Heya, thanks for this RFC!

The product I work on provides integration with dozens, if not hundreds of
remote services. This can be anything from mailboxes, (S)FTP(S), HTTP, and
we have a lot of databases for our tenants. Being the legacy code it is,
it's a never ending battle to not expose critical information to the end
user. Even though I've been working here for several years now, I still
keep finding these things occasionally. As we also use external logging
tools (such as the ELK stack), we want as little critical information sent
anywhere and this RFC seems to really help reduce the leaking of this
information here. In the ideal scenario the connection information minus
the password is logged explicitly, so that I have the information available
whenever one of the many tenants systems failed to connect to one of the
many dynamically configured APIs, but doing this retroactively through
millions of lines of code that's over 10 years old is a lot of work.

One possible addition; would it be possible to analyze the masked values
and mask any 100% matches elsewhere?

https://3v4l.org/G0RaQ
```
function one(string $param) {
throw new Exception($param);
}

function two(#[SensitiveParameter] string $param) {
one($param);
}

two('the secret');

Fatal error: Uncaught Exception: the secret in /in/G0RaQ:4
Stack trace:
#0 /in/G0RaQ(8): one('the secret')
#1 /in/G0RaQ(11): two('the secret')
#2 {main}
thrown in /in/G0RaQ on line 4
```

This would help in the scenario where the function `one` comes from an
external library, but it also means that I don't have to go through every
single layer and add the attributes. In the above example I want any
mention of `the secret` to be hidden. Let's say the sensitive data is a
password, then I don't want that password shown at all in the stacktrace.
The original RFC is already very valuable to me without this, but would
still expose a lot of sensitive data I fear.

The result I want from this:
```
Fatal error: Uncaught Exception: Object(SensitiveParameter) in /in/G0RaQ:4
Stack trace:
#0 /in/G0RaQ(8): one(Object(SensitiveParameter))
#1 /in/G0RaQ(11): two(Object(SensitiveParameter))
#2 {main}
thrown in /in/G0RaQ on line 4
```

This would also cover cases where for some reason the sensitive data is
added to the exception. Yes, this big facepalm is something I encounter
sadly too often in legacy code.

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Lynn

On 1/11/22 11:23 AM, Lynn wrote:

> One possible addition; would it be possible to analyze the masked values
> and mask any 100% matches elsewhere?

No, this is not in scope for this RFC, as it would require accurate
tracking of variable contents across reassignments and possibly function
calls.

My understanding is that this basically would require support for
tainted variables, i.e. this very old RFC: https://wiki.php.net/rfc/taint

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Lynn

4 years ago

On Wed, Jan 12, 2022 at 9:17 AM Tim Düsterhus, WoltLab GmbH <
duesterhus@woltlab.com> wrote:

> Hi Lynn
>
> On 1/11/22 11:23 AM, Lynn wrote:
> > One possible addition; would it be possible to analyze the masked values
> > and mask any 100% matches elsewhere?
>
> No, this is not in scope for this RFC, as it would require accurate
> tracking of variable contents across reassignments and possibly function
> calls.
>
> My understanding is that this basically would require support for
> tainted variables, i.e. this very old RFC: https://wiki.php.net/rfc/taint

I was thinking more of a "keep track of the values replaced, and in the
end purge all those values from the end-result" kinda thing.

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Lynn

On 1/12/22 9:30 AM, Lynn wrote:

>   I was thinking more of a "keep track of the values replaced, and in the
> end purge all those values from the end-result" kinda thing.
>

Thank you for the clarification. This still is not in scope, because I
believe that to be harmful, as the parameter redaction will be
completely unpredictable.

Consider a sensitive parameter that is of type '?string', i.e. nullable.
Now with your proposal, whenever 'null' is passed to this parameter, all
'null's within the stack trace would be hidden, even if they are
completely unrelated.

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Lynn

4 years ago

On Thu, Jan 13, 2022 at 10:04 AM Tim Düsterhus, WoltLab GmbH <
duesterhus@woltlab.com> wrote:

> Hi Lynn
>
> On 1/12/22 9:30 AM, Lynn wrote:
> >   I was thinking more of a "keep track of the values replaced, and in the
> > end purge all those values from the end-result" kinda thing.
> >
>
> Thank you for the clarification. This still is not in scope, because I
> believe that to be harmful, as the parameter redaction will be
> completely unpredictable.
>
> Consider a sensitive parameter that is of type '?string', i.e. nullable.
> Now with your proposal, whenever 'null' is passed to this parameter, all
> 'null's within the stack trace would be hidden, even if they are
> completely unrelated.
>
>

Yeah I'm with you 100%, this has more edge cases than I originally thought
of. The RFC as it is already improves a lot for me so I'll be glad to see
it regardless!

Benjamin Eberlei

4 years ago

Hi Tim,

On Mon, Jan 10, 2022 at 3:06 PM Tim Düsterhus, WoltLab GmbH <
duesterhus@woltlab.com> wrote:

> Hi Internals!
>
> this is a follow-up for my "Pre-RFC" email from last Friday, January, 7th.
>
> Christoph Becker granted me RFC editing permissions and I've now written
> up our proposal as a proper RFC:
>
> https://wiki.php.net/rfc/redact_parameters_in_back_traces

This is a very good addition in my opinion. And as one of the attributes
RFC authors I am thrilled about their use here ;-)

I believe it wouldn't hurt the RFC to add more words around the fact that
stacktraces are often sent to third party services (Exception Tracking
software) and as such a redaction of the parameters would be powerful for
additional redaction of credit cards, email addresses and other personal
data. The example with PDO::__construct is an obvious choice to redact
passwords, but application level data is a second source of input that is
critical to redact.

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Benjamin

On 1/15/22 7:07 PM, Benjamin Eberlei wrote:

> I believe it wouldn't hurt the RFC to add more words around the fact that
> stacktraces are often sent to third party services (Exception Tracking
> software) and as such a redaction of the parameters would be powerful for
> additional redaction of credit cards, email addresses and other personal
> data. The example with PDO::__construct is an obvious choice to redact
> passwords, but application level data is a second source of input that is
> critical to redact.
>

Thank you for the feedback. I've expanded (and hopefully clarified) the
"Introduction" section in version 1.2:

https://wiki.php.net/rfc/redact_parameters_in_back_traces?rev=1642064843&do=diff

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Internals!

On 1/10/22 15:05, Tim Düsterhus, WoltLab GmbH wrote:

> https://wiki.php.net/rfc/redact_parameters_in_back_traces

At the end of last week I've updated the RFC a little based on the
questions Derick Rethan asked me for episode #97 of PHP Internals News
podcast:

https://phpinternals.news/97

> - As indicated within the RFC and my previous email we still need a more
> experienced developer for the final implementation, as I have next to no
> experience with PHP's implementation.
>
> Specifically adding this attribute to existing functions is not clear to
> me. It is probably required to update the stub parser/generator to add
> support for attributes? If someone creates an example implementation for
> one function, I'll likely be able to apply this to other functions myself.

I also managed to figure this out. The proof of concept implementation at

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

now adds the \SensitiveParameter attribute to PDO::__construct()'s
$password parameter and to password_hash()'s $password parameter.

> - The RFC Impact to Opcache is not clear to me. I don't believe there is
> any, but I am not sure. So if someone knows, I'm happy to update that
> section.

For this I got a confirmation in private that Opcache will not be affected.

-

I believe I've answered all open questions and I also managed to resolve
the open issues I listed in my initial email.

As more than two weeks have passed since I started this discussion
thread and as the discussion appears to have died down by now I'd like
to start the vote on this RFC.

I plan to open voting on Wednesday, February, 2nd. Voting will run 2
weeks, 2/3 majority with the concept being voted on as explained in the
"Proposed Voting Choice" section:
https://wiki.php.net/rfc/redact_parameters_in_back_traces#proposed_voting_choices

This being my first RFC, please let me know if I missed something with
regard to the procedure!

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Alexandru Pătrănescu

4 years ago

On Mon, Jan 31, 2022 at 11:55 AM Tim Düsterhus, WoltLab GmbH <
duesterhus@woltlab.com> wrote:

> Hi Internals!
>
> On 1/10/22 15:05, Tim Düsterhus, WoltLab GmbH wrote:
> > https://wiki.php.net/rfc/redact_parameters_in_back_traces
> At the end of last week I've updated the RFC a little based on the
> questions Derick Rethan asked me for episode #97 of PHP Internals News
> podcast:
>
> https://phpinternals.news/97
>
> https://github.com/php/php-src/pull/7921
>
> now adds the \SensitiveParameter attribute to PDO::__construct()'s
> $password parameter and to password_hash()'s $password parameter.
>
>
> I believe I've answered all open questions and I also managed to resolve
> the open issues I listed in my initial email.
>
>

Hey Tim,

I think storing the original value within the replacement value should be
considered and voted in this RFC as well, even if implemented in a separate
PR.
I did write some code where I process programmatically the backtraces and
while I might not have used it with sensitive parameters, it would be good
to have the code generic, if this passes.

I'm guessing that mostly means accepting the value as a constructor
parameter exposing a getValue() method
And, of course, making sure var_dump/print_r/string-casting does not print
it. I mean, it looks like the implementation is doable.

Thinking about this will bring a small issue into plain sight, the
attribute is the same class as the replacing placeholder,
\SensitiveParameter.
I believe they should be separate classes, \SensitiveParameter marked as an
Attribute that can be applied to parameters and something like
\SensitiveParameterValue that replaces the original value  in stack traces.

Regards,
Alex

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Alex

On 2/1/22 07:38, Alexandru Pătrănescu wrote:

> I think storing the original value within the replacement value should be
> considered and voted in this RFC as well, even if implemented in a separate
> PR.
> I did write some code where I process programmatically the backtraces and
> while I might not have used it with sensitive parameters, it would be good
> to have the code generic, if this passes.

That's fair. I guess you are thinking of including this in the primary
vote, instead of a secondary vote, right? It doesn't make sense to leave
this out if you already have a use case that would break otherwise.

> I'm guessing that mostly means accepting the value as a constructor
> parameter exposing a getValue() method
> And, of course, making sure var_dump/print_r/string-casting does not print
> it. I mean, it looks like the implementation is doable.

I believe the following (userland) implementation should do the right thing:

final class SensitiveParameterValue
{
public function __construct(private readonly mixed $value) {}

public function getValue(): mixed { return $value; }

public function __debugInfo(): array { return []; }

public function __serialize(): array { return []; }
}

It allows you to explicitly retrieve the original value, but makes it
hard to accidentally expose it, by hiding it from 'var_dump()' and
'serialize()'.

> Thinking about this will bring a small issue into plain sight, the
> attribute is the same class as the replacing placeholder,
> \SensitiveParameter.
> I believe they should be separate classes, \SensitiveParameter marked as an
> Attribute that can be applied to parameters and something like
> \SensitiveParameterValue that replaces the original value  in stack traces.

You are right. If we also want to store the original value, we should
use a separate class. In any case re-using the attribute class will
limit future extensions.

I've already adjusted the RFC (and the PoC implementation) to update the
replacement value to SensitiveParameterValue:
https://wiki.php.net/rfc/redact_parameters_in_back_traces. Regarding
storing the original value I'll wait for your reply.

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Alex

On 2/1/22 07:38, Alexandru Pătrănescu wrote:

> I think storing the original value within the replacement value should be
> considered and voted in this RFC as well, even if implemented in a separate
> PR.
> I did write some code where I process programmatically the backtraces and
> while I might not have used it with sensitive parameters, it would be good
> to have the code generic, if this passes.
>
> I'm guessing that mostly means accepting the value as a constructor
> parameter exposing a getValue() method
> And, of course, making sure var_dump/print_r/string-casting does not print
> it. I mean, it looks like the implementation is doable.

I've now proceeded with this:

https://wiki.php.net/rfc/redact_parameters_in_back_traces?do=diff&rev2%5B0%5D=1643710897&rev2%5B1%5D=1643972253&difftype=sidebyside

I've also updated the example implementation:

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

Please take a look, looking forward to your feedback!

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Internals!

On 1/31/22 10:54, Tim Düsterhus, WoltLab GmbH wrote:

> I plan to open voting on Wednesday, February, 2nd. Voting will run 2
> weeks, 2/3 majority with the concept being voted on as explained in the
> "Proposed Voting Choice" section:
> https://wiki.php.net/rfc/redact_parameters_in_back_traces#proposed_voting_choices
>

This did not happen because of Alex' remark that storing the original
value should be part of this vote.

I've made the necessary changes and did not receive any further remarks,
thus:

I plan to open voting on Wednesday, February, 9th. Voting will run 2 weeks.


Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P