tcocca / active_pdftk

ruby wrapper for the pdftk command line utility for working with editable pdf files
MIT License
46 stars 37 forks source link

Should not concat everything into a string to make the cmd #27

Open darkhelmet opened 12 years ago

darkhelmet commented 12 years ago

Big security hole and doesn't work when any filename has spaces in it.

tcocca commented 12 years ago

Daniel, do you have any suggestions here? How can't we call the command if we are not creating a string of the command?

Thanks for the interest in the project.

darkhelmet commented 12 years ago

Check out the docs for popen3 http://www.ruby-doc.org/stdlib-1.8.7/libdoc/open3/rdoc/Open3.html

It works like system in that it can take variable args which it properly passes. It's the easiest way to safely pass args to a process like that. I've already got a branch mostly working, but all your documentation regarding return types (holy shit, nice docs) and what not isn't applicable anymore (haven't gotten to updating that).

tcocca commented 12 years ago

Cool, I'll check it out. I'd love to see the progress you have made here. Eagerly awaiting a pull request. I think this fork commit: https://github.com/roend83/active_pdftk/commit/702983e31d1e5ecafc0922e93d54b3283453cbad might fix the error when the filename has spaces in it.

What is the deal with the return types no longer working correctly? If you are passing args can you no longer specify a return type in your fork?

darkhelmet commented 12 years ago

All the return types for the methods that build the args show Strings, where now they would be Array. Just a documentation thing.

tcocca commented 12 years ago

OK sounds good, looking forward to your patch/pull request.

elmatou commented 12 years ago

Hi Guys,

Daniel, As you mentioned, I see that we can build the system call around an array of values, ok. But what is the point ? can you be more precise on the security issue, I didn't find any references about this.

darkhelmet commented 12 years ago

This is security 101: http://cwe.mitre.org/data/definitions/78.html

elmatou commented 12 years ago

Thx, I finaly get the point on the shell expension trick. We definitivly should pass the command as an array.