current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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. -->
share|improve this question

3 Answers 3

up vote 5 down vote accepted

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.

share|improve this answer
    
Thank you for your comments, sir. I actually have a follow-up question which you reminded me about. Should I be closing $con somehow? –  asteri Apr 17 '14 at 17:41
    
Yes, it's good practice to close the mysqli connection. Those connections are pooled, so there won't be much of a performance hit. –  Ollie Jones Apr 20 '14 at 19:14
up vote 4 down vote

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
share|improve this answer
up vote 6 down vote

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 ))

share|improve this answer
1  
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. –  asteri Apr 17 '14 at 12:14
    
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. –  Stepan Stepanov Apr 17 '14 at 12:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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