Command injection in Thor::Actions#get

[ecneladis]

Open-uri's open used in Thor::Actions#get allows executing system commands [1].

Documentation does not warn that source parameter is vulnerable to malicious input.

Example:

get "|uname -a > cmd_exec_test;\nhttps://gist.github.com", "/tmp/test_123"

[1] http://sakurity.com/blog/2015/02/28/openuri.html

[hoffoo]

can someone look into this? It is now published and failing in bundle-audit https://nvd.nist.gov/vuln/detail/CVE-2016-10545

Name: thor
Version: 0.20.0
Advisory: CVE-2016-10545
Criticality: Unknown
URL: https://github.com/erikhuda/thor/issues/514
Title: Command injection in Thor Gem
Solution: remove or disable this gem until a patch is available!

Vulnerabilities found!```

[pherris]

To ignore this warning until a patch is available: bundle-audit check --ignore CVE-2016-10545

[xymbol]

Is someone working on this? How can we help? /cc @rafaelfranca

[rafaelfranca]

O_0. Who asked for that CVE without discussion this with the project maintainers?

[xymbol]

@rafaelfranca I don't know, IIRC it was recorded as reported by MITRE.

Just reviewed the code and the specs. I understand the vulnerability around open but don't see how a public exploit would work in this case. I don't recall user input in well-behaved apps ever being passed onto Thor actions.

@galori It's not trivial to disable if, say, you're using bundler-audit's task.

Maybe the original submitter can chime in? /cc @ecneladis

[rafaelfranca]

@reedloden do you know who submitted that CVE? MITRE says it came from HackerOne

[xymbol]

According to specs, seamless file and url access are by design. In this case, getting open out of the way would not reduce any risk.

[rafaelfranca]

Yes, this is by design. Thor is a system tool that unlikely will receive user input. Saying thor is vulnerable for command injection is the same thing than saying bash is vulnerable for command injection.

[ghiculescu]

rubysec/ruby-advisory-db#341 - I've proposed that ruby-advisory-db remove this as the consensus here seems to be that this isn't going to get changed in Thor.

[bosoxbill]

Seems like we could solve the CVE complaint with a documentation-only patch, no?

[rafaelfranca]

@bosoxbill that is reasonable to me. Feel free to open a PR, I'll merge.

[bosoxbill]

Pull is here: #611

[reedloden]

Hey folks -- sorry for delayed response.

First of all, based on this discussion, I've gone ahead and removed this issue from ruby-advisory-db, and I will have the CVE marked as REJECT.

This came about because the reporter of this issue had submitted a report to RubySec a while ago to get it added to the database, and it had never been processed. I went through all submissions, and while I did confirm that this issue was still valid in the current master branch, the actual potential for this is low based on how this gem is utilized, as discussed in this thread.

It would have been helpful if this particular issue had not been left open for over two years. Makes it difficult for me to know how upstream would consider the report, and I generally then err on the side of caution of considering it valid.

Anyway, sorry for any problems here.

[rafaelfranca]

Thanks for the context. This issue was open between the change in maintainers so I missed it totally, sorry.

I'll close the issue but also document on get that passing user inputs can lead to command execution.

[rafaelfranca]

And also for deal with rejecting the CVE.

[xymbol]

@rafaelfranca If help is needed to do a triage or validate pull-requests, please let me know.

[jimconner]

Just thought I'd let you know that even though you've already merged the documentation patch to address the CVE, it appears that yesterday thor has been added to the list of vulnerabilities that Snyk reports on. I just got a notification in my email from them https://snyk.io/vuln/SNYK-RUBY-THOR-22041 - From the remediation section in there I'm guessing that bumping the release to 0.21 might keep their scanning engine happy.

[reedloden]

@jimconner Snyk should note that the CVE was rejected. I don't think the thor project needs to do anything here.

[karenyavine]

Hey all,

I'm Karen, a Security Analyst @ snyk.io.
Thanks for bringing this to our attention! We've removed the vulnerability advisory from our database.