RFC: Redacting parameters in back traces

[TimWolla]

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

---

Open Issues:

• Blocked by Fix memory leak of function attribute hash table #8070
• Memory Leak: RFC: Redacting parameters in back traces #7921 (comment)
• Prevent serialization or poison the object after unserialization? RFC: Redacting parameters in back traces #7921 (comment)
• Type of Exception thrown from SensitiveParameterValue::__unserialize(): https://externals.io/message/117022

[cmb69]

@TimWolla, RFC karma has been granted. Best of luck with the RFC! :)

[derickr]

It would be nice if this could be done through ArgInfo and the stubs at some point.

[TimWolla]

I agree, this would be nice. For now I've dropped adding the attribute to existing functions from this PR. This nicely separates the implementation of the behavior from adding the attribute to functions, making this easier to review.

[TysonAndre]

implementation seems correct at a glance, just minor nits

[TimWolla]

implementation seems correct at a glance, just minor nits

@TysonAndre Thank you for the review. I've implemented the suggested changes. Unfortunately the implementation is not yet correct and I could need some handholding here:

Apparently I'm mishandling the reference counting when storing the original value within SensitiveParameterValue. Specifically:

<?php

function foo(
	#[\SensitiveParameter]
	$bar = null
)
{
	throw new \Exception('error');
}

foo(...['bar' => 'bar']);

Will result in:

Fatal error: Uncaught Exception: error in php-src/test2.php:8
Stack trace:
#0 php-src/test2.php(11): foo(Object(SensitiveParameterValue))
#1 {main}
  thrown in php-src/test2.php on line 8
[Fri Feb 11 09:42:08 2022]  Script:  'php-src/test2.php'
php-src/Zend/zend_string.h(150) :  Freeing 0x00007fad2b258b80 (32 bytes), script=php-src/test2.php
=== Total 1 memory leaks detected ===

I believe this is about the call to Z_TRY_ADDREF_P. I don't know in which cases I'll need to increase the refcount myself and in which cases this will be done for me, as I've oriented myself at the original implementation. Is it safe when I just move this call into the else part of if (last_was_sensitive) {?

[TimWolla]

The RFC was accepted and I've just cleaned up this PR for review.

I've added a list of open issues / discussion points to the initial message of this PR:

• Fix memory leak of function attribute hash table #8070 should be reviewed and merged first.
• There's a memory leak where someone with better knowledge would need to advise me: RFC: Redacting parameters in back traces #7921 (comment)
• The type of exception thrown in SensitiveParameterValue::__unserialize() is up for discussion. See also: https://externals.io/message/117022

[TimWolla]

As all the open discussion points with regard to the implementation should be resolved now, I've squashed all my fixup commits to clean up the history. I've opted to keep the serialization commit separate, because that one is the change that differed from the original proposal.

The only open issue I'm aware of, is that #8070 is not yet reviewed/merged.

So, please take another look at the diff to make sure I didn't miss anything important!

[TimWolla]

Thanks to @bwoebi's assistance #8070 is resolved now and there are no known blockers.

As such this one is ready for review!

[TysonAndre]

lgtm, code seems correct but everything else using zend_object std is a struct with 2 or more properties.

Although it may make sense to leave as is and put the zval hidden; in the struct in a followup PR to hide it completely (and override get_gc, etc. like ext/spl/spl_fixedarray.c), so no need to change

[TysonAndre]

Nit: This works correctly, but doesn't need to be overridden with the current design and may confuse people about when to use XtOffsetOf to override the default .offset of 0.

It calls zend_object_std_dtor with the exact same address casted (offset=0), and zend_object_std_dtor is the default.

It'd be needed if you weren't using readonly properties but were putting things before std instead (e.g. ext/spl/spl_fixedarray.c)

[cmb69]

As of PHP 8.0.0, new code should better use offset of XtOffsetOf.

[TysonAndre]

#ifndef XtOffsetOf
# define XtOffsetOf(s_type, field) offsetof(s_type, field)
#endif

I assume you mean "use offsetof instead of XtOffsetOf". Good to know that it's unconditionally defined one way

[cmb69]

Yeah, right, "offsetof instead of XtOffsetOf".

[TimWolla]

Let me give the reasons behind why I did it, like I did it:

1. I've used a regular readonly property in the stub so that SensitiveParameterValue behaves like a user-land object as much as possible. It should protect against accidental exposure of the sensitive value, but if one explicitly uses Reflection to access the internal value (for whatever reason!) then it should work as if the object was implemented in userland (e.g. in a Polyfill).

As an example, this should be possible:

<?php

$foo = new SensitiveParameterValue('foo');

$r = new ReflectionClass($foo);
$p = $r->getProperty('value');

var_dump($p->getValue($foo));

2. Protecting against the (array) cast is only possible for native implementations and for that I need to override get_properties_for. While I don't need to store any additional internal values, I've added the attributes_sensitive_parameter_value struct with only the std member for consistency with other natively implemented classes, so that the SensitiveParameterValue class is no special case. Likewise I've calculated the offsets, even if I know they are always 0, so that adding additional struct members works properly without needing to adjust multiple locations.

If you think that I should simple use zend_object instead of attributes_sensitive_parameter_value then I can adjust the code, no problem. Just wanted to give my reasons.

Regarding offset vs XtOffsetOf: Do you have an example for that?

[TysonAndre]

If it's a deliberate design decision to expose properties with reflection, then that's fine, never mind.

If you think that I should simple use zend_object instead of attributes_sensitive_parameter_value then I can adjust the code, no problem. Just wanted to give my reasons.

I'd personally consider it worth it, there's other parts of the code using zend_object directly when there's no extra properties, the other examples you looked at probably had at least one other internal property. (e.g. __PHP_Incomplete_Class doesn't use std and has no hidden properties)

The code in php-src is used as an example by PECL authors, so the extra indirection may lead to copying an unnecessary abstraction for internal attributes on parameters to other places without understanding why

/* {{{ php_create_incomplete_class */
static zend_object *php_create_incomplete_object(zend_class_entry *class_type)
{
	zend_object *object;

	object = zend_objects_new( class_type);
	object->handlers = &php_incomplete_object_handlers;

	object_properties_init(object, class_type);

	return object;
}

PHPAPI void php_register_incomplete_class_handlers(void)
{
	memcpy(&php_incomplete_object_handlers, &std_object_handlers, sizeof(zend_object_handlers));
	php_incomplete_object_handlers.read_property = incomplete_class_get_property;
	php_incomplete_object_handlers.has_property = incomplete_class_has_property;
	php_incomplete_object_handlers.unset_property = incomplete_class_unset_property;
	php_incomplete_object_handlers.write_property = incomplete_class_write_property;
	php_incomplete_object_handlers.get_property_ptr_ptr = incomplete_class_get_property_ptr_ptr;
	php_incomplete_object_handlers.get_method = incomplete_class_get_method;

	php_ce_incomplete_class->create_object = php_create_incomplete_object;
}
/* }}} */

[TimWolla]

@TysonAndre Okay, if there's precedent then it makes sense to simplify this. I've made the change in 2fd9a60.

I've also added a test for reflection in 7e94af7.

If it's looking good to you I'll squash those commits into the initial commit.

[TimWolla]

I now went ahead and rebased it, as the commit links remain valid.

[kaznovac]

May I suggest putting this feature behind the ini configuration directive (defaulting to redact sensitive variables in production.ini, and show the wrapped sensitive values in development).

I find it helpful to see the config parameters (especially in docker/k8s when configuring via env/configs/secrets)

[TimWolla]

May I suggest putting this feature behind the ini configuration directive (defaulting to redact sensitive variables in production.ini, and show the wrapped sensitive values in development).

@kaznovac
A php.ini knob was not part of the RFC that was voted on and accepted: https://wiki.php.net/rfc/redact_parameters_in_back_traces#phpini_defaults. As php.ini knobs are an essential part of the default RFC template, adding them now would likely require a new RFC.

That said: As the RFC author I very intentionally did not propose any php.ini knobs for this, because PHP should not change its behavior based on some global configuration. To the best of my knowledge this opinion is shared by the majority of internals participants / active voters.

[bwoebi]

@TimWolla I think in this instance "PHP should not change its behavior based on some global configuration" is already sort defeated by the variety of error formatting options we already have. (which is a good thing. It's an error display configuration, which very much fits in INI. INIs which we do not want is anything which alters the data/control flow.)

Any ini should however only affect the step of translating backtrace to string and not affect debug_backtrace() output (i.e. only debug_print_backtrace(), Exception traces etc.).

[TimWolla]

@bwoebi Personally I disagree with an ini setting even if it's just for the default pretty printer for stack traces. I think that it makes it too easy to accidentally break the "safe by default" behavior of the RFC as proposed.

---

I also must admit that at this point it's hard to me to differentiate between actual review comments (“change requests”) and comments that just want to share some opinion on some matter (where I don't need to or don't should act on, unless agreed on with a separate vote).

Can we get this PR with the behavior as-agreed-on in the RFC (+ the serialization erratum) across the finish line first? Having an INI setting can then be discussed in a new RFC that builds upon this one.

[iluuu1994]

Merged as 9085197

[Hanmac]

I got a Question why SensitiveValue is only ever used on Parameters and not for example Properties?

like if i got the most generic User Object

class User
{
  protected $password;
}

and another function that takes a user object:

public function doSomethingWithUser(User $user)
{
    $this->doSomethingGeneric($user);
}

// this one can't make $object as Sensitive because it might not know that it can be?
public function doSomethingGeneric(object $object)
{
   // there might be Exception
}

and when some Exception happen inside this other function, the user password could be exposed in the Backtrace.
One way would be to mark every function that takes a User object as Sensitive, But that might not know it if that function normally takes a more generic object.

Wouldn't it be better in such case to mark the Property for password as Sensitive so it isn't shown in the Backtrace while the User Object is?