Possible information disclosure via Objects::stringOf

[johannes85]

While migrating some old xp6 code to xp9 (I know it's old but the latest version supporting scriptlets) I noticed a possible security flaw with Objects::stringOf.

In this case it's used by the PropertyManager (xp9) to generate a exception message by dumping its providers. see: https://github.com/xp-framework/core/blob/ff3d06223e16a8b72fe1c0523092775b43ebf18d/src/main/php/util/PropertyManager.class.php#L170 In xp6 the error message is fine, in xp9+ it contains all properties with their (secret) values. I think that this can be an issue in other places, too. Especially when migrating old code.

This is caused by the removal of the \lang\Object base object which inherited \lang\Generic. The old \xp::stringOf method used the toString method if called with an object inheriting \lang\Generic. The new one in Objects::stringOf serializes the full object.

I think it's a good idea to ether call the toString method by convention if its exists (even when there is no interface containing it) or introduce another way to ensure no secret data will be dumped.

[thekid]

I disagree that there is an information disclosure problem in util.Objects. Secret values that should not appear in stacktraces or anywhere else should use the util.Secret class introduced in 6.8.0 almost 7 years ago (see #108).

However, having toString() called even if objects do not implement lang.Value would simplify quite some code where hashCode() and compareTo are implemented in a way that always yield distinct objects, simply because the interface requires their implementation. I've come up with PR #313 but will have to give this some more thought.

[thekid]

The PropertyManager class was deprecated in 9.8.0 (October 2018) and subsequently removed in XP 11, which is the currently supported release series, see #290. You should really think about moving away from using this API!

[johannes85]

Yes, we are thinking about moving away from this old stuff, but you know... priorities. PropertyManager + Properties isn't using util.Secret when reading the secrets from an .ini file. Yes, I know PropertyManager is deprecated but in this case we have to migrate some old code using Scriptlets to a newer PHP version with minimum efford.

[thekid]

So, using an educated guess after looking at the code for PropertyManager and friends, your problem is a ResourcePropertySource where instead of just root, the cache is also dumped. The cache contains util.Properties instances, whose toString() method then also dumps any secrets in the property file. It all boils down to this:

$ cat test.ini
[global]
db.pass=secret!

$ xp -w '$p= new \util\Properties("test.ini"); $p->reset(); return $p->toString()'
util.Properties(test.ini)@{[global => [db.pass => "secret!"]]}

Also happens if we use the {$secret.xyz} notation, all of these expansions are performed while loading the file.

👉 This would mean the root cause is the Properties class. I'll come up with a PR for this.

[thekid]

Fix released in https://github.com/xp-framework/core/releases/tag/v11.4.0