Why PHP Coders Have a Bad Reputation



  • Found while updating email addresses across a website.  This little piece of code handled a "contact us" form.  I'm surprised the client hadn't complained about it yet.  I'm even more suprised they haven't received any related spam complaints:

    function contact_us(){
    $name = $_REQUEST['name'];
    $email = $_REQUEST['email'];
    $body = $_REQUEST['body'];

    $to      = 'correctemail@correctdomain.com'
    $subject = 'Contact Form';
    $message = 'Contact Form' . "\n\n";

    $message =  "Name: $name\n";
    $message .= "Email: $email\n";
    $message .= "Message: \n";
    $message .= stripslashes($body) . "\n\n";

    $headers = 'From: ' . $webmaster_email . "\n";
       'Reply-To: ' . $webmaster_email . "\n";

     if (mail($to, $subject, $message, $headers)){
       $out = '<p>Thank you for contacting WTF, Inc.</p>';
     }else{
       $out = '<p>There was a problem submitting the form.</p>';
     }

     return $out;
    }



  • Sorry, I don't see the WTF there. It's very readable, easily understandable bit of code. It's also PHP, which means that spam-bots can't find the email address, if that's what you're getting at.

     



  • Looks fine to me - but then PHP is not one of my great strengths!



  • Mind pointing out what's wrong with it? Looks fairly standard.



  • [quote user="overmyhead"]

    $message = 'Contact Form' . "\n\n";

    $message =  "Name: $name\n";

    [/quote]

     I don't really know PHP and I don't see the big wtf, but I think the "contact form \n\n" part of the message gets obliterated by the next line.  



  • [quote user="overmyhead"]

    $to      = 'correctemail@correctdomain.com'

    [/quote]

    Maybe this isn't an edit and what the code actually had? instead of "$correctemail"... 



  • $name = $_REQUEST['name'];
    $email = $_REQUEST['email'];
    $body = $_REQUEST['body'];
    $message = 'Contact Form' . "\n\n";
    .
    .
    .
    $message =  "Name: $name\n";
    $message .= "Email: $email\n";
    $message .= "Message: \n";
    $message .= stripslashes($body) . "\n\n";
    .
    .
    .
    if(mail($to, $subject, $message, $headers) ) {

    PHP mail() function won't check your data for you.  I'm pretty sure that this is vulnerable to header injection -- and thereby to being used to spam with fake BCCs.

    $headers = 'From: ' . $webmaster_email . "\n";
       'Reply-To: ' . $webmaster_email . "\n";

    1) $webmaster_email hasn't been declared in this function, and in PHP, globals have to be explicitly declared as global in each function where they're used, or they're null.  So the "from" address will be empty.

    2) The second line is a statement without an assignment, so reply-to is never used (not that it matters, since it's empty).

    This is a fairly representative snippet from a large system that usually does not-quite what it's supposed to (assuming you don't introduce any potential error conditions)... for reasons that are glaringly obvious once you track them down, but (as you can see) can occasionally be difficult to pinpoint.



  • Maybe it's just because I didn't sleep at all last night, but I don't really see anything WTF-worthy in this. This:

    $message =  "Name: $name\n";
    should be an append, not an assignment, and the use of stripslashes implies that magic quotes are turned on (which is a Bad Thing), and the code style is a bit inconsistent, but, again, no WTFs in sight.

    Edit: never used PHP's mail function, but if that's true, then it makes a bit more sense. 



  • [quote user="goldsz"][quote user="overmyhead"]

    $message = 'Contact Form' . "\n\n";

    $message =  "Name: $name\n";

    [/quote]

     I don't really know PHP and I don't see the big wtf, but I think the "contact form \n\n" part of the message gets obliterated by the next line.  

    [/quote]

    Yup, it does.  Nice catch - I missed that one.



  • the real WTF is that most php coders won't catch the header injection vulnerability.

    Not too long ago I looked for a free contact form script on hotscripts (it's often easier to take stuff like this for throwaway websites than it is to spend 10 min and write one)... I got through 3 or 4 before I made my own, as they were all vulnerable to header injections.



  • [quote user="overmyhead"]

    function contact_us(){
    $name = $_REQUEST['name'];
    $email = $_REQUEST['email'];
    $body = $_REQUEST['body'];


    [/quote]

     

    This is beautiful code </sarcasm>. I love how they use the $_REQUEST check instead of the $_GET or $_POST. Now if this is a public website we could have some real fun telling people about this great new stock program, maybe spam everyone with the new short POS story or make men feel as though they are really not man enough! 



  • [quote user="overmyhead"]

    $name = $_REQUEST['name'];
    $email = $_REQUEST['email'];
    $body = $_REQUEST['body'];
    $message = 'Contact Form' . "\n\n";
    .
    .
    .
    $message =  "Name: $name\n";
    $message .= "Email: $email\n";
    $message .= "Message: \n";
    $message .= stripslashes($body) . "\n\n";
    .
    .
    .
    if(mail($to, $subject, $message, $headers) ) {

    PHP mail() function won't check your data for you.  I'm pretty sure that this is vulnerable to header injection -- and thereby to being used to spam with fake BCCs.

    $headers = 'From: ' . $webmaster_email . "\n";
       'Reply-To: ' . $webmaster_email . "\n";

    1) $webmaster_email hasn't been declared in this function, and in PHP, globals have to be explicitly declared as global in each function where they're used, or they're null.  So the "from" address will be empty.

    2) The second line is a statement without an assignment, so reply-to is never used (not that it matters, since it's empty).

    This is a fairly representative snippet from a large system that usually does not-quite what it's supposed to (assuming you don't introduce any potential error conditions)... for reasons that are glaringly obvious once you track them down, but (as you can see) can occasionally be difficult to pinpoint.

    [/quote]

    Ah, that makes a little more sense now. PHP is such a WTF language in and of itself that it's hard for me to see any special WTFs in any PHP code unless it's really obvious.
     



  • Unfortunatlely it's one of those areas where the builtin functions in PHP are a WTF in and of themselves.

    If you pass completely unsanitised request variables into the mail function, nefarious spammers can inject their own headers, making it send to any email they wish.

     
    They really need to sort the mail() function out IMHO.  Until then I always use the very excellent phpMailer (http://phpmailer.sourceforge.net) which neatly avoids this problem by talking native smtp to your local relay.
     



  • @overmyhead said:

    $headers = 'From: ' . $webmaster_email . "\n";
       'Reply-To: ' . $webmaster_email . "\n";

    those "\n"s really really want to be "\r\n"'s. Other than that, it seems OK, but then PHP is not my native language...

    And where the f... is the preview post function when you need it?

    EDIT: Oh, there.



  • [quote user="overmyhead"]

    PHP mail() function won't check your data for you.  I'm pretty sure that this is vulnerable to header injection -- and thereby to being used to spam with fake BCCs.

    [/quote]

    The real WTF would be then, that they say nothing about it at http://www.php.net/manual/en/function.mail.php .

    Of course you should check what you're passing to the "additional headers" parameter.

    But I wasn't aware that you should also check the "To" and "Subject", too.

    Probably I should've excpected that after reading :

    <var class="parameter">subject</var>

    Subject of the email to be sent.

    This must not contain any newline characters, or the mail may not be sent properly.



  • I'm still not seeing the problem. The ONLY parameter that comes from user-specified data is $message. The other parameters - to-address, subject, and extra-headers - are all built from variables which are either shown as being defined as constants in the code, or variables which don't exist, neither of which are vulnerable to injection (... unless register_globals is on, in which case the administrator who arranged for this setting should be shot).

    [quote user="philjohn"]

    Unfortunatlely it's one of those areas where the builtin functions in PHP are a WTF in and of themselves.

    If you pass completely unsanitised request variables into the mail function, nefarious spammers can inject their own headers, making it send to any email they wish.

     
    They really need to sort the mail() function out IMHO.  Until then I always use the very excellent phpMailer (http://phpmailer.sourceforge.net) which neatly avoids this problem by talking native smtp to your local relay.
     

    [/quote]


  • So, basically what you're saying is that the WFT boils down to:

    a) A few typos and omissions that looked like anonymisation errors

    b) Using PHP's built-in mail() function which is (apparently) open to serious abuse.

    Note: A quick Google shows that recent versions of PHP are not vulnerable to header injections anyway!



  • [quote user="Hawk777"]I'm still not seeing the problem. The ONLY parameter that comes from user-specified data is $message. The other parameters - to-address, subject, and extra-headers - are all built from variables which are either shown as being defined as constants in the code, or variables which don't exist, neither of which are vulnerable to injection (... unless register_globals is on, in which case the administrator who arranged for this setting should be shot).[/quote]

    If the first line of the "message" is "Bcc: <insert one thousand email addresses here>", then this message will be blindly CC'ed to each of those people through this server.

     I think SMTP is partly to blame here. It's such an old system and it really wasn't designed to be "safe"...its designed to be a simple system that  is easy to use when everybody plays fair.



  • You don't actually know how BCC (which is mostly MUA-side (!), not MTA-side stuff) works, do you? Or, more generally, you don't know that message-headers are generally completely and utterly unimportant to the actual (successful) mail transport process, the important thing being the envelope sender and recipient(s), which come from static variables in the code you posted, even when using the sendmail binary under UNIX?

    Please, please, please try your injection idea, and if it doesn't work, reconsider whether the form processing method is actually broken; it isn't, and looks perfectly fine to me, except that it might allow fucked up content for the people having to read the replies, which definititely is not a problem of "spamtastic" proportions.

    (I'll put the blame for the other "simple" mistakes pointed out by others, such as overwriting variables, on your transliteration, not on the original coder)



  • damn straight modelnine... enjoy this thread over a cold beer, smiling to oneself at people blaming various unknowns for behaviour they haven't checked exists, not fully researching facts they mention and half reading some code that's clearly been pasted from an example without the author fully understanding it... welcome to PHP, please drive carefully.



  • [quote user="mrsticks1982"]

    This is beautiful code </sarcasm>. I love how they use the $_REQUEST check instead of the $_GET or $_POST.

    [/quote]

    You successfully linked to the documentation on $_REQUEST without reading any of the 3 tiny paragraphs that indicate that it is nothing more than a shortcut for $_GET and $_POST.  Congrats.


    And to others: superglobals only allow the request data to be defined as individual variables within the global scope of the document, not functions.

    If the register_globals directive is set, then these variables will also be made available in the global scope of the script; i.e., separate from the $_REQUEST array. These individual globals are not autoglobals.


    But I'd never use them anyway, of course.



  • This isn't an obvious WTF, I'm afraid. It's just standard PHP "security sure ain't first" attitude.

    The solution is pretty easy - run a regular expression over all your inputs to make sure nothing funny looking is coming in or use a library that has done it for you.

    More details at Secure PHP Wiki



  • [quote user="Pap"]And to others: superglobals only allow the request data to be defined as individual variables within the global scope of the document, not functions.[/quote]

     

    Maybe I'm misreading that, but superglobals ($_POST, $_GET, etc.) are always visible within every function, hence the name superglobals.

     

    My first thought was that what was posted was pretty much the entire script, so The Real WTF (tm) was that some spammer could take advantage of it with a script that keeps hitting http://www.example.com/mail.php?name=foo&email=foo@example.com&message=spam



  • I meant to say "the register globals" instead of superglobals.  my bad.



  • I don't see the WTF either; the mail will always be sent to correctemail@correctdomain.com, and no user data is being put in the $headers so there should be no header vulnerability. There's a few minor bugs (already spotted) but no WTF.



  • [quote user="dasluq"][quote user="overmyhead"]$headers = 'From: ' . $webmaster_email . "\n";
       'Reply-To: ' . $webmaster_email . "\n";[/quote]

    those "\n"s really really want to be "\r\n"'s. Other than that, it seems OK, but then PHP is not my native language...

    [/quote]

    Not if its a Unix box. Using Windows line-endings on Unix sendmail will result in an unintentionally double-spaced e-mail body. 



  • [quote user="savar"][quote user="dasluq"][quote user="overmyhead"]$headers = 'From: ' . $webmaster_email . "\n";
       'Reply-To: ' . $webmaster_email . "\n";[/quote]

    those "\n"s really really want to be "\r\n"'s. Other than that, it seems OK, but then PHP is not my native language...

    [/quote]

    Not if its a Unix box. Using Windows line-endings on Unix sendmail will result in an unintentionally double-spaced e-mail body. 

    [/quote]

     if the PHP mail() function pipes into sendmail directly, that's another WTF (though a small one, since a lot of things do that, stilll stupid).  If it's doing it with -t, that's a bigger WTF.  If sendmail itself can't process mail correctly in network standard form, even if that's not its preferred form on a platform,  that's really a WTF.
     



  • [quote user="Dragnslcr"]

    [quote user="Pap"]And to others: superglobals only allow the request data to be defined as individual variables within the global scope of the document, not functions.[/quote]

     

    Maybe I'm misreading that, but superglobals ($_POST, $_GET, etc.) are always visible within every function, hence the name superglobals.

     

    My first thought was that what was posted was pretty much the entire script, so The Real WTF (tm) was that some spammer could take advantage of it with a script that keeps hitting http://www.example.com/mail.php?name=foo&email=foo@example.com&message=spam

    [/quote]

     

    That is where I was thinking as well. What would stop someone from doing the above and pressing enter. I linked to the @_REQUST information because it contains the info from any post or get method as well as a cookie. A simple program could submit information automatically. Although I like the BCC injection better!



  • You'll notice that the $email var is only used in the body of the email.  The destination address is hardcoded in.  This is no different than your standard "Contact Us" form.



  • Agreed.  This is entirely standard for a Contact Us form.  I wouldn't even suggest that this is evidence of a poor PHP coder.  Indeed, PHP doesn't do a lot of hand holding, especially when it comes to security.  But then again, neither does C.  mail() is certainly no more intelligent than gets().  Should we then say that there are a lot of bad C programmers, or that C is a bad language?  As in most languages, it's up to the programmer to cover their ass; unfortunately, that usually requires knowing what's coming at you.

    The only issue here is that the programmer doesn't know about injection attacks and how to prevent them.  Then again, neither did I until I stumbled across news articles about SQL injection and cross-site scripting.  Looks like there are a lot of people out there who didn't know about this.

    The life lesson--never trust user input--is one that I haven't seen generally taught in universities or technical schools.  It's one of those things passed down from the elders, or learned the hard way when it bites you in the ass.

    Please kindly inform the author of this code of his mistake.  Also, please consider how much of a WTF something like this may be to others.  Just because something is obvious to you (because you've been informed of it) doesn't mean it is obvious to others (especially those who were never told about it).
     


Log in to reply