Hacker News Reader

Comment by inahga | Hacker News Reader

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

    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

View on HN

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.)

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.)

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!

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.

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.

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.

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.

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.

nit, CUE is independent from Google now, Marcel left to work on it full time years ago with some other folks, they started a company

The best thing I've seen is just regular JS or Python that outputs some data like JSON or protobuf. Google has a ton of DSLs that do this too, problem is they're DSLs and about 4 people fully understand them.

My point wasn't that YAML is an indefensible choice, but that embedding bash inside the YAML is at least to me not defensible. Making the `run` field take a string that's treated as a path to script within the repo would still give you all of the properties of "this config is data" without any of the nightmares that comes to multiple stacked layers of string interpolation in languages that are each already notorious for bugs from that sort of thing.

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.

There should be an array of arguments instead of space-separated string.

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

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.

> 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.

> 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.

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.

The problem is that if everyone is 10x faster with LLMs then we're also wrong ~10x as frequently as we were before

It can be that while also causing problems. It's like when a self driving car crashes, it's big news and everyone runs around like their pants are on fire. When a human driver crashes, it's a blip on local news or not even that.

Difference is in how you sell it.

We all know that people make mistakes. I.e. crash a car from time to time.

We all are sold a view that AI will save humanity, cure all diseases, solve all problems, allow for autonomous driving and many other (lies?).

While making basic mistakes or crashing on trivial crossroads..

Hype is way overblown.

The anti-hype is also way overblown. Nobody is saying that today's AI is omniscient and never makes mistakes. Future models will make fewer mistakes. They will still not hook into the universe with a cheat code and do everything at 100 percent reliability. It's a straw man.

Overblown? Not really. There is still not enough of criticism given asinine hype given by the LLM and adjacent companies.

Funny of you to mention strawman after presenting one.

The hype has significant real-world consequences such as nonsensical amounts of funding and valuations that could crash the world economy at some point. Until there some "check" on this system, i think the anti-hype is quite valid.

Probably because there are 1000x or whatever human driven miles compared to AI. And no one wants to be crippled by an AI have have to fight $billon_dollar_corp for compensation.

When a human driver controlling a bus crashes though it's a big story. Even bigger when it's a train. News scales with novelty and people impacted - 1 driver being a problem isn't an issue, an ai system used in many cars... That is more newsworthy.

CI is shell scripts as a service. There is no safe way to use it.

AI does it a lot faster and ignores rules even harder than humans do, but it's not the root problem here.

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.

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?

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

yes, it says "Now you can have beautiful clean workflows!" and links to a youtube video of a TV ad for a dermatologist.

Yes :-)

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

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.

Reply on news.ycombinator.com