SARIF support (GitHub Codescanning)

[LukasReschke]

This PR adds SARIF support to Psalm. SARIF is an open-standard for exchanging static code analysis data and is also used by GitHub Codescanning.

Adding SARIF support to Psalm enables triaging reports within the GitHub UI. For taint flow vulnerabilities, it will also display the whole path and code inline.

How to set this up with GitHub Codescanning

A workflow file like the following should be able to upload the results on every push and after cron schedule. You still will have to install psalm though. (e.g. using composer)

name: "Upload SARIF"

# Run workflow each time code is pushed to your repository and on a schedule.
# The scheduled workflow runs every at 00:00 on Sunday UTC time.
on:
  push:
  schedule:
  - cron: '0 0 * * 0'

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
    # This step checks out a copy of your repository.
    - name: Checkout repository
      uses: actions/checkout@v2
    - name: Run Psalm
      run: psalm --report "results.sarif" --taint-analysis 
    - name: Upload SARIF file
      uses: github/codeql-action/upload-sarif@v1
      with:
        # Path to SARIF file relative to the root of the repository
        sarif_file: results.sarif

Screenshots

TODO

• Add tests
• Cleanup (remove workflow code + test.php + fixup commits)
• Document how this can be used in a GitHub workflow
• Establish best path forward for rule identifiers

[muglug]

Awesome – let me know if you need help with anything.

cc @josepalafox

[muglug]

This output format should, I think, be the default when running Psalm with --taint-analysis inside GitHub Actions.

[josepalafox]

+💯 ping us if you need any support or want some swag! Thx for adding this! josepalafox @ github

[muglug]

Once this is done I'll add some functionality that sends the generated SARIF reports to the panel from external CI environments. Vimeo uses Jenkins + self-hosted GitHub Enterprise, and getting that hooked up to the security pane will be great for surfacing Psalm's output.

[LukasReschke]

@muglug, there is one thing that I'd like to figure out the best solution for: Rule Identifiers.

SARIF uses rules to map types of issues together. For example a rule could be:

• Identifier: 114
• Name: PossiblyInvalidPropertyFetch
• Description: Emitted when trying to fetch a property on a value that may not be an object or may be an object that does not have the desired property. See https://psalm.dev/114

Findings would then itself define back what rule identifier they belong to. This allows you to filter them easily. The GitHub UI also supports this. For example a finding could be:

• Rule Identifier: 114
• Message: Message generated by Psalm for this issue
• Codeflows: ...

For non-taint-flow issues this works fine. Everything has a different identifier and gets mapped properly.

As soon as it is about taint-flow issues, they all receive the short code 205 though:

psalm/src/Psalm/Issue/TaintedInput.php

Line 10 in 516141a

public const SHORTCODE = 205;

Which results in a less than optimal user experience:

In my opinion, the right solution here would be to create independent rule identifiers for each type of vulnerability. (XSS, SQL, Shell, ...). But I am not really deep in the Psalm code and am not sure how much effort that would take.

As a quick workaround, I could also filter for the type "205" and as identifier use the message such as "Detected tainted html" or "Detected tainted SQL". Not quite optimal, as that would depend on these strings not changing :-) (that said, the worst that could break are pre-existing filters)

[LukasReschke]

As a general "nice to have for later" note: SARIF also supports passing rule descriptions in Markdown. We could include the descriptions from the website such as https://psalm.dev/docs/running_psalm/issues/PossiblyInvalidPropertyFetch/ in the results.

But as we only have one identifier for taint-flow vulnerabilities that isn't super exciting yet. That said, if you tell me how to split these into several, or volunteer to do that yourself, I'd be happy to draft documentation for each type :-)

[muglug]

@LukasReschke I'll add more specific issues for all of those like TaintedHtml, TaintedUserSecret etc.

[muglug]

@LukasReschke see 43af3b1

[LukasReschke]

Awesome, @muglug. Let me rebase and give it another try :)

I did the following changes now:

1. Rule ID is now the short code (205 currently - but others in the future)
2. Rule name is now the class name (TaintedInput)
3. Rules ship the the markdown from /docs/running_psalm/issues/$RULE_NAME.md (if available)

The UI will now look as following:

[muglug]

@LukasReschke you may or may not want to strip the example PHP code from the markdown – not sure what the optimal UX is there.

[weirdan]

I'm curious what the UI looks like with the final version.

[LukasReschke]

@muglug, let's keep the sample PHP code and I'll just follow-up later with refining the documentation a bit :)

I think this should be ready to review now. It will now look as attached:

Issue List

Issue View - Not Expanded

Issue View - Expanded

[LukasReschke]

Filtering by rule ID is also working now:

[muglug]

Looks great, thanks!

[muglug]

Do you think there should be a separate GitHub Action for security scanning?

The existing one is here: https://github.com/psalm/psalm-github-actions

[LukasReschke]

@muglug, how about an argument to the Action to perform taint analysis? Then one can just choose whatsoever one want. (or have two jobs - one with and one without)

     - name: Psalm
       uses: docker://vimeo/psalm-github-actions
       with:
        security_analysis: true
    - name: Upload SARIF file
      uses: github/codeql-action/upload-sarif@v1
      with:
        sarif_file: results.sarif

[muglug]

Sure - moved discussion to psalm/psalm-github-actions#19

[nickfyson]

👋 Hi @LukasReschke!

I'm from the Code Scanning team at GitHub and we'd love to include Psalm as one of our starter workflows (which as you've probably seen, appear as tiles in the first-run experience for each repo). The above workflow looks nearly ready to go, but as you noted, still needs installation of the Psalm tool itself. Do you have a full example that works out-of-the-box for an ubuntu-latest runner? All we'd then need in addition is an SVG logo for Psalm and a brief description of the tool.

Thanks again for adding SARIF support, and bringing PHP support to Code Scanning! 🙂

(FYI At present we manage all our starter workflows directly, but will soon be pulling them from the actions/starter-workflows repo.)

[LukasReschke]

@muglug - can you help here? I guess https://github.com/psalm/psalm-github-security-scan would be the resoruce to point to?

(I will likely have some resources to commit to some Taint flow changes in the soon future :) )

[nickfyson]

Following that link, I think a suitable workflow would be as follows (including templating for the standard code scanning triggers). Does this seem correct? :-)

name: Psalm Security Scan

on:
  # following standard triggers for code scanning
  push:
    branches: [ $default-branch, $protected-branches ]
  pull_request:
    branches: [ $default-branch ]
  schedule:
    - cron: $cron-weekly

jobs:
  psalm:
    name: Psalm
    runs-on: ubuntu-latest
    steps:
      - name: Checkout code
        uses: actions/checkout@v2

      - name: Psalm Security Scan
        uses: docker://ghcr.io/psalm/psalm-security-scan
        with:
          security_analysis: true
          report_file: results.sarif
      - name: Upload Security Analysis results to GitHub
         uses: github/codeql-action/upload-sarif@v1
         with:
           sarif_file: results.sarif

I had a quick look at the repo, but is there a specific Psalm logo?

[imjohnbo]

Following that link, I think a suitable workflow would be as follows (including templating for the standard code scanning triggers). Does this seem correct? :-)

That workflow looks good from a sanity check over here! And looks like the icon could be potentially be from the Psalm org page?

Could you verify for Psalm to be added to the starter workflows, @muglug? 🙏

[josepalafox]

@muglug I think @nickfyson and @imjohnbo is just waiting for a +1 from you to get this merged into the GH UI. Can you comment on the logo and other requests?