Reputation:

Add a rate limit on this AJAX/PHP MySQL search field

I am making a webpage in which users are allowed to search a MySQL Database from a search field, here's the code:

<script>
function finduser(str) {
    if (str == "") {
        document.getElementById("txtstatus").innerHTML = "";
        return;
    } else { 
        if (window.XMLHttpRequest) {
            // code for IE7+, Firefox, Chrome, Opera, Safari
            xmlhttp = new XMLHttpRequest();
        } else {
            // code for IE6, IE5
            xmlhttp = new ActiveXObject("Microsoft.XMLHTTP");
        }
        xmlhttp.onreadystatechange = function() {
            if (this.readyState == 4 && this.status == 200) {
                document.getElementById("txtstatus").innerHTML = this.responseText;
            }
        };
        xmlhttp.open("GET","search.php?q="+str,true);
        xmlhttp.send();
    }
}
</script>

and search.php

 <!DOCTYPE html>
<html>
<head>

</head>
<body>

<?php
$q = htmlspecialchars($_GET['q']);

$con = mysqli_connect('localhost','user','pass','db');
if (!$con) {
    die('Could not connect: ' . mysqli_error($con));
}

mysqli_select_db($con,"db");
$sql="SELECT *  FROM people WHERE CONCAT( firstname,  ' ', lastname) LIKE '".$q."' OR phone LIKE '".$q."' OR email LIKE '".$q."'";
$result = mysqli_query($con,$sql);

$num_rows = mysqli_num_rows($result);

echo "<div class='records'><small>" . $num_rows . " records</small></div>";


while($row = mysqli_fetch_array($result)) {

    if (is_null($row['hidden'])) {
        } else {
    continue;
    }
    echo "<div class='people'><small><a href='" . $row['id'] . "'>" . $row['firstname'] . " "  . $row['lastname'] . "</a> submitted on " . date("M j\, Y", strtotime($row['timestamp']))"</small></div>";
}
echo "</ul>";
mysqli_close($con);
?>
</body>
</html>

Few questions:

1) Is this code secure to protect agains attacks such as mysql injection attacks? If not how do I secure it?

2) I would like to impose a rate limit, in which a user can only search a certain amount of times every second to prevent against flooding. Like 5 queries a minute. This is just a personal project, there's not like finical information at stake or anything like that but I would like to be shown how to do this correctly. What's the best way of doing this?

I suspect someone will already tell me to put the login credentials into a separate PHP file and include it. I intend on doing that, can someone inform me why that is a good practice to do? Is it in the event that the server somehow accidentally sends out the raw PHP upon HTML request?

Upvotes: 1

Answers (1)

Gabor Lengyel

Reputation: 15570

This code is about the guinea pig of some of the most common web application vulnerabilities. :) Which is totally fine, and may be educational for others as well.

SQL injection

You are using htmlspecialchars wrong. It is not an input sanitization function, but an output encoding one. It is useless the way you use it, and should be removed from there. As there is no effective protection, the code above is vulnerable to SQL injection with an input on the q parameter similar to this: union select * from other; -- \ (mind the space at the beginning).

With this, your query on line 17 would be

SELECT *  FROM people WHERE CONCAT( firstname,  ' ', lastname) LIKE ' union select * from other; -- \' OR phone LIKE ' union select * from other; -- \' OR email LIKE ' union select * from other; -- \'

Note that htmlspecialchars does nothing to a backslash, and it doesn't have to. Removing the LIKE expression string for readability and because of how character escaping works, this effectively equals

SELECT *  FROM people WHERE CONCAT( firstname,  ' ', lastname) LIKE '...' union select * from other

Some solutions are to use prepared statements, PHP PDO or an ORM. This has been answered multiple times here. It's even described in the PHP manual.

You should always have proper protection against SQL injection, and you should never concatenate SQL statements with parameters like you did.

Cross-Site Scripting (XSS)

The code above is potentially vulnerable to XSS, depending on how data gets into the database. But regardless of that, your code should be more robust against XSS by sanitizing dynamic output.

The problem is that in Javascript, you use innerHTML to insert data into the DOM, which you just read from the database in PHP and never encode before writing to the page. If the fields firstname or lastname ever contain HTML code like a script tag with javascript, it will be written and inserted to the page and the javascript will be run.

To avoid this, you should

Use htmlspecialchars(), htmlentities(), or an output encoding library to properly encode your output. Note that the first two functions are for an HTML context, and they are not adequate for Javascript (which does not apply here, because you are writing to plain HTML). So in your example, you should apply encoding to variables in echo lines, like htmlentities($row[firstname]) etc.
In Javascript, avoid innerHTML if you can, and use innerText instead. As your PHP writes HTML with tags, you can't do this, and that's ok, but then all variables need to be encoded as shown above.
Note that while it's unlikely that the id column would ever be user input, as a best practice, you should not write variables alone into the href of a link. Consider this XSS: <a href="javascript: alert(1)">, if the id was javascript:.... (And btw writing an id like that to an href doesn't make much sense anyway, unless you have really funky ids in the database.)

Unencoded string inserted into the URL (very weak URL injection)

In your Javascript, the str parameter to the function is supposedly user input somehow (maybe comes from the value of an input field, or through some logic). That string is directly concatenated to a URL, which may or may not be exploited in a compound attack. It's not a direct vulnerability, but it's a weakness I would say. You should URL encode that value before adding it to the queried URL.

Hardcoded credentials

As you correctly pointed out, credentials should not be hardcoded. But the reason is not that a PHP file would ever be returned as text. If it were, a different PHP would not help as that too would be returned as text. :) But how would you put this into version control for example? Anybody that has access to your codebase would have access to production secrets, which is unacceptable in many environments. Also a change to these credentials would mean a new "release", a new version of the software, because you had to change code, not just configuration.

So as a best practice, secrets (at least in the production version) should be read from somewhere safe, for example environment variables, which the webserver can set for PHP. That way, if access rights are set correctly, you would have to be the webserver user (or even root) to gain access to these credentials (or would have to impersonate the application process, which would of course be enough to read its environment).

How exactly you can set an environment variable for your PHP process depends on your webserver, but the way to read it in PHP is something like getenv("databasepassword").

This prevents file inclusion attacks from getting your secrets (because secrets are not in a file accessible to your application), and also allows you to freely check in any code to version control.

Potential denial of service (DoS)

The query in your question is very inefficient due to the concat() on two string fields, being used in like comparisons.

As you mentioned, rate limiting (throttling) is one option you can choose, but it's not straightforward to implement in PHP.
You can do this rate limiting in your webserver, which is off-topic here, but Apache, Nginx etc. can all do this if configured correctly.
You can implement something like a captcha so that valid queries can only be sent by humans.
You can implement authentication for such functions, and accept the residual risk of authenticated users exploiting DoS. You could probably handle that by different means, eg. service terms.