Project

General

Profile

Actions

Bug #2246

closed

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

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Story points:
1.0

Subtasks 2 (0 open2 closed)

Task #2521: Use git in --remote mode instead of checking out repository on head nodeResolvedTom Clegg04/08/2014Actions
Task #2564: Review 2246-fix-git-clone-raceResolvedTom Clegg04/08/2014Actions
Actions #1

Updated by Tom Clegg over 10 years ago

  • Release deleted (6)
Actions #2

Updated by Tom Clegg over 10 years ago

  • Target version set to 2014-04-16 Dev tools and data/resource management
Actions #3

Updated by Tom Clegg over 10 years ago

  • Story points set to 1.0
Actions #4

Updated by Tom Clegg over 10 years ago

  • Assigned To set to Tom Clegg
Actions #5

Updated by Tom Clegg over 10 years ago

  • Status changed from New to Resolved
Actions #6

Updated by Tom Clegg over 10 years ago

  • Status changed from Resolved to New
Actions #7

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.

Actions #8

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 (no cd &&), 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.

Actions #9

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.

Actions #10

Updated by Tom Clegg over 10 years ago

  • Status changed from New to Resolved
Actions

Also available in: Atom PDF