Bug #2246
closedRace condition in crunch-job: multiple jobs use the same /tmp/crunch-job/ on head node to check out git trees and make archives. Fix as simple as using git archive --remote?
Added by Tom Clegg over 10 years ago. Updated over 10 years ago.
Updated by Tom Clegg over 10 years ago
- Target version set to 2014-04-16 Dev tools and data/resource management
Updated by Brett Smith over 10 years ago
This is probably out of scope for the branch, but I'll go on the record here: crunch-job has a number of potential shell exploits. Any backticks that interpolate user data are a likely risk. That may not seem like a big deal when the point of crunch-job is to run the user's arbitrary code anyway, but that means it's a good channel for privilege escalation attacks. An attacker who can only submit Jobs to the API server can leverage that power to get crunch-job to run arbitrary shell. I think this is serious enough to be worth fixing, especially since the fix is relatively straightforward (use multi-argument open()
calls to build pipes, or \Q$var\E
in backticks). This branch does make that job easier by eliminating the need to use a shell for these git calls (no cd &&
), so thanks for that.
my $repo = $git_dir || $ENV{'CRUNCH_DEFAULT_GIT_DIR'} || $Job->{'repository'};
Are you sure adding the repository field here will do what you mean? The rest of crunch-job is expecting $repo
to be a full path, but I think the repository field just has the name. Did I miss some magic somewhere?
All the other changes look good to me. Thanks.
Updated by Tom Clegg over 10 years ago
Brett Smith wrote:
This is probably out of scope for the branch, but I'll go on the record here: crunch-job has a number of potential shell exploits. Any backticks that interpolate user data are a likely risk. That may not seem like a big deal when the point of crunch-job is to run the user's arbitrary code anyway, but that means it's a good channel for privilege escalation attacks. An attacker who can only submit Jobs to the API server can leverage that power to get crunch-job to run arbitrary shell. I think this is serious enough to be worth fixing, especially since the fix is relatively straightforward (use multi-argument
open()
calls to build pipes, or\Q$var\E
in backticks). This branch does make that job easier by eliminating the need to use a shell for these git calls (nocd &&
), so thanks for that.
Yes, there are lots of assumptions in there. Putting a bunch of \Q\E in there sounds like a good idea. I poked around and added a few obvious ones.
Unfortunately \Q\E (like PHP's escapeshellarg) doesn't seem to consider the "empty string" case: `foo --bar \Q$baz\E --qux`
probably doesn't do what you want if $baz eq ''
. So I suppose the idiom is `foo --bar ''\Q$baz\E --qux`
...? (ouch)
my $repo = $git_dir || $ENV{'CRUNCH_DEFAULT_GIT_DIR'} || $Job->{'repository'};
Are you sure adding the repository field here will do what you mean? The rest of crunch-job is expecting
$repo
to be a full path, but I think the repository field just has the name. Did I miss some magic somewhere?
Indeed, this is a bit mysterious. The last resort lets you specify a local path when running a "local" job in your shell VM. Added comments to explain.
Updated by Brett Smith over 10 years ago
Unfortunately \Q\E (like PHP's escapeshellarg) doesn't seem to consider the "empty string" case:
`foo --bar \Q$baz\E --qux`
probably doesn't do what you want if$baz eq ''
. So I suppose the idiom is`foo --bar ''\Q$baz\E --qux`
...? (ouch)
I guess so. This is another reason to prefer multi-argument open() calls over anything that interpolates shell. But I think your current implementation works.
Thanks for expounding on the $Job->{repository} thing. I'd forgotten about the local directory use case (which is funny since I was using it to work on Docker crunch jobs). I think this is good to merge. Thanks again.