Please review my mail script

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
ben04
Forum Newbie
Posts: 2
Joined: Tue Sep 23, 2008 3:01 pm

Please review my mail script

Post by ben04 »

Hello,

I've written a php script that can send mails. It's mainly for myself when I want to quickly send mails and don't have my mail client at hand. Security holes in this script are obviously an invitation to spammer therefore I would be glad if you could look over it and tell me if you spot something I missed.

The password is mymail. If the script goes life I'll change it to something more secure.

Code: Select all

<?php
 
// Configuration constants
define("PW_MD5", "061b876db2ac78e2473150f959cb7ae3");
define("THIS_FILE", $_SERVER['PHP_SELF']);
define("USER_EMAIL", "foo@bar.net");
define("BLIND_COPY_EMAIL", "blindcopy@bar.net");
define("USER_NAME", "Me");
 
sleep(1);
 
// Helper Functions
function is_valid_address($email){
    return
        // foo@bar.tdl
        preg_match("/^[^@]+\\@.{3,}\\.[a-zA-Z]{2,}$/", $email) == 1 ||
        // Name <foo@bar.tdl>
        preg_match("/^.+<[^@]+\\@.{3,}\\.[a-zA-Z]{2,}>$/", $email) == 1
    ;
}
 
function escape_quotes($str){
    return str_replace("\"","\\\"", $str);
}
 
function is_space_string($str){
    return preg_match("/^\\s*$/", $str);
}
 
function constains_newlines($str){
    return strpos($str, "\n") !== false;
}
 
// Test config values
if(!is_valid_address(USER_EMAIL))
    die("USER_EMAIL=".USER_EMAIL." is invalid");
 
// Default values
$pw = "";
$to = "";
$cc = array();
$from = USER_NAME." <".USER_EMAIL.">";
$reply_to = USER_NAME." <".USER_EMAIL.">";
$copy_to_sender = true;
 
$title = "";
$body = "";
 
$error_msg = array();
$notice_msg = array();
 
// Load POST input
$input_complete = false;
 
if(array_key_exists("send_email", $_POST)){
    $input_complete = true;
    
    if(array_key_exists("pw", $_POST))
        $pw = $_POST["pw"];
    else
        $input_complete = false;
    
    if(array_key_exists("to", $_POST))
        $to = $_POST["to"];
    else
        $input_complete = false;
        
    if(array_key_exists("cc", $_POST)){
        $cc = array();
        foreach(explode("\n", str_replace("\r\n", "\n", $_POST["cc"])) as $email)
            if(!is_space_string($email))
                $cc[] = $email;
    }else
        $input_complete = false;
        
    if(array_key_exists("from", $_POST))
        $from = $_POST["from"];
    else
        $input_complete = false;
    
    if(array_key_exists("reply_to", $_POST))
        $reply_to = $_POST["reply_to"];
    else
        $input_complete = false;
    
    if(array_key_exists("title", $_POST))
        $title = $_POST["title"];
    else
        $input_complete = false;
    
    if(array_key_exists("body", $_POST))
        $body = $_POST["body"];
    else
        $input_complete = false;
    
    if(array_key_exists("copy_to_sender", $_POST))
        $copy_to_sender = true;
    else{
        // A missing copy_to_sender can mean that the checkbox
        // is unchecked or that the information was never send by
        // the browser. :/
        //
        // Workaround: If all input was complete so far then it
        // was most likely not set.
        if($input_complete)
            $copy_to_sender = false;
    }
    
    if(!$input_complete)
        $error_msg[] = "Post data was incomplete (This error should not show in normal usage scenarios.)";
}
 
// Check input validity
 
$input_valid = false;
 
if($input_complete){
    $input_valid = true;
    
    if(md5($pw) != PW_MD5){
        $error_msg[] = "Wrong password";
        $input_valid = false;
    }
    
    function format_ith($i){
        if(10 <= $i && $i < 20)
            return $i."th";
        if(100 <= $i || $i <= 0)
            return $i."th";
            
        switch($i % 10){
            case 1:return $i."st";
            case 2:return $i."nd";
            case 3:return $i."rd";
            default:return $i."th";
        }
    }
    
    for($i = 0; $i < count($cc); ++$i)
        if(!is_valid_address($cc[$i]) || constains_newlines($cc[$i])){
            $input_valid = false;
            $error_msg[] = format_ith($i+1)." cc-email invalid";
        }
    
    if(!is_valid_address($to) || constains_newlines($to)){
        $input_valid = false;
        $error_msg[] = "to-email invalid";
    }
    
    if(!is_valid_address($from) || constains_newlines($from)){
        $input_valid = false;
        $error_msg[] = "from-email invalid";
    }
 
    if(!is_valid_address($reply_to) || constains_newlines($reply_to)){
        $input_valid = false;
        $error_msg[] = "reply-to-email invalid";
    }
    
    if(count($to) == 0){
        $input_valid = false;
        $error_msg[] = "no to-email";
    }
    
    if(is_space_string($title)){
        $input_valid = false;
        $error_msg[] = "no title";
    }else if(constains_newlines($title)){
        $input_valid = false;
        $error_msg[] = "title mustn't contain newlines";
    }
    
    if(is_space_string($body)){
        $input_valid = false;
        $error_msg[] = "no body";
    }
    
    // Make sure all newlines are of the style \r\n
    $body = str_replace("\r\n", "\n", $body);
    $body = str_replace("\n", "\r\n", $body);
}
 
// Send Mail
 
if($input_complete && $input_valid){
    
    if($copy_to_sender){
        $subject = "This mail was sent to: $to \r\n";
        if(count($cc)){
            $subject .= "\r\n".
                "This mail was also cc-ed to:\r\n".
                "\r\n";
            foreach($cc as $email)
                $subject .= '- '. $email . "\r\n";
        }
        
        $subject .= "\r\n".
            "The content of the original mail was:\r\n".
            "------------------------------------ \r\n".
            $body;
        
        $header = 'From: ' . BLIND_COPY_EMAIL . "\r\n";
        
        if(@mail(USER_EMAIL, 'blind copy:'.$title, $subject, $header))
            $notice_msg[] = 'blind copy sent to '.USER_EMAIL;
        else
            $error_msg[] = 'could not send blind copy to '.USER_EMAIL;
    }
 
    $header = 'From: ' . $from . "\r\n";
    if($from != $reply_to)
        $header .= 'Reply-to: ' . $reply_to . "\r\n";
    
    foreach($cc as $email)
        $header .= 'Cc: '. $email . "\r\n";
    
    if(@mail($to, $title, $body, $header))
        $notice_msg[] = 'e-mail was sent';
    else
        $error_msg[] = 'could not send e-mail';
}
 
// Send form
 
?>
<html>
    <head>
        <title>Send E-Mails from <?php echo USER_EMAIL ?></title>
    </head>
    <body>
<?php
    if(count($error_msg) != 0){
        if(count($error_msg) == 1)
            echo "\t\t<h1>Error occured</h1>\n";
        else
            echo "\t\t<h1>Errors occured</h1>\n";
        
        echo "\t\t<ul>\n";
        
        foreach($error_msg as $msg)
            echo "\t\t\t<li>".$msg."</li>\n";
            
        echo "\t\t</ul>\n";
    }
    
    if(count($notice_msg) != 0){
        if(count($notice_msg) == 1)
            echo "\t\t<h1>Notice was raised</h1>\n";
        else
            echo "\t\t<h1>Notices were raised</h1>\n";
        
        echo "\t\t<ul>\n";
        
        foreach($notice_msg as $msg)
            echo "\t\t\t<li>".$msg."</li>\n";
            
        echo "\t\t</ul>\n";
    }
    
?>      <form action="<?php echo THIS_FILE ?>" method="post">
            <p>
                <input type="submit" value="Send E-Mail" />
                <input type="hidden" name="send_email" value="do"/>
                <a href="<?php echo THIS_FILE?>">New mail</a>
            </p>
            <table border="0">
                <tr>
                    <td>
                        Password:
                    </td>
                    <td>
                        <input style="font-family:monospace" size="70" name="pw" type="password"/>
                    </td>
                </tr>
                <tr>
                    <td>
                        To:
                    </td>
                    <td>
                        <input style="font-family:monospace" size="70" name="to" type="text" value="<?php echo escape_quotes($to)?>"/>
                    </td>
                </tr>
                <tr>
                    <td>
                        CC:
                    </td>
                    <td>
                        <textarea style="font-family:monospace" cols="70" name="cc" rows="5"><?php echo htmlspecialchars(implode("\n", $cc))?></textarea>
                    </td>
                </tr>
                <tr>
                    <td>
                        From:
                    </td>
                    <td>
                        <input style="font-family:monospace" size="70" name="from" type="text" value="<?php echo escape_quotes($from)?>"/>
                    </td>
                </tr>
                <tr>
                    <td>
                        Reply-To:
                    </td>
                    <td>
                        <input style="font-family:monospace"size="70" name="reply_to" type="text" value="<?php echo escape_quotes($reply_to)?>"/>
                    </td>
                </tr>
                <tr>
                    <td>
                        <input type="checkbox" name="copy_to_sender" value="on" <?php if($copy_to_sender)echo "checked"?>>
                    </td>
                    <td>
                        Send blind copy to <?php echo USER_EMAIL?>
                    </td>
                </tr>
                <tr>
                    <td>
                        Title:
                    </td>
                    <td>
                        <input style="font-family:monospace"size="70" name="title" type="text"  value="<?php echo escape_quotes($title)?>"/>
                    </td>
                </tr>
            </table>
            <p>
                <textarea style="font-family:monospace" name="body" cols="100" rows="50"><?php echo htmlspecialchars($body)?></textarea>
            </p>
            <p>
                <input type="submit" value="Send E-Mail" />
                <a href="<?php echo THIS_FILE?>">New mail</a>
            </p>
            
        </form>
    </body>
</html>
 
marcth
Forum Contributor
Posts: 142
Joined: Mon Aug 25, 2008 8:16 am

Re: Please review my mail script

Post by marcth »

You're code is good and quite readable, so bear that in mind and read my comments with a grain of salt.

Code: Select all

 
// Configuration constants
define("PW_MD5", "061b876db2ac78e2473150f959cb7ae3");
define("THIS_FILE", $_SERVER['PHP_SELF']);
define("USER_EMAIL", "foo@bar.net");
define("BLIND_COPY_EMAIL", "blindcopy@bar.net");
define("USER_NAME", "Me");
 
I would split off your constants and add them to a constants.php file--one stop shopping for all application constants.

Why do you need to define the THIS_FILE as a constant? Why not just use $_SERVER['PHP_SELF'] directly? What's the advantage?

Having built applications with hundreds of constants, I've gotten into the habit of prefixing the constants with the type, for instance, EMAIL_USERNAME, EMAIL_BLIND_COPY, EMAIL_USER, etc.

I wouldn't use a constant for any data that is dynamic or changes from server to server.

Code: Select all

 
sleep(1);
 
Why pause your script for a second?

Code: Select all

 
 // Helper Functions
 function is_valid_address($email){
     return
         // foo@bar.tdl
         preg_match("/^[^@]+\\@.{3,}\\.[a-zA-Z]{2,}$/", $email) == 1 ||
         // Name <foo@bar.tdl>
         preg_match("/^.+<[^@]+\\@.{3,}\\.[a-zA-Z]{2,}>$/", $email) == 1
     ;
 }
  
 function escape_quotes($str){
     return str_replace("\"","\\\"", $str);
 }
  
 function is_space_string($str){
     return preg_match("/^\\s*$/", $str);
 }
  
 function constains_newlines($str){
     return strpos($str, "\n") !== false;
 }
 
These seems like generic validation functions that don't necessarily have anything to do with your email script. Why not put them in their own file/class so any other code you write can use them, if needed.

I'd rename is_valid_address to isEmail() or is_email() and make that code more readable (it is efficient though). I'd rename is_space_string and constains_newlines to hasSpace and hasNewLine--Whatever syntax/symantics you want to use, so long as it's consistent.

I think there may be a better way to get the functionality of the escape_quotes() function--does it have something to do with PHP magic quotes or html_entities?


As for the rest, I'd encapsulate your mail functionality into a function or class to separate it from your implementation. An application just needs one emailer...not one per email form.
ben04
Forum Newbie
Posts: 2
Joined: Tue Sep 23, 2008 3:01 pm

Re: Please review my mail script

Post by ben04 »

I would split off your constants and add them to a constants.php file--one stop shopping for all application constants.
I forgot to say, that I want the script to be self contained. Meaning just drop the script anywhere on a php server and it should just work fine.
Why pause your script for a second?
Now that I think of it, it is pretty useless. My original attempt was to make brute forcing the password impossible. If you need at least one second to test a password then you'll need a very long time even for small and weak passwords. It doesn't work because the attacker can load the page several times at each moment. I would need to limit the execution of the script to once per second. Is there some easy way to find out when the script was last executed?
I think there may be a better way to get the functionality of the escape_quotes() function--does it have something to do with PHP magic quotes or html_entities?
There is addslashes but it also escapes ' which I don't need. However I also need to escape regular \ so there is a bug in my function. My function now looks like this:

Code: Select all

function escape_quotes($str){
    if(!get_magic_quotes_gpc())
        return str_replace(array("\"", "\\"), array("\\\"", "\\\\"), $str);
    else
        return str_replace("\\\'", "\'", $str);
}
Post Reply