3
\$\begingroup\$

Is this script sufficient enough to validate user email input?

<?php 
//1 DATABASE CONNECTION
$dbHost = "HOST";
$dbUser = "USER";
$dbPassword = "PASSWORD";
$dbName = "DATABASE";

try {
  $dsn = "mysql:host=" . $dbHost . ";dbname=" . $dbName;
  $pdo = new PDO($dsn, $dbUser, $dbPassword);
  $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
  $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch(PDOException $e) {
  echo "DB Connection Failed: " . $e->getMessage();
  exit(0);
}
//1 END

//2 ADD EMAIL TO DATABASE

//set date and time
date_default_timezone_set('America/Los_Angeles');
$timestamp = strtotime('NOW');
$dateTime = date('Ymd-His', $timestamp);

//variable to store ipv4 address
$userIP4 = gethostbyname($_SERVER['REMOTE_ADDR']);
//storing ip6 could be something like: "bin2hex(inet_pton($_SERVER['REMOTE_ADDR']));" but I couldn't figure out if the output was correct, because it looked nothing like an ipv6 address.....

if(filter_var($userIP4, FILTER_VALIDATE_IP)) {
    //yes it's valid IPv4
    if($_SERVER['REQUEST_METHOD'] == 'POST') {
        $email = htmlspecialchars($_POST['email']); //convert special characters to HTML entities (&,",<,>)
        $Temail = trim($email); //trim spaces on ends
        
        //allow international characters
        if(preg_match("/^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$^/", $Temail)) {
            //prevents invalid email addresses
            header("Location: invalid.html");
            exit (0);
        } else {
            //Check Email Domain MX Record
            $email_host = strtolower(substr(strrchr($Temail, "@"), 1));
            if (!checkdnsrr($email_host, "MX")) {
                header("Location: invalid.html");
                exit (0);
            } else {
                //Prevent users from inputting a specific domain...
                $notallowed = [
                    'mydomain.com',
                ];
                $parts = explode('@', $Temail); //Separate string by @ characters (there should be only one)
                $domain = array_pop($parts); //Remove and return the last part, which should be the domain
                if ( ! in_array($domain, $notallowed)) {

                    //checks database to make sure the email is not a duplicate
                    $stmt1 = $pdo->prepare("SELECT * FROM emailTable WHERE email=?");
                    $stmt1->execute([$Temail]);
                    $user = $stmt1->fetch();
                    if($user) {
                        //prevents adding a duplicate email
                        header("Location: duplicate.html");
                        exit (0);
                    } else {
                        //generate Activation code
                        $Acode = md5(time().$Temail);
                        
                        //send verification email
                        $emailfrom = 'no-reply@mydomain.com';
                        $fromname = 'MY NAME';
                        $subject = 'Confirm Your Email Subscription';
                        $emailbody = "
                            <html>
                            <body style='background-color: #000; padding: 15px;'>
                                <table style='background-color: #222;'>
                                    <tr style='background-color: #333; padding: 15px; font-size: 1.3rem;'>
                                        <td><h2 style='color: #FFF;' align='center'>Please Verify Subscription</h2></td>
                                    </tr>
                                    <tr>
                                        <td style='color: #FFF; font-size: 1.1rem;' align='center'>
                                            <br/>
                                            <br/>
                                            If you didn't sign up for my email list, simply delete this message. You will not be added unless you push the button below.
                                            <br/>
                                            <br/>
                                        </td>
                                    </tr>
                                    <tr>
                                        <td style='color: #FFF; font-size: 1.3rem;' align='center'>
                                            <button style='background-color: #000; width: 6rem; height: 2rem;'><a href='https://www.MYDOMAIN.com/verify.php?acode=$Acode' style='color: #F00; text-decoration: none; font-size:1rem;'>VERIFY</a></button>
                                            <br/>
                                            <br/>
                                        </td>
                                    </tr>
                                    <tr>
                                        <td style='color: #FFF; font-size: 1.1rem;' align='center'>
                                            <font style='font-size:0.8rem;'>This email was automatically generated from a mailbox that is not monitored.</font>
                                        </td>
                                    </tr>
                                </table>
                            </body>
                            </html>";
                            
                        $headers = "Reply-To: MY NAME <no-reply@MYDOMAIN.com>\r\n"; 
                        $headers .= "Return-Path: MY NAME <no-reply@MYDOMAIN.com>\r\n"; 
                        $headers .= "From: MY NAME <no-reply@MYDOMAIN.com>\r\n";  
                        $headers .= "MIME-Version: 1.0\r\n";
                        $headers .= "Content-type: text/html; charset=UTF-8\r\n";
                        $headers .= "X-Priority: 3\r\n";
                        $headers .= "X-Mailer: PHP". phpversion() ."\r\n" ;
    
                        $params = '-f ' . $emailfrom;
                        $send = mail($Temail, $subject, $emailbody, $headers, $params); // $send should be TRUE if the mail function is called correctly
                        if($send) {
                            //add the new email and other data to the database
                            $sql = "INSERT INTO emailTable (IP4, datetime, email, acode) VALUES (:IP4, :datetime, :email, :acode)";
                            $stmt2 = $pdo->prepare($sql);
                            $stmt2->execute(['IP4' => $userIP4, 'datetime' => $dateTime, 'email' => $Temail, 'acode' => $Acode]);
                            $userIP4 = "";
                            $dateTime = "";
                            $Temail = "";
                            $Acode = "";
                            header("Location: success.html");
                            exit (0);
                        } else {
                            header("Location: invalid.html");
                            exit (0);
                        }
                    }
                } else {
                    header("Location: notallowed.html");
                    exit (0);
                }
            }
        }
    } else {
        header("Location: invalid.html");
        exit (0);
    }
} else {
    header("Location: invalid.html");
    exit (0);
}
//2 END
?>

Security threats in mind:

1. SQL Injections!!! --- Solutions: Prepared Statements (PDO), using only UTF-8, and including "$bpdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);" in the database connection

2. XSS Attacks!!! --- Solutions: htmlspecialchars(), Content-Security Policy (placed in htaccess):

<FilesMatch "\.(html|php)$">
    Header set Content-Security-Policy "default-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; img-src 'self' data: 'unsafe-inline'; media-src 'self' data: 'unsafe-inline'; connect-src 'self';"
</FilesMatch>

3. OS Command Attacks!!! --- Solutions: Striping whitespace (not necessary with emails), validating against a whitelist of permitted values.

4. DOS Attacks!!! --- Solution: None implemented. I'm unsure if any additional precaution is necessary, since there are no login possibilities on my website.

5. PHP Email Injection!!! --- Solution: A Regular Expression (the one I have is mostly designed to allow for international characters).

Additionally, I use an SSL Certificate, SiteLock Security- Essential, CloudFlare CDN, and have implemented a DMARC Policy in my DNS (something I'll be fine tuning for the foreseeable future).

\$\endgroup\$
  • \$\begingroup\$ htmlspecialchars is nonsense. Use this function to sanitize data to be written to a HTML document. You're not doing that anywhere... \$\endgroup\$ – slepic Oct 1 at 3:33
  • 1
    \$\begingroup\$ You should let PDO handle it for you. No extra treatment Is necesary. Further down you do regex check anyway. And that regex basically rules out all HTML entities anyway... Use htmlspecialchars really only before outputting a value into HTML. \$\endgroup\$ – slepic Oct 1 at 3:48
  • 1
    \$\begingroup\$ Unfortunately i am short on time So ill leave that to others. Maybe just one more note, you should not show pdo exception message to the client, that belongs to a server side log. \$\endgroup\$ – slepic Oct 1 at 4:35
  • 1
    \$\begingroup\$ Oh btw i just noticed you posted two versions of this question codereview.stackexchange.com/questions/250058/… you better remove one of them or you're leaving the decision which one will be closed as duplicate to the moderators... \$\endgroup\$ – slepic Oct 1 at 4:41
  • 1
    \$\begingroup\$ What you may and may not do after receiving answers \$\endgroup\$ – Jamal Oct 3 at 1:53
6
\$\begingroup\$

First and foremost, regardless of any real or imaginary security issue, this code is a pain in the eye. It's nearly impossible to get the gist of it and to answer the actual question because the code is constantly shifting out of sight and the vast amount of HTML gets in the way in the midst of the supposedly email verification code. Useless verifications also add to that.

You should really rewrite your code first, in order to make it readable. After all, it's sort of a security issue as well - in such a wilderness it's easy to overlook a real issue. Give your code some love:

  • move the database connection into a separate file and then just include it. By the way, here is how to do it properly. Right now your PDO connection is a security issue because revealing the system error message to the outside world is not a minor one
  • create a function to send emails, put all this $headers .= "From: MY NAME <no-reply@MYDOMAIN.com>\r\n"; business there. then put the function itself into a separate file and then just include it.
  • move the code to send the actual email into a function and put this function at the bottom of the code. So it won't distract a reader from the main algorithm.
  • get rid of the useless code. Checking REMOTE_ADDR makes no sense, there is no situation when it would be invalid. htmlspecialchars is also useless here. and clearing your variables, i.e. $userIP4 = ""; as well
  • get rid of that stepladder of a code. Given your conditions stop the execution anyway - why not to just test for the negative result and stop the execution?

So instead of

if (condition) {
    if (condition2) {
        do something;
    } else {
        display error;
        die;
    } 
} else {
    display error;
    die;
}

just write

if (!condition) {
    display error;
    die;
}
if (!condition2) {
    display error;
    die;
}
do something;

After making your code suitable for the review, you are welcome to ask a new question regarding your security concerns. By far I was able to spot at least one.

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

Unnecessary information disclosure

Don't do this:

$headers .= "X-Mailer: PHP". phpversion() ."\r\n" ;

This will show up in the mail headers and be visible by the recipient. It's nobody's business what version of PHP you're running. In case you're running a version of PHP that has vulnerabilities, this provides hackers with insight into your systems for a tailored exploit. Just don't copy-paste code without understanding the implications...

IPv4 & IPv6

I don't understand the point of gethostbyname. You already have the IP address, so you can store it.

Are you sure your server isn't reachable on the Internet over IPv6 ? You might want to test your site over IPv6 by adding an entry to your DNS configuration eg ipv6.yoursite.com with one AAAA record, and no A record.

Randomization

The verification code is not really random:

$Acode = md5(time().$Temail);

time — Return current Unix timestamp

Someone (a hacker) who knows your formula will be able to bruteforce the verification code because it follows a predictable pattern. You have many better options to generate reasonably random strings.

Misc

I would probably reorder some code, for example save the record to database before sending the mail. If the database crashes for some reason or the connection is lost, you've already sent an E-mail with a verification code that is not recorded anywhere and will not work - which is confusing for the user. If the record was not saved for some reason, notify the user that there was an error (and notify yourself too), and don't proceed further.

The sending of the mail is probably less likely to crash, because it usually goes to a mail queue to be handled by the MTA on your server.

It's possible that the mail doesn't arrive or was discarded or spam-trapped, so it should be possible for the user to request a new code after some time.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Even uniqid could be sufficient here (with some caveats), because we do not need "cryptographically secure values". But the doc also says "This function does not guarantee uniqueness of return value", so consider adding additional entropy. Actually random_bytes is a better option. \$\endgroup\$ – Anonymous Oct 1 at 21:41
  • \$\begingroup\$ "it should be possible for the user to request a new code after some time," Thank you Anonymous. I didn't include it in this post, but I use an hourly cronjob to check the table for rows where a record has gone unverified for 24 hours or more. The cronjob then runs another php file and deletes the 'expired' records. I'm cleaning up my code and plan to make another post that is easier to read and includes everything I have. :) \$\endgroup\$ – CodeLoveGuy Oct 1 at 21:42
4
\$\begingroup\$
  • Regarding db connection and error handling, please read this answer. You must not ever reveal system-generated error details to your end users -- these details are for you and no one else. I recommend a require call, but not before the user's submission qualifies for its usage.

  • There is absolutely no reason that should need to mutate or sanitize the incoming email address. You might like to whitespace trim(), but honestly, who is actually going make the mistake of adding a rogue space? I never have ...ever.

  • For consistency, I always write my negative/failure/false conditional branches before positive/success/true branches. This way, you (or other developers) will know that the lower the script progresses, the more successful the flow has been and additional resources can be initialized/declared.

  • Do not generate the timestamp for the db row in php. You don't even need to mention it in your sql. Set your emailTable's datetime column to DEFAULT to CURRENT_TIMESTAMP. https://stackoverflow.com/q/168736/2943403

  • If the expectation is to permit multibyte characters in email addresses (//allow international characters), your regex is missing the unicode flag. That said, I do not recommend using regex to try to parse/validate an email address because as your validation pattern improves its accuracy, the readability and maintainability plummets. I recommend filter_var() https://stackoverflow.com/q/12026842/2943403

  • $email_host already contains the domain, so don't perform surgery again with explode()/array_pop().

  • SELECT * FROM emailTable WHERE email=? is asking for too much data. You only need the COUNT(), not the row's data. This way, you fetch the count only (which will be zero or one), so the condition is very simple and readable.

  • If you want to DRY out the "header & die" lines, you could create a small helper function that receives the Location text, then calls the header(), then die()s.

  • I would recommend using <<<HTML ... HTML; (HEREDOC) syntax so that you can use double quotes in your markup and inline variables. Alternatively, you could use sprintf(), but that is less compelling with just one variable.

  • Since $headers lines are all delimited by \r\n, I recommend, creating an array of lines, then implode()ing with \r\n to be more DRY. The fourth parameter of mail() is the header data which can receive an array.

  • I do not recommend the native mail(). I always build PHPMailer into all of my projects for ease of use and functionality.

  • I am concerned about the ambiguity if invalid.html. If mail() fails, then the user won't know if there is something that they can fix or not.

  • $userIP4 = ""; $dateTime = ""; $Temail = ""; $Acode = ""; this is all useless nonsense, just omit all of it.

  • Comb through your script and search for single-use variables. When you see a declared variable only used one time, then you don't need to declare it.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ There is absolutely no reason that should need to mutate or sanitize the incoming email address Shouldn't all incoming user input be treated as potentially malicious? \$\endgroup\$ – CodeLoveGuy Oct 2 at 16:19
  • 1
    \$\begingroup\$ Validate: Yes. Sanitize: No. If it is a valid email address then proceed; if it is anything else kill the process. \$\endgroup\$ – mickmackusa Oct 2 at 16:20
  • \$\begingroup\$ My problem with filter_var is that it does not allow international characters. What additions can I include with filter_var to allow for them? \$\endgroup\$ – CodeLoveGuy Oct 2 at 16:23
  • \$\begingroup\$ Hmm... good question. I haven't researched this. \$\endgroup\$ – mickmackusa Oct 2 at 16:23

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.