Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Pwning the Nix ecosystem

ptrpa.ws

Post reply on HN
Print view View original

11–20 of 69 posts

Re: Pwning the Nix ecosystem

#11

Post by woodruffw »

ishouldbework wrote: post #10

> It is not possible for xargs to be used securely Eh... That is taken out of context quite a bit, that sentence does continue. Just do `cat "$HOME/changed_files" | xargs -r editorconfig-checker --` and this specific problem is fixed.

Yeah, I don't think the specific reason for that sentence in the manpage applies here. But the general sentiment is correct: not all programs support `--` as a delimiter between arguments and inputs, so many xargs invocations are one argument injection away from arbitrary code execution.

(This is traditionally a non-issue, since the whole point is to execute code. So this isn't xargs' fault so much as it's the undying problem of tools being reused across privilege contexts.)

Re: Pwning the Nix ecosystem

#12

Post by edoceo »

Mic92 wrote: post #9
jmclnx wrote: post #2

Well the "good" new is, OpenBSD and NetBSD still uses CVS, even for packages. So this will not work on those systems. I do not know about FreeBSD. Security by obscurity :) But I have been seeing docs indication those projects are looking to go to git, will see if it really happens. In OpenBSD's case seems it will be based upon got(1).

Don't they use email to accept contributions? Seems like security nightmare w.r.t to impersonation.

Aren't messages and/or patches signed?

Re: Pwning the Nix ecosystem

#13

Post by amluto »

I find it rather embarrassing that, after all these years of trying to design computer systems, modern workflows are still designed so that bearer tokens, even short-lived, are issued to trusted programs. If the GitHub action framework gave a privileged Unix socket or ssh-agent access instead, then this type of vulnerability would be quite a lot harder to exploit.

Re: Pwning the Nix ecosystem

#14

Post by hombre_fatal »

ishouldbework wrote: post #10

> It is not possible for xargs to be used securely Eh... That is taken out of context quite a bit, that sentence does continue. Just do `cat "$HOME/changed_files" | xargs -r editorconfig-checker --` and this specific problem is fixed.

Though that's like adding `{escapeHtml(value)}` everywhere you ever display a value in html to avoid xss.

If you have to opt in to safe usage at every turn, then it's an unsafe way of doing things.

Re: Pwning the Nix ecosystem

#15

Post by udev4096 »

Mic92 wrote: post #9
jmclnx wrote: post #2

Well the "good" new is, OpenBSD and NetBSD still uses CVS, even for packages. So this will not work on those systems. I do not know about FreeBSD. Security by obscurity :) But I have been seeing docs indication those projects are looking to go to git, will see if it really happens. In OpenBSD's case seems it will be based upon got(1).

Don't they use email to accept contributions? Seems like security nightmare w.r.t to impersonation.

How? It's signed with their keys. Linux kernel also uses mail lists and I have yet to see someone trying to impersonate someone

Re: Pwning the Nix ecosystem

#16

Post by zamalek »

woodruffw wrote: post #3

This is a great example of why `pull_request_target` is fundamentally insecure, and why GitHub should (IMO) probably just remove it outright: conventional wisdom dictates that `pull_request_target` is "safe" as long as branch-controlled code is never executed in the context of the job, but these kinds of argument injections/local file inclusion vectors demonstrate that the vulnerability surface is significantly large…

This is what GitHub says about it:

> This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow.

Which is comical given how easily secrets were exilfiltrated.

Re: Pwning the Nix ecosystem

#17

Post by woodruffw »

zamalek wrote: post #16
woodruffw wrote: post #3

This is a great example of why `pull_request_target` is fundamentally insecure, and why GitHub should (IMO) probably just remove it outright: conventional wisdom dictates that `pull_request_target` is "safe" as long as branch-controlled code is never executed in the context of the job, but these kinds of argument injections/local file inclusion vectors demonstrate that the vulnerability surface is significantly large…

This is what GitHub says about it: > This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow. Which is comical given how easily secrets were exilfiltrated.

Yeah, I think that documentation is irresponsibly misleading: it implies that (1) attacker code execution requires the attacker to be able to run code directly (it doesn't, per this post), and (2) that checking out at the base branch somehow stymies the attacker, when all it does is incentivizes people to check out the attacker-controlled branch explicitly.

GitHub has written a series of blog posts[1] over the years about "pwn requests," which do a great job of explaining the problem. But the misleading documentation persists, and has led to a lot of user confusion where maintainers mistakenly believe that any use of `pull_request_target` is somehow more secure than `pull_request`, when the exact opposite is true.

[1]: https://securitylab.github.com/resources/github-actions-prev...

Re: Pwning the Nix ecosystem

#18

Post by leeter »

woodruffw wrote: post #3

This is a great example of why `pull_request_target` is fundamentally insecure, and why GitHub should (IMO) probably just remove it outright: conventional wisdom dictates that `pull_request_target` is "safe" as long as branch-controlled code is never executed in the context of the job, but these kinds of argument injections/local file inclusion vectors demonstrate that the vulnerability surface is significantly large…

I don't disagree... but, there is a use case for orgs that don't allow forks. Some tools do their merging outside of github and thus allow for PRs that cannot be clean from a merge perspective. This won't trigger workflows that are pull_request. Because pull_request requires a clean merge. In those cases pull_request_target is literally the only option.

The best move would be for github to have a setting for allowing the automation to run on PRs that don't have clean merges, off by default and intended for use with linters only really. Until that happens though pull_request_target is the only game in town to get around that limitation. Much to my and other SecDevOps engineers sadness.

NOTE: with these external tools you absolutely cannot do the merge manually in github unless you want to break the entire thing. It's a whole heap of not fun.

Re: Pwning the Nix ecosystem

#19

Post by woodruffw »

leeter wrote: post #18
woodruffw wrote: post #3

This is a great example of why `pull_request_target` is fundamentally insecure, and why GitHub should (IMO) probably just remove it outright: conventional wisdom dictates that `pull_request_target` is "safe" as long as branch-controlled code is never executed in the context of the job, but these kinds of argument injections/local file inclusion vectors demonstrate that the vulnerability surface is significantly large…

I don't disagree... but, there is a use case for orgs that don't allow forks. Some tools do their merging outside of github and thus allow for PRs that cannot be clean from a merge perspective. This won't trigger workflows that are pull_request. Because pull_request requires a clean merge. In those cases pull_request_target is literally the only option. The best move would be for github to have a setting for allowing…

That's a fantastic use case that should be supported discretely!

Re: Pwning the Nix ecosystem

#20

Post by leeter »

woodruffw wrote: post #19
leeter wrote: post #18

Earlier quoted context omitted.

I don't disagree... but, there is a use case for orgs that don't allow forks. Some tools do their merging outside of github and thus allow for PRs that cannot be clean from a merge perspective. This won't trigger workflows that are pull_request. Because pull_request requires a clean merge. In those cases pull_request_target is literally the only option. The best move would be for github to have a setting for allowing…

That's a fantastic use case that should be supported discretely!

Why github didn't is beyond me. Even if something isn't merge clean doesn't mean linters shouldn't be run. I get not running deployments etc. but not even having the option is pain.
Post reply on HN