Exec($_GET(github.com)

212 pointsby throwaway204812 yıl önce122 comments

https://github.com/search?q=exec%28%24_GET&ref=cmdform&type=Code

33 comments

This is awful. Shell commands are not guaranteed to be idempotent, people! These should all be of the form exec($_POST, not exec($_GET.

izietto12 yıl önce

It depends by the command, echo is idempotent for example. There should be some checking like

  $cmdname = split(' ', $_GET['command']);
  if(!in_array(IDEMPOTENT_COMMANDS, $cmdname))
     echo '<h1>Your request is not guaranteed to be idempotent. Please use a POST.</h1>';
  else
    exec($_GET['command'] ...

TeMPOraL12 yıl önce

    if(!in_array(IDEMPOTENT_COMMANDS, $cmdname)) {
     header("HTTP/1.1 405 Method Not Allowed");
     die();
     }

Fixed ;).

nexxer12 yıl önce

in_array(needle, haystack)

PHP with its argument ordering strikes again :)

pygy_12 yıl önce

... actually,

    if(!IDEMPOTENT_COMMANDS[$cmdname]) { ...

would do, and, assuming dictionary lookups are optimised, is possibly faster.

RutZap12 yıl önce

actually,

   if (! isset(IDEMPOTENT_COMMANDS[$cmdname]))

otherwise you'll get an undefined index notice :)

pygy_12 yıl önce

Indeed. My PHP-fu is a bit rusty... I tested `if(NULL)...` in the REPL and since it was working, I left it at that.

izietto12 yıl önce

Damn!!! :D

dspillett12 yıl önce

  echo is idempotent

As a simple call, yes. But there are many shell tricks (redirection, command substitution, process substitution) that can make an echo call have significant side effects - so if you were daft enough to be considering this you'd need to do much more checking before submitting the provided instruction to your shell, and those checks would need to know which shell you were targeting (in fact you'd probably want to force the issue by exec()ing a specific shell instead of just using the default for the user the code is running as).

izietto12 yıl önce

Yes, though if a shell is chosen and the command is shell escaped you are good to go

RutZap12 yıl önce

I think the problem here is the fact that tainted variables (user input) are used to execute shell commands. it doesn't matter if that's $_POST or $_GET, both of these are user input and therefore these are huge vulnerabilities.

kvcrawford12 yıl önce

I'm sorry, but I can't hear you over the sound of the joke flying overhead.

cassm12 yıl önce

I think he is was being indeed sarcastic.

saurik12 yıl önce

"That's the joke."

masklinn12 yıl önce

I'm pretty sure GP was being sarcastic.

RutZap12 yıl önce

Whoops! My bad.. now I feel stupid :)

huseyinkeles12 yıl önce

I think he is being sarcastic.

oleganza12 yıl önce

I think he was indeed sarcastic.

[deleted]12 yıl önce

gnurag12 yıl önce

I think he is indeed being sarcastic.

venomsnake12 yıl önce

I prefer exec($_REQUEST when I have to do something like that. You capture both get and post variables.

[deleted]12 yıl önce

RoryH12 yıl önce

Love the sarcasm ;-)

mikeytown212 yıl önce

I found a legit repo for this code https://github.com/andresriancho/w3af-moth "A set of vulnerable PHP scripts used to test w3af's vulnerability detection features."

elwell12 yıl önce

Which points out that the context has to considered as well. E.g., if it's a personal intranet or password protected page, this may not be vulnerability.

robbles12 yıl önce

It would still be vulnerable to a CSRF attack. The attacker can just get a logged-in user to launch their exploit, through a vulnerable site they frequent or even a link or image in an email.

blueskin_12 yıl önce

Theoretically, building CSRF protection in isn't mutually exclusive with passing unsanitised variables to a shell. Although sure, most people who do the latter won't do the former.

xai12 yıl önce

What if a hacker would have access to a user of said personal intranet, or would find a way to bypass the password protected page. Then he'd be able to execute anything on the server, even stuff the user he hacked shouldn't be able to access. So no, this is always a vulnerability.

vz012 yıl önce

What about phpMyAdmin and others alike webapps? Are they inherently insecure?

coldtea12 yıl önce

Those are different, as they only do what they are meant to do (give access to the databases).

They don't give untintended full access to the web server.

That said, they are a little insecure.

blueskin_12 yıl önce

>They don't give unintended full access to the web server.

https://en.wikipedia.org/wiki/Webmin

Maybe not unintended, but definitely full access, and the world is almost certainly full of outdated/non whitelist access/weakly passworded panels.

Also, on a a sufficiently misconfigured server, you could always use \! (mysql's shell_exec, etc.) with phpmyadmin etc. to open a remote shell somewhere, then work from there.

[deleted]12 yıl önce

utunga12 yıl önce

Yes. No.

deckiedan12 yıl önce

As a theoretical aside, I wonder if it'd be possible to have a typesystem based solution to these kinds of problems - where variables coming from the user (or from another program) are considered 'unsafe' and the compiler refuses to let exec() or whatever use them until they've been through a cleaner/tester of some kind... (OK, I know PHP doesn't have a compiler as such - but a static checker of some kind could work the same...)

gst12 yıl önce

https://en.wikipedia.org/wiki/Taint_checking

_pmf_12 yıl önce

Of note is that Perl had this since at least 1998. See http://gunther.web66.com/FAQS/taintmode.html

But of course we just laugh about Perl and pat ourself on our backs with our safe new languages because we clearly know much more than those anachronistic neckbeards.

brohee12 yıl önce

The issue I have with Perl taint checking is that data is untainted by a group match within a regexp.

It's not explicit enough and it's easy enough to find legitimate code with accidental untainting of dangerous data.

Ruby requires an explicit untaint call, and IMHO it's the right way to go.

Silhouette12 yıl önce

But of course we just laugh about Perl and pat ourself on our backs with our safe new languages

And there is some justification for that: if those "safe new languages" are doing the type checking at compile time, that is better than only finding out you have a safety issue when you fail at run time.

deckiedan12 yıl önce

Ah, "Taint" was the missing magic word I couldn't figure out to aid my google-fu (well, duckduckgofu...). Thanks for that - there is a lot of interesting stuff out there. :-)

Argorak12 yıl önce

On the practical side, Ruby (MRI specifically) has such a system ($SAFE), that taints certain objects (e.g. those gained by IO) and only allows you to use them in certain fashions (e.g. only after explicit handling).

It's leaky as hell, because all components have to get the marking right. Also, not all Objects have equal trust levels. Objects created due to a HTTP request (GET-Parameters) should certainly be tainted, but how about Object read through IO - do we trust out filesystem? Do we trust the database? Thats more of an architectural decision.

In the end, the problem comes down to this: you have to whitelist the world and everything you miss is an error.

sltkr12 yıl önce

Then people notice that exec($_GET['foo']) doesn't work, but some smart cookie figures out that exec(untaint($_GET['foo'])) (or whatever the syntax is for removing the taint from an input variable) does, posts it online, and everyone copies that instead without thinking about the consequences.

Morgawr12 yıl önce

There is Argos[0] which is a research project at my university, it's a qemu-based emulator that tracks tainted data coming from the network and other compromised sources and makes sure it doesn't tamper with memory or filesystem locations than it shouldn't. It's a bit overkill,especially for this, but it's interesting to check out :)

[0] http://www.few.vu.nl/argos/?page=1

riffraff12 yıl önce

perl and ruby have "taint" which is similar to what you describe.

And yes, this can be encoded in the type system and you can also make it so the sanitization is context dependent, i.e. http://www.comp.nus.edu.sg/~prateeks/papers/csas-ccs11.pdf

jonnyscholes12 yıl önce

PHP does to http://pecl.php.net/package/taint but I haven’t used it or the perl/ruby ones so I'm not actually sure how similar they are...

hellerbarde12 yıl önce

How ironic.

chriswarbo12 yıl önce

Here's one approach: http://blog.moertel.com/posts/2006-10-18-a-type-based-soluti...

Different languages (SQL, JS, HTML, Shell, Plaintext, etc.) are treated as different types. Language-specific functions only accept arguments of the relevant type (eg. shell_exec takes Shell, db_query takes SQL, etc.). User input is Plaintext (usually; sometimes it might be something more specific like BASE64).

Different languages can't be combined (eg. SQL can't be concatenated with Plaintext), but they can be converted down to Plaintext and Plaintext can be converted to any language via escaping functions. This avoids injection attacks, since the only way to please the type-checker is by escaping properly.

slight12 yıl önce

PHP has a runtime taint checker module: http://www.php.net/manual/en/book.taint.php

adamnemecek12 yıl önce

http://www.impredicative.com/ur/

shakesbeard12 yıl önce

https://wiki.php.net/rfc/taint

zoomerang12 yıl önce

It's pretty easy to implement such a feature in any strongly typed language, but sadly few framework/libraries/stacks actually do it.

MichaelGG12 yıl önce

Phantom types can help with this. F#'s unit of measurement does something similar for numbers.

nathancahill12 yıl önce

Note that most of these are intentional. pointer.jpg.php, pypwn/test.php, w3af-moth, etc.

brianbarker12 yıl önce

My fav:

$device = $_GET['device']; $state = $_GET['state'];

exec( "sudo ./send " . $device . " " . $state );

EDIT: it is for home automation, but also appears to be a CS class group project.

voltagex_12 yıl önce

I really, really, really want to post on their issue tracker, but maybe I shouldn't. There's no guarantee the person grading the project will care, either.

marlin12 yıl önce

You should. But be factual. No point in bringing the attitude.

voltagex_12 yıl önce

The repo in question is 2 years old. It may be worth getting in contact with the uni who runs that course though.

hashx12 yıl önce

Directly passing user data to the command line is highly dangerous.It allows an attacker to execute arbitrary commands on the command line [0]. escapehellarg [1] has to be used to Escape a string to be used as a shell argument

[0] http://gcattani.co.vu/2013/03/a-tale-of-a-php-shell/ [1] http://php.net/manual/en/function.escapeshellarg.php

fleitz12 yıl önce

Directly doing anything with attacker supplied data is generally a no-no.

Everything that may come from a user must be filtered, escaped or generally treated as hostile.

As an example on an IRC channel someone once made their chan bot log the channel to the web, all it took was pasting javascript into an IRC window, and typing "LOL look at this! http://stupidbot.com/ircweblog". Channel pwned.

catmanjan12 yıl önce

"We've found 99,841 code results"

Someone should write a script that automatically raises an issues for each line and each project, it's probably possible, but I'm chronically lazy.

JelteF12 yıl önce

I remember reading about something like this and Github their response was that the guy should stop according to their ToS. Their ToS blocks stuff like this to prevent spamming.

rplnt12 yıl önce

As noted in other comment(s), there are legit uses for this.

ircmaxell12 yıl önce

Can you please provide just one? No other comment does that.

And to be clear, I'm talking about providing at least one legit use for passing user input directly to exec without any kind of filtering...

TheCoelacanth12 yıl önce

Test cases for a PHP vulnerability scanner[1]

[1] https://news.ycombinator.com/item?id=7665232

scott_karana12 yıl önce

The sourcecode of a hypothetical Github commenting bot searching for this vulnerability will have the same search token, and will be flagged.

andrewryno12 yıl önce

A great example is: https://github.com/andresriancho/w3af-moth

He deliberately wrote vulnerable code to test his auditing script. There are more repos like this.

ircmaxell12 yıl önce

That's fair. But that's also the vast minority of these results that I can tell...

monksy12 yıl önce

I thought about doing the exact same thing. But I wonder about the: limiting of the issue creation API on github and false positives. [The variable has been sanitized prior to being executed]

tlrobinson12 yıl önce

I don't like Issue bots, but it could actually be a huge value-add for GitHub to integrate these kinds of checks (as well as hardcoded credentials, etc) for their users.

munimkazia12 yıl önce

Well, some of them are false positives.

marlin12 yıl önce

After looking at the search result i can see that most of the results have legit reasons.

Instead of half-assing the problem, please dedicate 10 minute of your life to look in, analyze & report one or two of the problems you find.

Also explain why you think this is an security issue.

You will:

  * help someone out by pointing out an issue

  * hopefully educate the person how to write better code

  * educate yourself in reading and understanding others spaghetti code

jafingi12 yıl önce

This seems pretty secure.. https://github.com/searchmanipulator/missteencincinnati/blob...

WestCoastJustin12 yıl önce

As a side note, this is how many SQL injection attacks happen too. You almost never want unfiltered user input to directly interact with your system. A while back, I did an episode on how SQL injection can lead to code execution by using unfiltered user input on a LAMP stack. See it @ http://sysadmincasts.com/episodes/21-anatomy-of-a-sql-inject...

ASneakyFox12 yıl önce

Side side note. This same carelessness is what caused the heart bleed bug.

Kim_Bruning12 yıl önce

Well, in a sense maybe, but not on purpose there. The programmer had missed a bounds check.

ASneakyFox12 yıl önce

I think you're giving him too much credit. The input was not sanitized. Now its no one programmers fault. It was a long living bug many had a chance to see it and correct it for a long time. It was rooted in the same carelessness as exec(GET)

rallemoose12 yıl önce

Meet his brother https://github.com/search?q=eval%28%24_GET&type=Code&ref=sea... (eval($_GET

lauriswtf12 yıl önce

Also relevant - https://github.com/search?q=mysql+%24_GET&type=Code&ref=sear...

bowlofpetunias12 yıl önce

The names of the files alone suggests many of these are quite deliberate.

elwell12 yıl önce

most of those are demonstrations of the vulnerability

VMG12 yıl önce

Are there any more sophisticated parses that also find the non-obvious cases?

It should be easily doable to write a tool that finds an exec() of a variable that was assigned a $GET etc

theon14412 yıl önce

I think it's easier to just avoid exec() altogether...

Xylakant12 yıl önce

Sadly, there's cases where exec() is impossible to avoid - for example, every kind of tool that doesn't have a proper library and language bindings. See git and the grit library for an example.

VMG12 yıl önce

True, but there is a lot of software out there where people don't do that. Might be interesting to find vulnerabilities that are slightly less obvious.

mback2k12 yıl önce

Other examples primarily for Python, but also including Ruby and JavaScript:

- "eval(raw_input())" --> https://github.com/search?q=%22eval%28raw_input%28%29%29%22&...

- "eval(request" --> https://github.com/search?q=%22eval%28request%22&type=Code&r...

- "eval(request.POST" --> https://github.com/search?q=exec%28%24_POST&type=Code&ref=se...

- "eval(request.GET" --> https://github.com/search?q=%22eval%28request.GET%22&type=Co...

DanOWar12 yıl önce

Example from the Github search:

     <?php
          $result=shell_exec("cat ".$_GET['name'].".txt");
          echo $result;
     ?>

How to abuse:

     $_GET['name'] = "/dev/null; rm -rf /; echo ";

[deleted]12 yıl önce

izietto12 yıl önce

Wrong. `rm -rf /` fails without the `--no-preserve-root` option.

moloch12 yıl önce

'rm -rf /*' should do the trick; kind enough to leave them with a '/' directory.

xikrib12 yıl önce

need root access, no?

LeonM12 yıl önce

Thats not the point. Of course "rm -rf /" won't work, but what about downloading and installing a backdoor? Or modifying the website scripts itself? Or dumping a database? Or...

Removing all the files from a filesystem is something only a script kiddy would do, and it's probably a "best case scenario" for the owner of the server, because the impact of that is relatively small (just re-install the server and restore the backups). But once the attacker starts injecting mallware, stealing customer information (credit card numbers anyone?) or anything else nasty they can think of that they would benefit from, then you are in a whole lot more trouble...

skriticos212 yıl önce

True, but once you have <foo; $any_command_with_user_privilege> you can start executing any user commands. So you do a </dev/null; mkdir ~/www/nefarious; cp ~/www/AdminSettings.php ~/www/nefarious/settingns.txt;> (without the index file you can just view the file as plain text, which probably contains the database username and password. Then you can go on and download a database dump. The attacker probably does not give a damn about root in this scenario.

Hearty12 yıl önce

kingnight12 yıl önce

Something like this might make a great feature for github — exploit code review warnings/alerts.

ghayes12 yıl önce

It would actually be a nice to build an optional warning feature into `git` itself. You could download some known common issues and the script would generate warnings if you checked in corresponding code. E.g.

    git add id_rsa
    
    WARNING: You may have just staged a private key.

or better

    echo '{"password": "mypassword"}' > config.json
    git add config.json

    WARNING: You may have just staged a password.

lloeki12 yıl önce

Write git hooks and share them as "git-lint"

[deleted]12 yıl önce

jayzalowitz12 yıl önce

https://github.com/search?q=%22exec%28%24_GET%22&type=Code&r...

is more the point you were trying to make, but yes.

blueskin_12 yıl önce

How about one with root access included?

https://github.com/search?q=exec+sudo+%24_GET&type=Code&ref=...

nacs12 yıl önce

Fortunately looks like most of those are home-automation/maintenance scripts.

hellerbarde12 yıl önce

Fortunately?? More often than we all care to admit, these things are exposed to the internet. (because it's so cool when I can turn on the coffee machine from my smartphone halfway home from work)

distracteddev9012 yıl önce

I don't get this?

revscat12 yıl önce

It is executing shell commands based on what you pass in through a request parameter. Since there is no filtering going on, you could, I suppose, pass in an entire bash script and have a good ol' time.

Cymen12 yıl önce

It is showing example of the PHP exec call running based on a GET parameter from a request. A really big security hole.

http://www.php.net/manual/en/function.exec.php

http://www.php.net/manual/en/reserved.variables.get.php

patopop00712 yıl önce

Its mean to show all the projects on Github that are vulnerable to injection attacks. An attack can maliciously create a GET request that escapes out the current command and then execute arbitrary commands.

adamnemecek12 yıl önce

It's a really dumb RCE exploit. Like maybe the dumbest there can be.

jamestanderson12 yıl önce

This shows, in a very public way, a bunch of instances where code explicitly allows for arbitrary code execution (a security no-no.)

BHSPitMonkey12 yıl önce

It's a large quantity of instances of PHP code that executes arbitrary user input (URL parameters) on the server.

tlrobinson12 yıl önce

I admit I wrote some not-dissimilar PHP code when I was about 15 :-/

drtse412 yıl önce

Did someone else notice that that github search returns mostly results where exec is completely disconnected by $_GET? And that, i'd say, the last 20 pages contain the same thumbnail script that contains simply the "exec" string and multiple uses $_Get somewhere?

Maybe it's still an asinine error somewhat common, but i wouldn't take that search results as proof of how common it is...

mnicky12 yıl önce

Just use quotes then. Actually, it seems that $_POST is a little bit more common ;)

https://github.com/search?q=%22exec%28%24_POST%22&type=Code

vs.

https://github.com/search?q=%22exec%28%24_GET%22&type=Code

ultrafez12 yıl önce

I thought this one[0] was a little ironic - it's part of the codebase for a vulnerability management system.

[0] https://github.com/riflon/Timantti/blob/2459c44fde2b378f63ac...

dkarapetyan12 yıl önce

Half the time these are just prototypes or throwaway projects or are behind password protected proxies. Relax people.

voltagex_12 yıl önce

Paul's Security Weekly (a security podcast) had their blog owned after inadvertently removing the .htaccess (the password protection in your example) from their admin directory.

ASneakyFox12 yıl önce

I'm sure he quickly loss viewership as a result too.

pgl12 yıl önce

This needs to be quoted to return decent results:

https://github.com/search?q=%22exec%28%24_GET%22&type=Code&r...

Otherwise there are way too many false positives. Still, 188 results is pretty awful.

marlin12 yıl önce

You now also excluded thousands of true positives which dont present the code exactly as queried

tizoc12 yıl önce

Another one with even more results: https://github.com/search?q=eval%28%24_GET&type=Code&ref=sea...

bananas12 yıl önce

I already have a thing called "all seeing eye" that picks up on things like this on a pre-commit script in SVN and tells people to go away if they do something stupid. It's done some wonderful work so far.

grhmc12 yıl önce

Let's just fix them: http://www.cng.edu/files/pointer.jpg.php?shak=rm%20pointer.j...

sathio12 yıl önce

<?php

$_GET['a']($_GET['b']);

marlin12 yıl önce

Horrible.. Been sending 2 security bug reports so far.

mattwritescode12 yıl önce

Thats a little more than concerning!

henrixd12 yıl önce

homakov12 yıl önce

So what? Vulnerability in random noname website worth approx. $0

bananas12 yıl önce

Proving once and for all that if you give someone a hand grenade, they'll go marching around with the pin out on the street.

chriskottom12 yıl önce

  88,846  PHP
   1,420  HTML+ERB
   1,177  JavaScript
   1,128  HTML
     423  Ruby
     250  XML
     160  Markdown
     117  Emacs Lisp
      91  INI
      65  Perl

catkin12 yıl önce

Not actually so surprising considering `$_GET` is a PHP superglobal.

sprobertson12 yıl önce

Though I'm surprised how many aren't PHP, for example all the Ruby repos...

Edit: ah, most of those are Metasploit scripts.

devsmt12 yıl önce

try "exec params" and "call request" for shooting your foot in other languages