Skip to content

Conversation

@longboardcat
Copy link
Contributor

Fastclone is using cocaine improperly, allowing malicious command injection.

@longboardcat longboardcat self-assigned this Dec 16, 2015

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.

4 participants