5 ms·

I probably would have made the same mistake. It is negligent to write GitHub Actions without using static analysis. Use zizmor in CI https://github.com/zizmorc

by inahga 1mo ago

I probably would have made the same mistake. It is negligent to write GitHub Actions without using static analysis. Use zizmor in CI https://github.com/zizmorcore/zizmor https://github.com/zizmorcore/zizmor error[template-injection]: code injection via template expansion --> .github/workflows/jira_issue.yml:24:29 | 22 | run: | | --- this run block 23 | # Escape special characters in title and body 24 | TITLE=$(echo '${{ github.event.issue.title }}' | sed 's/"/\\"/g' | sed "s/'/\\\'/g") | ^^^^^^^^^^^^^^^^^^^^^^^^ may expand into attacker-controllable code | = note: audit confidence → High = note: this finding has an auto-fix

madeofpalk 1mo ago
Github Actions is actually so incredibly scary to have on public repo. It's full of so many footguns that's far from obvious. It's a shame Github is buried under their current server issues, because it would be great to get improvements all of this - at least warning/erroring on these sorts of things themselves.
btown 1mo ago
This is a really cool tool! Would zizmor have caught the below as well? From the article: > The workflow had an if: condition that appeared protective: > if: (github.event_name == 'issues' && github.event.pull_request.user.login != 'whitesource-for-github-com[bot]') > However, on issues events, github.event.pull_request is always null. So the condition reduces to (null != 'whitesource-for-github-com[bot]'). This is always true, and every GitHub user passes the gate. Speaking broadly: it's a massive reminder that AI is trained on a veritable mountain of insecure GitHub Actions examples, many of which "fail open" in highly unpredictable ways even if widely used. Actions is almost unique in this regard, with the combination of a difficult-to-audit language and the type of privileged RCE environment that makes attackers salivate. (I do think that this stems in part from GitHub's often-inscrutable documentation, and a decision to release Actions without a robust security linting solution, leaving that to the community - but I do understand how it's an uphill battle, and we could have ended up with a much less flexible CI/CD system without this having shipped fast.)
- woodruffw 1mo ago
  zizmor wouldn’t catch that condition at the moment, although it does have similar checks for other unsound conditions and incorrect/vulnerable bot actor checks. This one wouldn’t be too hard to add, though. (Source: I am zizmor’s maintainer.)
  - btown 1mo ago
    That would be awesome - and thanks for your work, I'll want to start using it! Nullable event payloads silently null-coalescing to '' are a real "spooky action at a distance" kind of issue, because something that works perfectly when running and being QA'd on PRs, can silently fall apart if made to also run on the main/develop branch (which is only really monitored if the "build breaks," and thus a silent failure or skip might easily sneak through). Our codebase indeed has comments like this, without which we'd be totally lost: # Note contains('') is false if there is no PR at all e.g. on a push to develop, # so this will always run on pushes to the develop branch. if: ${{ !contains(github.event.pull_request.labels.*.name, 'ci-skip-backend') }} I imagine that's true of others as well!
- IshKebab 1mo ago
  Frankly it's absurd that you don't get a null reference error in that case. I knew Github Actions was bad... I didn't know it was that bad. This is 90% Github's fault.
netdevphoenix 1mo ago
Difference is you are not a trillion dollar plus technology hyped as a harbinger of civilisational change.
- SV_BubbleTime 1mo ago
  I mean… it’s only Monday! But yes, there is an interesting change in the past decade, where everything new must be over-hyped. Perhaps it is attention overload and needing to shout. Perhaps it’s that technological progress has significantly slowed while communication options have exploded (coincidence?). I look at it a lot like EVs. They’re great, if your use case is inside the specific band. But, that isn’t who they were being marketed to. And now… “pushback” is putting it lightly.
  - nomel 1mo ago
    > But yes, there is an interesting change in the past decade, where everything new must be over-hyped. Or, it's that the last two years have been the largest and fastest shift in the daily life of a programmer since the compiler, with near everyone moving, simultaneously, to this new tech, not because of hype, but because of practical personal benefit. Regardless, everything being hyped isn't new. There's always been silly hype in tech.
  - netdevphoenix 27d ago
    > But yes, there is an interesting change in the past decade, where everything new must be over-hyped. Yep, the good old "Our tech is making the world a better place" has gotten old and a new approach was needed. If you are not making standard databases obsolete with a blockchain or MapReduce or gaining business enlightenment from Big Data, no one wants to fund your startup. Civilisation level change is the new upper bound.
- inahga 1mo ago
  Sadly I'm not. Either way, how LLMs work mean that traditional software analysis tools are every bit as important as they were in the before times. This is why we see a lot of hype around LLMs and formal verification.
brewmarche 1mo ago
I get scared when I see these string interpolations in GitHub Actions. Use `env:` instead and just work with environment variables in your shell script. Yes, you still need to vet your script. Quoting is a common source of problems. Use shellcheck. Do not call eval/source/python/perl/whatever with untrusted input. But you removed one layer of problems already by not pasting a value into your shell script code directly.
- codedokode 1mo ago
  There should be an array of arguments instead of space-separated string.
dataflow 1mo ago
Would it be fair to say the blame falls squarely on Github? Why do they even allow pasting of arbitrary strings from a title directly into a script? And if they feel there is a reason, what did they imagine the safe way to do it was?
saghm 1mo ago
Shell scripts on their own already are so perilous without static analysis. I'll never understand how we ended up deciding that embed them in yaml instead of requiring an external script file was a reasonable idea.
- fizlebit 1mo ago
  yeah it is so mad: TITLE=$(echo '${{ github.event.issue.title }}' | sed 's/"/\\"/g' | sed "s/'/\\\'/g") Even in ruby it would have been something like TITLE=shell_escape(github.event.issue.title) where shell_escape could at least have unit tests. I don't know anything about github actions, but anything that takes user input needs to be processed with a language that uses static typing I believe to help ensure that the user input doesn't get into expressions given to other programs, shell, sql, or whatever. Unix was just not made for this sort of thing. It was made for in-house people to write patent applications.
- frollogaston 1mo ago
  Bash script embedded in yaml. Wow, I'd view each of those with heavy suspicion on its own, and they're nonlinearly bad together because of how special chars interact.
- jiggawatts 1mo ago
  YAML seems reasonable because it allows the sequence of steps to be treated as data, which then enables GUI visualisation, introspection, etc... without actually having to run anything. That's critical for a platform like GitHub and for devops pipelines in general. The failure is that "data" ends up being a "terrible custom DSL" that is bad at everything: Not good at data, not a good DSL, and not even a proper programming language. The best approaches I have seen to this kind of thing are: - Pulumi: You get to run custom code, but it outputs data. In other words, your "build automation script" must be a pure function taking data in and returning data out. The resulting data is then treated as the "thing" that the pipeline executes, which means that all decisions (parameters, inputs, etc...) have to be "baked in", before the pipeline starts executing. - Google CUE (Configure Unify Execute): lets you build up JSON using a strongly typed constraint language. Great for huge, complex configuration.
IncreasePosts 1mo ago
Is that project named after the serial nyc subway advertising dermatologist?
- tingletech 1mo ago
  yes, it says "Now you can have beautiful clean workflows!" and links to a youtube video of a TV ad for a dermatologist.
- woodruffw 1mo ago
  Yes :-)
cryo32 1mo ago
Proof that LLMs are trained on mediocre shit. That is by definition, mediocre shit.
frollogaston 1mo ago
Honestly I wouldn't make this mistake. I wouldn't know how to fix that particular loc while still using bash for this, but I'd know it's dangerous.

by inahga 1mo ago

madeofpalk 1mo ago

Github Actions is actually so incredibly scary to have on public repo. It's full of so many footguns that's far from obvious. It's a shame Github is buried under their current server issues, because it would be great to get improvements all of this - at least warning/erroring on these sorts of things themselves.

btown 1mo ago

This is a really cool tool! Would zizmor have caught the below as well? From the article: > The workflow had an if: condition that appeared protective: > if: (github.event_name == 'issues' && github.event.pull_request.user.login != 'whitesource-for-github-com[bot]') > However, on issues events, github.event.pull_request is always null. So the condition reduces to (null != 'whitesource-for-github-com[bot]'). This is always true, and every GitHub user passes the gate. Speaking broadly: it's a massive reminder that AI is trained on a veritable mountain of insecure GitHub Actions examples, many of which "fail open" in highly unpredictable ways even if widely used. Actions is almost unique in this regard, with the combination of a difficult-to-audit language and the type of privileged RCE environment that makes attackers salivate. (I do think that this stems in part from GitHub's often-inscrutable documentation, and a decision to release Actions without a robust security linting solution, leaving that to the community - but I do understand how it's an uphill battle, and we could have ended up with a much less flexible CI/CD system without this having shipped fast.)

woodruffw 1mo ago
zizmor wouldn’t catch that condition at the moment, although it does have similar checks for other unsound conditions and incorrect/vulnerable bot actor checks. This one wouldn’t be too hard to add, though. (Source: I am zizmor’s maintainer.)
- btown 1mo ago
  That would be awesome - and thanks for your work, I'll want to start using it! Nullable event payloads silently null-coalescing to '' are a real "spooky action at a distance" kind of issue, because something that works perfectly when running and being QA'd on PRs, can silently fall apart if made to also run on the main/develop branch (which is only really monitored if the "build breaks," and thus a silent failure or skip might easily sneak through). Our codebase indeed has comments like this, without which we'd be totally lost: # Note contains('') is false if there is no PR at all e.g. on a push to develop, # so this will always run on pushes to the develop branch. if: ${{ !contains(github.event.pull_request.labels.*.name, 'ci-skip-backend') }} I imagine that's true of others as well!
IshKebab 1mo ago
Frankly it's absurd that you don't get a null reference error in that case. I knew Github Actions was bad... I didn't know it was that bad. This is 90% Github's fault.

netdevphoenix 1mo ago

Difference is you are not a trillion dollar plus technology hyped as a harbinger of civilisational change.

SV_BubbleTime 1mo ago
I mean… it’s only Monday! But yes, there is an interesting change in the past decade, where everything new must be over-hyped. Perhaps it is attention overload and needing to shout. Perhaps it’s that technological progress has significantly slowed while communication options have exploded (coincidence?). I look at it a lot like EVs. They’re great, if your use case is inside the specific band. But, that isn’t who they were being marketed to. And now… “pushback” is putting it lightly.
- nomel 1mo ago
  > But yes, there is an interesting change in the past decade, where everything new must be over-hyped. Or, it's that the last two years have been the largest and fastest shift in the daily life of a programmer since the compiler, with near everyone moving, simultaneously, to this new tech, not because of hype, but because of practical personal benefit. Regardless, everything being hyped isn't new. There's always been silly hype in tech.
- netdevphoenix 27d ago
  > But yes, there is an interesting change in the past decade, where everything new must be over-hyped. Yep, the good old "Our tech is making the world a better place" has gotten old and a new approach was needed. If you are not making standard databases obsolete with a blockchain or MapReduce or gaining business enlightenment from Big Data, no one wants to fund your startup. Civilisation level change is the new upper bound.
inahga 1mo ago
Sadly I'm not. Either way, how LLMs work mean that traditional software analysis tools are every bit as important as they were in the before times. This is why we see a lot of hype around LLMs and formal verification.

brewmarche 1mo ago

I get scared when I see these string interpolations in GitHub Actions. Use `env:` instead and just work with environment variables in your shell script. Yes, you still need to vet your script. Quoting is a common source of problems. Use shellcheck. Do not call eval/source/python/perl/whatever with untrusted input. But you removed one layer of problems already by not pasting a value into your shell script code directly.

codedokode 1mo ago
There should be an array of arguments instead of space-separated string.

dataflow 1mo ago

Would it be fair to say the blame falls squarely on Github? Why do they even allow pasting of arbitrary strings from a title directly into a script? And if they feel there is a reason, what did they imagine the safe way to do it was?

saghm 1mo ago

Shell scripts on their own already are so perilous without static analysis. I'll never understand how we ended up deciding that embed them in yaml instead of requiring an external script file was a reasonable idea.

fizlebit 1mo ago
yeah it is so mad: TITLE=$(echo '${{ github.event.issue.title }}' | sed 's/"/\\"/g' | sed "s/'/\\\'/g") Even in ruby it would have been something like TITLE=shell_escape(github.event.issue.title) where shell_escape could at least have unit tests. I don't know anything about github actions, but anything that takes user input needs to be processed with a language that uses static typing I believe to help ensure that the user input doesn't get into expressions given to other programs, shell, sql, or whatever. Unix was just not made for this sort of thing. It was made for in-house people to write patent applications.
frollogaston 1mo ago
Bash script embedded in yaml. Wow, I'd view each of those with heavy suspicion on its own, and they're nonlinearly bad together because of how special chars interact.
jiggawatts 1mo ago
YAML seems reasonable because it allows the sequence of steps to be treated as data, which then enables GUI visualisation, introspection, etc... without actually having to run anything. That's critical for a platform like GitHub and for devops pipelines in general. The failure is that "data" ends up being a "terrible custom DSL" that is bad at everything: Not good at data, not a good DSL, and not even a proper programming language. The best approaches I have seen to this kind of thing are: - Pulumi: You get to run custom code, but it outputs data. In other words, your "build automation script" must be a pure function taking data in and returning data out. The resulting data is then treated as the "thing" that the pipeline executes, which means that all decisions (parameters, inputs, etc...) have to be "baked in", before the pipeline starts executing. - Google CUE (Configure Unify Execute): lets you build up JSON using a strongly typed constraint language. Great for huge, complex configuration.

IncreasePosts 1mo ago

Is that project named after the serial nyc subway advertising dermatologist?

tingletech 1mo ago
yes, it says "Now you can have beautiful clean workflows!" and links to a youtube video of a TV ad for a dermatologist.
woodruffw 1mo ago
Yes :-)

cryo32 1mo ago

Proof that LLMs are trained on mediocre shit. That is by definition, mediocre shit.

frollogaston 1mo ago

Honestly I wouldn't make this mistake. I wouldn't know how to fix that particular loc while still using bash for this, but I'd know it's dangerous.