Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.0.5 cocaine security fixes #5

Merged
merged 1 commit into from Dec 17, 2015
Merged

1.0.5 cocaine security fixes #5

merged 1 commit into from Dec 17, 2015

Conversation

longboardcat
Copy link
Contributor

Fastclone is using cocaine improperly, allowing malicious command injection.

@longboardcat longboardcat self-assigned this Dec 16, 2015
@@ -144,12 +144,16 @@ def clone(url, rev, src_dir)
initial_time = Time.now

with_git_mirror(url) do |mirror|
Cocaine::CommandLine.new("git clone --quiet --reference '#{mirror}' '#{url}'" \
" '#{File.join(abs_clone_path, src_dir)}'").run
Cocaine::CommandLine.new('git clone --quiet --reference', ':var')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be easier to read with separate named args, i.e.

Cocaine::CommandLine.new('git clone', '--quiet --reference :mirror :repository :directory')
  .run(mirror: mirror, repository: url, directory: File.join(abs_clone_path, src_dir))

@dlubarov
Copy link

LGTM though it would be nice to polish this later.

@longboardcat
Copy link
Contributor Author

Gonna clean it up

@bburky
Copy link

bburky commented Dec 16, 2015

The code isn't the prettiest, but it does look safe after reading through it. This is safe from the two POCs I provided. I'm really not an expert on Cocaine::CommandLine though.

I'd echo @dlubarov and suggest you use use multiple meaningfully named parameters in clone, thread_update_submodule and store_updated_repo.

It looks like you're using the interpolations from Cocaine correctly and safely now.

@longboardcat
Copy link
Contributor Author

Yeah, it's really difficult to follow right now. I'll revert it and add the meaningfully-named parameters tomorrow.

longboardcat added a commit that referenced this pull request Dec 17, 2015
@longboardcat longboardcat merged commit d455385 into master Dec 17, 2015
@longboardcat longboardcat deleted the jchang/fix-security branch March 30, 2016 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants