protected properties could be public, simplifying encoding

[mcdruid]

See #26

In some cases, the way that PHP serializes protected properties means phpggc's output contains NULL bytes which are easy to lose if the payload is transmitted / stored as plain text without encoding.

It looks like it should be possible to change some protected properties to public, thus avoiding the NULL bytes in the payload (added bonus: payloads are marginally smaller).

For example, Drupal7/RCE1 seems to work fine with all of the properties converted to public.

I suppose in some cases the specific target may require that properties remain protected, but is there any reason not to try converting them to public, and suggesting that as a default?

Happy to submit a PR, but wanted to sanity check the idea first.

[cfreal]

Hello @mcdruid,

The reason we don't do this is that it does not work with older PHP versions.

See this example code:

<?php

class SomeClass {
    protected $a;

    function getA() {
        return $this->a;
    }
}

$z = unserialize('O:9:"SomeClass":1:{s:1:"a";s:5:"hello";}');
var_dump($z);
var_dump($z->getA());

a is protected, but through the unserialize call we set it as if it was a public property, as you proposed. Now, see the difference:

$ php8.1 demo.php
object(SomeClass)#1 (1) {
  ["a":protected]=>
  string(5) "hello"
}
string(5) "hello"
$ php5.6 demo.php
object(SomeClass)#1 (2) {
  ["a":protected]=>
  NULL
  ["a"]=>
  string(5) "hello"
}
NULL

Now, we could add this behaviour as optional (with something like a --public-attributes flag). If you want to PR this, I'll happily add it.

Best,
Charles

[mcdruid]

Cool, thanks for the explanation.

I'll have a look at how we'd add an option for public attributes and submit a PR if I can come up with a half-decent approach.

[mcdruid]

Submitted a PR with a first pass at adding an --public-attributes option.

One thing though.. shouldn't it be --public-properties?

[swapgs]

+1 in favor of --public-properties to stick with the "official" PHP terminology (attributes, properties).

[swapgs]

@mcdruid Looking back at the (new) potential complexity of the PR and the fact that it won't work with all PHP versions, I wonder if this is really worth it. If this is only about storing the payloads in a "binary safe" format, there are already a few supported encoders. Or do you have another goal in mind?

[cfreal]

I came to the same conclusions as you regarding the feasibility of other solutions. We could do something cleanish where we parse the ast of gadgets.php, but what about PHP-defined classes?

I was going to comment that there is also --ascii-strings, that uses the S representation of strings to get rid of binary characters, but it got me wondering how I implemented it. Turns out it bears the same limitations as the code you have PR'ed, @mcdruid.

See https://github.com/ambionics/phpggc/blob/master/lib/PHPGGC/Enhancement/ASCIIStrings.php

Therefore... Well, with a proper warning, your code should be included.

[mcdruid]

@swapgs the use case I had for this was specifically where the payload could not be encoded at all; it would be stored in a database like reflected stored XSS, and passed to unserialize verbatim from a SQL query.

The only way I could get the Drupal7/RCE1 payload to work in that situation was to eliminate the null bytes.

I suppose one alternative to making the solution complex is to keep the implementation simple but acknowledge that it's a sort of "experimental" feature and may not work with all payloads against all targets.

I think I could improve the existing PR without allowing too much scope-creep if we accept that.

[mcdruid]

@cfreal :) I cross posted with you, but we seem to be in general agreement!

I'd be happy to take at least one more pass at the PR before your final review though if you don't mind.

Apart from anything else I'd like to look at supporting private properties as well as protected.

[mcdruid]

I think the PR is ready for review now; it should remove prefixes for private properties as well as protected.

There are warnings in the help text and comments about the circumstances in which the enhancement might not work.