pestphp / pest

Pest is an elegant PHP testing Framework with a focus on simplicity, meticulously designed to bring back the joy of testing in PHP.
https://pestphp.com
MIT License
9.06k stars 313 forks source link

feature(presets): Add security preset #1174

Closed ClaraLeigh closed 3 weeks ago

ClaraLeigh commented 3 weeks ago

Adds an additional preset for functions often seen as insecure.

I have a couple more laravel specific ones but I'll include those in another PR.

ClaraLeigh commented 3 weeks ago

Also wondering if it should be renamed but building out a security arch test further seems like it would be a neat idea

nunomaduro commented 3 weeks ago

@valorin any concrete ideas here?

ClaraLeigh commented 3 weeks ago

If y'all are curious, this was sparked by a pentest I'm doing on a laravel site. As I was working on it, I started building out a very crude Security pest test, with laravel in mind.

Here it is: https://gist.github.com/ClaraLeigh/1be29ec270f9371510ee3f81e2401464

The things I'd like to clean up and bring across for a laravel based security test are:

This is my working file so please excuse how crude it is. You can also ignore my todo notes / plans for a command that goes through and essentially checks everything I'd normally check and output a report so its easier for me to review the most common things.

nunomaduro commented 3 weeks ago

very interesting; probably willing to accept some of those other functions.

valorin commented 3 weeks ago

I think this is a great idea. 🙂

The random number classes (rand & mt_rand) should definitely be blocked, as they can be replaced with random_int(), except when seeding values. If that is needed, it should either be done through the new Randomizer class, or via something like https://github.com/valorin/random.

It might be worth adding mt_srand and srand into the list too, as these indicate seeding the insecure random number generator, which does flow on to everywhere else PHP does anything with the RNG.

The shuffle methods, str_shuffle',shuffle', array_rand' are also insecure and should definitely be flagged and replaced with a secure alternative like [PHP's Randomizer](https://www.php.net/manual/en/class.random-randomizer.php), my https://github.com/valorin/random package, or Laravel'sArrandCollection` helpers which are now cryptographically secure.

The random string generators, uniqid & tempnam, should be flagged too. Their output is insecure and predictable, and they usually get used in some form of security-through-obscurity unique naming. Str::random() does the same job, but with much more security.

These do sometimes get used internally just for generating unique names, with the argument that the timestamp base prevents collisions, but that's false given their timestamp is per-second or per-millisecond unique.

It becomes trickier with md5 and sha1 though. These are commonly used for checksums with older APIs, or generating unique internal keys (such as for cache). Flagging these is likely to throw more false positives than the above randomness functions. Although maybe this is an acceptable risk. Is there any way to toggle these specifically on/off within the context of the preset?

I would also recommend flagging eval, system, exec, passthru, shell_exec, etc, too. It's very easy to introduce RCEs through these functions, so devs should be pointed towards a Process wrapper that does proper escaping, etc.

A potentially controversial one is nl2br. It's often used on unescaped output, and can sometimes find some good XSS.

Also phpinfo - it can leak cookies and other sensitive information, so it should never be used.

Here it is: https://gist.github.com/ClaraLeigh/1be29ec270f9371510ee3f81e2401464

😍

  • Ensure All Models have policies

This is a nice idea - would be good as an optional arch test. Not sure if it makes sense for the preset, as some models may not be web exposed. Unless it works only for models which have controllers?

  • Doesn't have request->all() in the project (common mass assignment issue)

Yes, checking for this and other non-safe/validated data methods on $request would be a good idea for the preset.

  • Common sensitive db fields are hidden

Would need to be customisable, but a set of standard defaults would be a nice test.

Some more security arch test ideas: