Consider & document security Implications of running arbitrary code through custom validation functions

[annakrystalli]

While ultimately it will be the responsibility of administrators to ensure any custom validation checks they run are safe, introducing the ability to run arbitrary code external to our software can saways pose a security threat to users.

It might be worthwhile to have a brief think about where and how things could go wrong and add a note as well as a disclaimer in our docs to bring this aspect to user attention.

[zkamvar]

Much of the risk will come from pull requests inserting code malicious code that will be run later in our workflows which can potentially read our AWS keys and other secrets.

One of the problems I've run into is that the pull_request target allows people submitting pull requests to insert arbitrary steps in the action to run them and they will run. This means that an attacker can modify these workflows to "pass checks."

This article about Keeping your GitHub Actions and workflows secure gives some scope around security basics and the differences between pull_request and pull_request_target triggers (though it's definitely an article aimed at people who have been mis-using pull_request_target).

In The Carpentries, we've used a pre-flight check to see if any workflow files were modified, and if they were, to alert the maintainer and not run any further checks: https://github.com/carpentries/actions/tree/main/check-valid-pr

[annakrystalli]

Thanks @zkamvar ! Do you think then that adding something like a check-valid-pr template to our hubverse actions repo and suggesting hub admins use them for protecting their hubs against malicious actors would be a good step in helping hubs address some of the security risks involved?

And if I'm understanding correctly, you don't see any potential serious security threat from running custom validation checks, right?

[zkamvar]

Do you think then that adding something like a check-valid-pr template to our hubverse actions repo and suggesting hub admins use them for protecting their hubs against malicious actors would be a good step in helping hubs address some of the security risks involved?

I think that would be a good idea. In fact, for security, we should just fork the carpentries actions repository as well.

And if I'm understanding correctly, you don't see any potential serious security threat from running custom validation checks, right?

I will preface this by saying that I am not a security engineer and I am likely overlooking something.

I think that there are potential security threats, but they all start with an attacker manipulating trust and getting a pull request submitted to a hub (and this can be even a small pull request that is a typo fix). GitHub blocks pull request workflows from running by new contributors to a repository until they get maintainer approval, but once someone contributes to a repository, each subsequent PR they create will run automatically (unless the hub admin provisions the PR to require a tag or comment or something to approve).

An attacker could insert malicious code via one of the following:

1. code directly in the repository
2. code that calls an external resource that will execute an attack
3. a dependency that will run the attack when loaded (see the left-pad incident)

One outcome could be running their code on GitHub to use up our runner time (which is a crypto-mining strategy).
Another one is to use our AWS storage as their own (though the permissions we set up should sufficiently limit the spread).
One other, more malicious outcome could be running their code on local machines.