Everything

php

Merging fuzzing SAPI into core

php.internals

Stas Malyshev

7 years ago

Hi!

As you probably know, we've been running PHP fuzzing under Google's
OSS-Fuzz[1] project for a while now (and found and fixed some bugs due
to it).

This has been enabled by the PHP fuzzing API SAPI[2] which currently
lives in a separate repository. Since the setup is working pretty well
for a while now, I would like to propose to merge it into core
repository as a core SAPI, and make Travis CI setup build it as part of
the CI tests.

This would ensure the fuzzing scripts are not broken by core changes
(happened several times recently) and would provide wider exposure to
the fuzzing setup we have, hopefully prompting extension authors and
other contributors to add more fuzzing modules to it, thus enhancing PHP
security and reliability.

Are there any objections or suggestions about this? Do we need an RFC
for it? Note that this is only for master branch (only master is being
fuzzed now), though it would not be hard to port to other branches if
there's interest, the fuzzer should work on pretty much any recent
branch with small code changes.

[1] https://github.com/google/oss-fuzz/
[2] https://github.com/php/php-fuzzing-sapi

--
Stas Malyshev
smalyshev@gmail.com

Nikita Popov

7 years ago

On Mon, Jul 29, 2019 at 2:48 AM Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> Hi!
>
> As you probably know, we've been running PHP fuzzing under Google's
> OSS-Fuzz[1] project for a while now (and found and fixed some bugs due
> to it).
>
> This has been enabled by the PHP fuzzing API SAPI[2] which currently
> lives in a separate repository. Since the setup is working pretty well
> for a while now, I would like to propose to merge it into core
> repository as a core SAPI, and make Travis CI setup build it as part of
> the CI tests.
>
> This would ensure the fuzzing scripts are not broken by core changes
> (happened several times recently) and would provide wider exposure to
> the fuzzing setup we have, hopefully prompting extension authors and
> other contributors to add more fuzzing modules to it, thus enhancing PHP
> security and reliability.
>
> Are there any objections or suggestions about this? Do we need an RFC
> for it? Note that this is only for master branch (only master is being
> fuzzed now), though it would not be hard to port to other branches if
> there's interest, the fuzzer should work on pretty much any recent
> branch with small code changes.
>
> [1] https://github.com/google/oss-fuzz/
> [2] https://github.com/php/php-fuzzing-sapi

Sounds good to me. Feel free to submit a PR for review.

Nikita

Bishop Bettini

7 years ago

On Sun, Jul 28, 2019 at 8:48 PM Stanislav Malyshev <smalyshev@gmail.com>
wrote:

>
> As you probably know, we've been running PHP fuzzing under Google's
> OSS-Fuzz[1] project for a while now (and found and fixed some bugs due
> to it).
>
> This has been enabled by the PHP fuzzing API SAPI[2] which currently
> lives in a separate repository. Since the setup is working pretty well
> for a while now, I would like to propose to merge it into core
> repository as a core SAPI, and make Travis CI setup build it as part of
> the CI tests.
>
> This would ensure the fuzzing scripts are not broken by core changes
> (happened several times recently) and would provide wider exposure to
> the fuzzing setup we have, hopefully prompting extension authors and
> other contributors to add more fuzzing modules to it, thus enhancing PHP
> security and reliability.
>
> Are there any objections or suggestions about this? Do we need an RFC
> for it? Note that this is only for master branch (only master is being
> fuzzed now), though it would not be hard to port to other branches if
> there's interest, the fuzzer should work on pretty much any recent
> branch with small code changes.
>

I see benefit both ways. On the one hand, having the fuzzer and engine code
together removes the effort of integrating the two, which (as you mention)
makes it easier to extend and invoke. This is good for core and extension
maintainers. On the other, I've found it refreshing working in a slender
repo that doesn't have all the history and process rules. This is good for
external (non-core and non-extension) collaborators, particularly allowing
write access to those who wouldn't want or need write access to engine code.

I lean toward the "keep it separate" option, since the process barriers are
much smaller. Couldn't we update Travis to clone the repo, run the
build/configure steps, then invoke the fuzzer as part of the make test
phase? Or, how about the other way: setup Travis on the fuzzing repo to
clone php/php-src:master, integrate it, and run?

Either way, I don't think an RFC is applicable. To me, the fuzzer is a
dev-dependency (in composer parlance) that doesn't surface to users.

Stas Malyshev

7 years ago

Hi!

> This is good for external (non-core and non-extension) collaborators,

particularly allowing write access to those who wouldn't want or need
write access to engine code.

Yes, in theory. In practice I don't see too many of those flocking to
it, unfortunately. As for process, I think we have enough people
(starting with myself) that would gladly guide through any pull req to
this particular part if asked, and I don't see anybody ever needing to
go through RFCs etc. to contribute there. So the process is probably
"make the pull, ping the list" :)

> I lean toward the "keep it separate" option, since the process barriers
> are much smaller. Couldn't we update Travis to clone the repo, run the
> build/configure steps, then invoke the fuzzer as part of the make test
> phase? Or, how about the other way: setup Travis on the fuzzing repo to
> clone php/php-src:master, integrate it, and run?

That's an interesting idea. I still like the integrated repo better,
since it's easier for the person making changes to ensure everything
builds properly instead of being alerted by Travis and go back digging
for a third-party repo they may not even be aware of. But this could be
an option too. And btw that reminds me that if it'd be integrated, we
also need Travis build to support it. Separate thanks for this reminder!

--
Stas Malyshev
smalyshev@gmail.com

Johannes Schlueter

7 years ago

On Tue, 2019-07-30 at 13:28 -0400, Bishop Bettini wrote:

> On the other, I've found it refreshing working in a
> slender repo that doesn't have all the history and process rules.
> This is good for external (non-core and non-extension) collaborators,
> particularly allowing write access to those who wouldn't want or need
> write access to engine code.

We could grant access to the respective SAPI directory only. Also this
is non-production code so even if merged rules can be relaxed. Only
rule we have no real flexibility over is the merge requirement. (not
observing that would cause trouble for others)

I am also not sure having this independent makes it simpler for
outsiders to contribute. You maybe ease the submission process, but
setting it up becomes harder as you need to clone to repos, put it in
the right place, make sure you are in compatible branches (on next PHP
7 like API change this might become relevant, maybe even sooner as it
goes into different extension's APIs where we observe BC a bit less)

I think merging it into PHP makes the most sense, also for signaling to
the outside that we care about security by having fuzzing routines as
core part of the thing.

johannes

Bishop Bettini

7 years ago

On Tue, Jul 30, 2019 at 6:15 PM Johannes Schlüter <johannes@schlueters.de>
wrote:

> On Tue, 2019-07-30 at 13:28 -0400, Bishop Bettini wrote:
> > On the other, I've found it refreshing working in a
> > slender repo that doesn't have all the history and process rules.
> > This is good for external (non-core and non-extension) collaborators,
> > particularly allowing write access to those who wouldn't want or need
> > write access to engine code.
>
>
> I am also not sure having this independent makes it simpler for
> outsiders to contribute. You maybe ease the submission process, but
> setting it up becomes harder as you need to clone to repos, put it in
> the right place, make sure you are in compatible branches (on next PHP
> 7 like API change this might become relevant, maybe even sooner as it
> goes into different extension's APIs where we observe BC a bit less)
>
> I think merging it into PHP makes the most sense, also for signaling to
> the outside that we care about security by having fuzzing routines as
> core part of the thing.
>

Fair points, Johannes and Stas. I've no objection to moving the SAPI into
php/php-src.