3
\$\begingroup\$
<?php
    $userinput = $_GET['host'];
    $e = escapeshellcmd($userinput);
    $arr = (explode(".",$e));
    $num = count($arr);
    $times = (int)$_GET['times'];
    $time = (range(1,51));
    if (!isset($time[$times])){
        $times = 5;
    }
    function isValidURL($url){
        return preg_match('/(www\.)?(.)*[\.](.)*$/i', $url);
    }
    if($num == "2"){
        if(isValidURL($e)){
            echo"<pre style=\"background:black; color:white;\">", passthru("ping -n $times $e"),"</pre>";}
        else {
            echo "The URL or IP $e isn't valid <br />";
    }
?>

This code is pinging the user requested host. Is this code insecure because I am using the passthru() function?

I am using explode to know if it's domain.com or just text and a regular expression to validate its not texttext.text. Is there way for the user to access my terminal with this?

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

Your regular expression for detecting valid urls is very weak and you can slip a lot past it.

Though you have used escapeshellcmd() to protect yourself a bit from people trying to run alternative commands you have not protected yourself from people sending extra parameters to ping.

Two alternative attacks spring to mind:

  • You can overload your servers by making them send lots of useless packets in the ping requests.
  • You can use your servers as the host for an attack on somebody else.

An example of sending extra bytes with ping (I put your script in the file test.php on myhost (not real name).

http://myhost.com/test.php?host=plop.com%20-s%20500

Generates:

ping -n 0 plop.com -s 500
\$\endgroup\$
1
  • \$\begingroup\$ well this code isn't effective online anywhere yet , i wanna see all attack / breaches that i have in mind before ill upload it \$\endgroup\$
    – Eli Y
    Sep 24, 2011 at 10:49
2
\$\begingroup\$

I would do $times = min( (int)$_GET['times'], 999); where 999 is the maximum number of times.

\$\endgroup\$
5
  • \$\begingroup\$ that's an idea , but not my point , my point is what breaches are there so i will make a script to avoid that. \$\endgroup\$
    – Eli Y
    Sep 23, 2011 at 22:50
  • \$\begingroup\$ That's part of it. Someone might know how to kill ping by sending excessively huge numbers. \$\endgroup\$
    – Christian
    Sep 23, 2011 at 23:02
  • 1
    \$\begingroup\$ Also, you should validate the URL before escaping it... \$\endgroup\$
    – Christian
    Sep 23, 2011 at 23:04
  • \$\begingroup\$ @Christian could you clarify why you'd do what you suggest in your answer? I'm not a PHP expert but just curious. (At any rate you should consider adding these clarifications into your answer.) \$\endgroup\$ Sep 24, 2011 at 2:55
  • \$\begingroup\$ @Joseph Weissman well what i said is to make the limiter of the times for ping with count() instead of array \$\endgroup\$
    – Eli Y
    Sep 24, 2011 at 9:31
2
\$\begingroup\$

Should escapeshellarg or escapeshellcmd be used for the host? I've read both of the man pages just now, and they offer conflicting advice. I'm about 99% sure that escapeshellarg should be used with the host though:

$host = shellescapearg("google.com");
exec("ping {$host}");

As Christian Sciberras began saying in his comment:

I would validate the url before escaping it instead of afterwards. I can't imagine that an escaped one would ever pass when a non-escaped one would not; however, when you escape it and then check it, you're not really validating what was inputted, but rather you're validating a processed version of it.

Imagine this scenario: You have user profiles, and they allow users to enter a description of themselves. Now, these may have some HTML entities in them, such as & or <. Now, when you store this, you're going to store the original version. When you display it back to the browser, you will pass it through something like htmlentities(). My point is, when you let the user update their profile, you're going to validate the original version with & instead of &amp;.

Christian's (and my) feelings on the host validation are along those same lines. When you validate user input, you want to validate the actual input, not the processed version.

But also to add something:

You should never assume that array keys exist in the $_GET/$_POST/$_COOKIES, so on arrays (any user-dependent array).

For example:

$userinput = $_GET['host'];

If the user just goes to mypage.php without the host param, then PHP will throw a notice when your script tries to access a non-existent array key.

Another case where users can trigger errors messages is something like: mypage.php?host[]=blah

That means that $_GET['host'] is array('blah'). So, when you try to pass that into escapeshellcmd or explode, it will throw an error about expecting a string and getting an array.

What I typically do is something like:

if(isset($_GET['blah']) && is_string($_GET['blah'])) {
    $blah = $_GET['blah'];
} else {
    $blah = null;
}

Or, the shorter version I usually use:

$blah = (isset($_GET['blah']) && is_string($_GET['blah'])) ? $_GET['blah'] : null;

Note though that isset() is not the same as array_key_exists, which sometimes makes more sense. Also, you can always use is_string because input arrays will always contain string values, even if the user's input is numeric. (The exception is, if the user inputs an array, it does become an array.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.