1
\$\begingroup\$

I have this PHP curl function to check if a site is online or not:

function check($proc,$url,$keyword,$method,$timeout,$maxredirs) {
    // Format Link
    $link = $proc."://".$url;
    // Agent
    $user_agent='Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.130 Safari/537.36';

    $options = array(
        CURLOPT_CUSTOMREQUEST  => $method,     //set request type post or get
        CURLOPT_POST           =>false,        //set to GET
        CURLOPT_USERAGENT      => $user_agent, //set user agent
        CURLOPT_RETURNTRANSFER => true,     // return web page
        CURLOPT_HEADER         => false,    // don't return headers
        CURLOPT_FOLLOWLOCATION => true,     // follow redirects
        CURLOPT_ENCODING       => "",       // handle all encodings
        CURLOPT_AUTOREFERER    => true,     // set referer on redirect
        CURLOPT_CONNECTTIMEOUT => $timeout,      // timeout on connect
        CURLOPT_TIMEOUT        => $timeout,      // timeout on response
        CURLOPT_MAXREDIRS      => $maxredirs,       // stop after 10 redirects
        CURLOPT_REFERER        => "http://example.com",
        CURLOPT_SSL_VERIFYPEER => false,
    );
    $ch      = curl_init( $link );
    curl_setopt_array( $ch, $options );
    $content = curl_exec( $ch );
    $err     = curl_errno( $ch );
    $errmsg  = curl_error( $ch );
    $header  = curl_getinfo( $ch );
    curl_close( $ch );
    $header['errno']   = $err;
    $header['errmsg']  = $errmsg;
    $header['content'] = $content;

    // Time (ms)
    $totaltime = $header['total_time']*1000;
    $totaltime = number_format($totaltime, 0, "", "");
    // Handle keyword search
    $izkey = 1;
    if(!empty($keyword)) {
        if (strpos($header['content'],$keyword) !== false) {
            $izkey = 1;
        }
        else {$izkey = 0;}
    }

    // Return
    return $header['http_code']."|".$totaltime."|".$izkey;
}

The function variables are as follow:

  • $proc: not user input, will either be http or https

  • $url: user input and will be a full link except the http:// or https:// part at the beginning

  • $keyword: user input, just alphanumeric characters

  • $method: not user input, will be GET

  • $timeout: user input, only digits, no more than 60

  • $maxredirs: user input, only digits, no more than 60

My question is: with the above mentioned user input and returning just 3 variables (see last line of code) is this function safe for the server to run like it is now? Could a user pass a malicious link to harm the server?

I know it may be a silly question, but, I want to be sure. It's better safe than sorry, and it can't hurt to ask.

\$\endgroup\$
  • \$\begingroup\$ Cross-posted on Stack Overflow \$\endgroup\$ – Mast Aug 28 '15 at 10:30
  • \$\begingroup\$ It seems to be resistent to Slowloris. \$\endgroup\$ – Ismael Miguel Aug 28 '15 at 10:43
  • \$\begingroup\$ @IsmaelMiguel That could be the basis for an answer if you extend it. \$\endgroup\$ – Mast Aug 28 '15 at 10:52
2
\$\begingroup\$

A user will probably not be able to hurt your server. You do open your system up to SSRF which might cause a problem, depending on your system. There might also be a possibility to DOS your server as you keep connections open for 60 seconds, which is a rather long time (I'm not quite sure about this, but you should definitely test this).

You also provide your users the possibility to attack other systems using your server. They may exploit vulnerabilities in other systems that can be exploited via GET, and they may use your server to scan websites because you allow a keyword search. It may also aid in DOS attacks. You may or may not be liable for damages causes by this, I personally wouldn't risk it.

At a minimum, I would disable the keyword search, limit the amount of requests a user can do, and change the user agent to something more honest (eg MyAvailabilityChecker (more info: http://mywebsite.com/availabilitychecker), and provide an option to opt-out).

Regarding your code: It's generally good. A couple of points:

  • your comments don't add anything (except the comment about time in ms). And they are partly wrong (eg stop after 10 redirects). I would just remove them.
  • why proc? Shouldn't it be prot? Either way, don't shorten variable names, it makes code harder to read. Same goes for maxredirs. Also, use snake_case for all variables, so it might be max_redirects (max is commonly used, so shortening is ok).
  • else {$izkey = 0;} is not properly formatted.
\$\endgroup\$
  • \$\begingroup\$ Thank you so much @tim for the great feedback. SSRF shouldn't be an issue. Any uptime monitoring service (such as pingdom, statuscake) can be used to access any website on the Internet over and over, so my function shouldn't be any different than that, should it? I will surely change the agent as you have suggested. Thanks for the comments and variables feedback, all though I don't understand your last remark 'else {$izkey = 0;} is not properly formatted.'. \$\endgroup\$ – Roberto Aug 28 '15 at 19:16
  • \$\begingroup\$ @Roberto I just meant that it's customary to put the else statement on its own line. And regarding the scanning: the main difference is that this is under your name so you are responsible^^ Pingdom and statuscace also seem to require payment and registration, and i'd assume that they also analyze what their users do and block malicious users. Free services might remove all query strings from request etc/not allow keyword search, or limit/analyse requests. \$\endgroup\$ – tim Aug 28 '15 at 19:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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