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

`` in author names are not escaped, allowing execution of arbitrary commands #17

Closed
ghost opened this issue May 4, 2013 · 5 comments
Closed

Comments

@ghost
Copy link

ghost commented May 4, 2013

Whenever an author name contains , they are output as-is in gnuplot files. gnuplot then runs the command that they contain, making it extremely unsafe to run gitstats on untrusted repositories, where author names could contain malicious command``.

@CiaranG
Copy link

CiaranG commented Oct 31, 2013

At least this case would be simply fixed by using single instead of double quotes around the author names when writing that file. That would prevent all kinds of similar problems as well.

@CiaranG
Copy link

CiaranG commented Nov 2, 2013

So just to clarify the above, simply was a foolish choice of word. Yes, using single quotes instead of double solves the problem, but that will fail if the author's name contains one. 'Tim O'Really' for example. It's not possible to escape that as ' either. In bash, you can do $'Tim O'\Really' (which is what I tried), but apparently you can't in the gnuplot file. I'll investigate what else might be possible when I have time.

@CiaranG
Copy link

CiaranG commented Nov 2, 2013

(the other simple/obvious option would be to just remove any backtick characters from the author name, but just doing that may well leave other avenues of attack open - I was looking for a solution that ensures the name is taken as a literal string no matter what)

@hoxu
Copy link
Owner

hoxu commented Dec 21, 2013

Indeed. I was kind of hoping some gnuplot expert would pop in and tip us on how to do this properly, but alas, we will have to do with the unoptimal blacklisting.

commit 5ba386aede189ce22f900fd9548d3135e46bd7ff
Author: Heikki Hokkanen <hoxu@users.sf.net>
Date:   Sat Dec 21 15:04:04 2013 +0200

    Remove backticks from author names passed to gnuplot.

    Without this, author names containing `touch /tmp/vulnerable` would cause said
    file to appear after generating statistics for the given repository.

    This is not an optimal solution. Instead of blacklisting characters we should
    either whitelist some, or find a safe escape mechanism for gnuplot.

Thanks for reporting the bug.

@hoxu hoxu closed this as completed Dec 21, 2013
@CiaranG
Copy link

CiaranG commented Jan 8, 2014

I think that solution is probably fine. I did have a good read of the documentation later, and I don't see any way of causing problems with any other characters other than the backtick.

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

No branches or pull requests

2 participants