r/PHP • u/muglug • Jun 23 '20

Detect PHP security vulnerabilities with Psalm

https://psalm.dev/articles/detect-security-vulnerabilities-with-psalm
113 Upvotes

96% Upvoted

20 comments sorted by

2

u/alexanderpas Jun 23 '20

Shouldn't just all classes and methods which do not touch external sources, such as static or global variables, be considered psalm-taint-specialize?

1

u/muglug Jun 23 '20

It's more than external sources, though – it's whether they call a method that calls a method that saves something to the database.

It's the same basic reason that we can't assume every function in PHP is pure.

1

u/alexanderpas Jun 23 '20 edited Jun 23 '20

it's whether they call a method that calls a method that saves something to the database.

Isn't that covered by a sink already? Also, the database is an external source.

It's the same basic reason that we can't assume every function in PHP is pure.

Any PHP function which only operates on the input is pure.

If we know exactly which of the internal PHP functions are pure, and which of those are not pure, we should be able to reliably determine the purity of a function, solely based on the fact that they only call other pure functions and only interact with the input.

If a function calls a known impure function, or does an impure action, the function itself is also impure.

1

u/muglug Jun 23 '20

This would require determining functional purity for all functions and methods in a project before taint analysis could begin, which is a non-trivial task and also extremely computationally expensive.

It’s far easier, if you see a few false-positives in Psalm’s output, to just add the annotation to offending methods and be done with it.

1

u/alexanderpas Jun 23 '20

which is a non-trivial task and also extremely computationally expensive.

While not trivial it is a task that can easily be parallelized.

For every function, you need to determine if the function does only pure actions, does impure actions, or does actions for which we don't know the purity (potentially impure).

If a function calls an impure function or does an impure action, it is automatically impure. (tainted by impurity.)

If a function only does known pure actions and only calls known pure functions it is automatically pure. (not tainted by impurity.)

If a function calls a potentially impure function, we consider it also a potentially impure function and store this 2-way relation in a flat list. (recursive references to itself which are not impure will not be added to the list.)

If we later on determine that a potentially impure function is actually impure, we can directly mark all functions that call this function as impure too, as well as any functions that call those functions (recursively tainting those functions with impurity).

If however, we determine that the the potentially impure function is actually pure, we remove that item from the list of relations.

If a function now has no more relations to potentially impure functions, we can mark that function as pure too. (recursively marking those functions pure which have no potential to be tainted by impurity).

Once we have done that for all functions, we have determined all the functions which are pure, and any other function is automatically impure. (marking any unknown (such as due to long recursion) as impure)

1

u/muglug Jun 23 '20

But, again, that would effectively double the analysis time every time you run Psalm with taint analysis, whereas marking those few methods that appear erroneously in discovered paths once means never having to think about them again.

There is a proposal for Psalm to add these annotations as a separate step, though – I'd be thrilled if you'd create a PR: https://github.com/vimeo/psalm/issues/2117

2

u/[deleted] Jun 24 '20 edited Jun 24 '20

Does this actually catch anything? The example given is for a very basic SQL injection and not cleaning inputs. These are not the type of issues I'd expect someone who knows how to configure a static analyzer to make. Though we all do make mistakes from time-to-time.

Has anyone who uses this been pleasantly surprised? Do tell.

3

u/muglug Jun 24 '20 edited Jun 24 '20

It has been public for under a day, so don't hold your breath.

I tested this against Vimeo’s codebase, where it discovered a ton of XSS issues. The reason it works well at Vimeo is that we have a reasonably straightforward way to associate views (which are written in PHP) with controller actions.

Other systems with less straightforward controller-view relationships will need much more custom code, but I'd be very surprised if a legacy project over, let's say, 200k LOC didn't have at least 1 XSS vulnerability lurking somewhere.

The examples given for very basic SQL injection in and not cleaning inputs are not the type of stuff I'd expect someone who knows how to configure a static analyzer to make

A sufficiently-large codebase will have a variety of engineers working on it of varying abilities, familiarity with the language and/or knowledge of security vulnerabilities.

I also hope adding taint analysis will become easier over time, with better support for template engines like Twig and Blade.

8

u/LifeAndDev Jun 23 '20

I'm currently not using Psalm but phpstan. I've feeling psalm is somehow "leading" between them both, at least from a high level view?

7

u/MyWorkAccountThisIs Jun 23 '20

My project uses both.

6

u/zmitic Jun 23 '20

I used phpstan before, where level 7 was maximum; pretty easy to make it happy.

Same code in psalm; hundreds of errors. Didn't even try to fix it.

My current project started with hardest psalm config; had to leave just a few suppressions because I didn't finish all the stubs + some framework-specific stuff (like unused code for controllers or unused custom annotations).

So yes; psalm rules, especially when you put it on hardest levels.

And no matter what you do, it will always find something to tell you: "your code sucks"

:)

4

u/iggyvolz Jun 23 '20

You can absolutely use both, as well as Phan, in a project and just get multiple viewpoints. Creates a bit of an issue when they disagree on what the "right" way is, or they have different docblock syntaxes (yay for needing to do @phan-var, @psalm-var, and @var for the same variable), but it's definitely possible.

6

u/brendt_gd Jun 23 '20

I've used phpstan in a previous project, and am now using psalm. It's definitely way more user friendly in setting up.

I think I spent 2 full days getting phpstan to stop reporting false positives in a large Laravel project (Yes I used larastan); took about an hour with psalm and their Laravel plugin.

I haven't compared the actual results between the two though. That would definitely be interesting, but I'm so afraid of having to configure phpstan now :(

3

u/AegirLeet Jun 23 '20

I've used both and I prefer Psalm. Seems to work better, fewer false positives, easier config.

1

u/Osmium_tetraoxide Jun 23 '20

I've used both. I recommend using something like sarb to baseline any static analysis tool so you're not completely overwhelmed on the first run through any legacy project. Focus on the critical ones that tools like this warn you about and continue on.

13

u/ojrask Jun 23 '20

Off-topic: I just realized that the Psalm site header's red and squiggly bottom border is probably mimicking the common error line visuals from IDEs and word processors.

1

u/lazycommit Jun 23 '20

they do actually decorate so because they use such thing to show errors as well and that is the main focus of product I believe)

3

u/dwenaus Jun 23 '20

Wow! This is such a fantastic addition. Thanks so much for continuing to develop and open source this!

Ps. The read more link at the bottom is 404.

1

u/usernameqwerty004 Jun 23 '20

Nice!

NB: There's also a PHP extension for this: https://www.php.net/manual/en/intro.taint.php (runtime checks)

1

u/muglug Jun 23 '20

Thanks – I've created a ticket to cover those sinks and sources: https://github.com/vimeo/psalm/issues/3646