JS: promote js/actions/injection out of experimental
#9021
Conversation
erik-krogh
added
WIP
erik-krogh
removed
Awaiting evaluation
| * | ||
| * are equivalent. | ||
| */ | ||
| class MappingOrSequenceOrScalar extends YAMLNode { |
There was a problem hiding this comment.
Should this be in the standard library instead of in Actions.qll.
Actions can't be the only place where this liberal parsing is done.
| * ``` | ||
| * uses: actions/checkout@v2 | ||
| * ``` | ||
| * TODO: Does not currently handle local repository references, e.g. `.github/actions/action-name`. |
There was a problem hiding this comment.
Lets do something about this TODO, or track it elsewhere.
There was a problem hiding this comment.
The class is only used in the experimenal js/actions/pull-request-target query, and in that query it's only used to check if there is a actions/checkout step.
I think we can just remove the TODO: part (and keep the rest of the comment), since it doesn't affect any current query.
| exists(string regexp | regexp = "([^/]+)/([^/@]+)@(.+)" | | ||
| repositoryOwner = this.getValue().regexpCapture(regexp, 1) and | ||
| repositoryName = this.getValue().regexpCapture(regexp, 2) and | ||
| version = this.getValue().regexpCapture(regexp, 3) |
There was a problem hiding this comment.
Can we bind these in the getters instead of the charpred such that we still have Uses instances for the local repository references that we do not yet parse correctly?
| * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this `run` command. | ||
| * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions. | ||
| */ | ||
| string getAReferencedExpression() { |
There was a problem hiding this comment.
Could we avoid the TODO below if we rename the predicate to be more explicit about the limitations, i.e. getASimpleReferenceExpression?
In any case, the limitation should also have a positive and a negative example since the regexps are hard to parse mentally.
| The token has write access to the repository, and thus an attacker | ||
| can use it to modify the repository. |
There was a problem hiding this comment.
The GITHUB_TOKEN has write access by default, but it is configurable today: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token.
There was a problem hiding this comment.
I've tried to update the QHelp such that it should be clear that the GITHUB_TOKEN only sometimes has write access.
…add examples of what it can parse
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| class YAMLMappingLikeNode extends YAMLNode { | ||
| YAMLMappingLikeNode() { | ||
| this instanceof YAMLMapping | ||
| or | ||
| this instanceof YAMLSequence | ||
| or | ||
| this instanceof YAMLScalar | ||
| } | ||
|
|
||
| /** Gets sub-name identified by `name`. */ | ||
| YAMLNode getNode(string name) { | ||
| exists(YAMLMapping mapping | | ||
| mapping = this and | ||
| result = mapping.lookup(name) | ||
| ) | ||
| or | ||
| exists(YAMLSequence sequence, YAMLNode node | | ||
| sequence = this and | ||
| sequence.getAChildNode() = node and | ||
| node.eval().toString() = name and | ||
| result = node | ||
| ) | ||
| or | ||
| exists(YAMLScalar scalar | | ||
| scalar = this and | ||
| scalar.getValue() = name and | ||
| result = scalar | ||
| ) | ||
| } | ||
|
|
||
| /** Gets the number of elements in this mapping or sequence. */ | ||
| int getElementCount() { | ||
| exists(YAMLMapping mapping | | ||
| mapping = this and | ||
| result = mapping.getNumChild() / 2 | ||
| ) | ||
| or | ||
| exists(YAMLSequence sequence | | ||
| sequence = this and | ||
| result = sequence.getNumChild() | ||
| ) | ||
| or | ||
| exists(YAMLScalar scalar | | ||
| scalar = this and | ||
| result = 1 | ||
| ) | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Acronyms should be PascalCase/camelCase.
esbena
added
the
ready-for-doc-review
Co-authored-by: Steve Guntrip <12534592+stevecat@users.noreply.github.com>
I did a LGTM run on ~1000 projects to evaluate
js/actions/injection(see the internal backref).I really liked the 3 results it found, so I think we should promote this query basically as is.
I've expanded the QHelp a bit, such that it's clear you can use this vulnerability to steal tokens.
(Those tokens have write access 😱).
I've put it as a high precision warning query, because I think this is good enough to run by default.
Commit-by-commit review is recommended.
Evaluation was uneventful.