xp-framework/core

Possible information disclosure via Objects::stringOf

johannes85 opened this issue · 5 comments

johannes85 commented

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:

core/src/main/php/util/PropertyManager.class.php

Line 170 in ff3d062

Objects::stringOf(array_values($this->provider))

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 commented

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 commented

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 commented

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 commented

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.