3
\$\begingroup\$

For a website, I've got some inline PHP, posted below. It's supposed to log traffic to the website, and it does its job fine. But at the end of the day, I'm not even close to a PHP developer, and this is really just hacked together from Googling and inferring from other, more familiar languages. How can I improve this? Is using PHP in the actual file like this bad? (This is in index.html.) Also, I don't believe I'm open to an SQL injection attack since the variables are all drawn directly from the server rather than the user, but I could be wrong.

<body>
    <?php
        $ip = (!empty($_SERVER["HTTP_CLIENT_IP"]) ? $_SERVER["HTTP_CLIENT_IP"] : $_SERVER["REMOTE_ADDR"]);
        $fwd = (!empty($_SERVER["HTTP_X_FORWARDED_FOR"]) ? "\"".$_SERVER["HTTP_X_FORWARDED_FOR"]."\"" : "null");
        $hostname = gethostbyaddr($ip);

        if($ip !== "127.0.0.1" && $ip !== "192.168.1.1") {
            $con = mysqli_connect("localhost","username","password","database");
            if(!mysqli_connect_errno($con)) {
                $result = mysqli_query($con, "INSERT INTO table_name VALUES(\"".$ip."\", now(), \"".$hostname."\", ".$fwd.")");
            }
        }
    ?>
    <div class="container-fluid">
        <div class="row-fluid">
            <!-- etc. -->
Improve this question
\$\endgroup\$

3 Answers 3

Reset to default
5
\$\begingroup\$

a couple of things.

gethostbyaddr() is notoriously slow and unpredictable in speed. So be careful with that.

You may wish to use a prepared SQL statement to injection-proof your code. That would go like this, with error checking:

if (!($stmt = $con->prepare("INSERT INTO table_name VALUES (?, ?, ?)"))) {
   echo "prepare failed: (" . $mysqli->errno . ") " . $mysqli->error;
}
if (!($stmt->bind_param('sss', $ip, $hostname, $fwd))) {
   echo "bind_param failed: (" . $mysqli->errno . ") " . $mysqli->error;
}
if (!($stmt->execute())) {
   echo "execute failed: (" . $mysqli->errno . ") " . $mysqli->error;
}
$stmt->close();

It's a little more overhead, but it's safer.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your comments, sir. I actually have a follow-up question which you reminded me about. Should I be closing $con somehow? \$\endgroup\$
    – asteri
    Apr 17, 2014 at 17:41
  • \$\begingroup\$ Yes, it's good practice to close the mysqli connection. Those connections are pooled, so there won't be much of a performance hit. \$\endgroup\$
    – O. Jones
    Apr 20, 2014 at 19:14
6
\$\begingroup\$

I don't think its really SQL-injection, but $hostname is not taken from server but from DNS.

Theoretically i can add some kind of malicious domain name and affect your query. But from other hand, domain syntax is really limited. I don't think you can use it to do some serious thing.

Though i would recommend some error protection to avoid SQL-errors.

And correct mysql-escaping can never be bad ))

Improve this answer
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for your comments. What do you mean by "correct MySQL escaping"? I thought I was escaping the values appropriately in the string. At least, appropriately enough for the INSERT to work, haha. \$\endgroup\$
    – asteri
    Apr 17, 2014 at 12:14
  • \$\begingroup\$ I mean mysql_real_escape_string. E.g. mysql_real_escape_string($hostname) instead of $hostname. You think you always get correct domain or ip. But what if you get an error message, or faked domain name. \$\endgroup\$
    – Stepan Stepanov
    Apr 17, 2014 at 12:24
4
\$\begingroup\$

I prefer to use PDO when working with database. I would say, that it is a standard these days.

You should always escape input that is not directly under your control. That includes values which come from $_SERVER as well. It might look like safe source of data, but that is not always correct. This is a good read regarding values coming from $_SERVER.

You can easily avoid SQL injection by using prepared statements. There is no need to create your SQL queries by putting values from variables directly inside SQL queries.

  • "\"" can be simplified to this '"'
  • if a string does not contain any variables it should be wrapped into '' instead of "", because PHP interpreter does not need to check if a string contains variables in that case
Improve this answer
\$\endgroup\$

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.