0

I have until now been accessing $_REQUEST in my PHP as follows:

//JS
xmlhttp.open("GET", "logic.php?q=" + itemOne  + "&w=" + itemTwo, true);

//PHP
$q = $_REQUEST['q'];
$w = $_REQUEST['w'];

The items being sent through get used for MSSQL server queries (SQLSRV).

My question is what would be the best-practice methods for doing the above differently/correctly? I read somewhere that this is not good in terms of being vulnerable to SQL injection attacks etc.

BernardV
  • 640
  • 10
  • 28
  • it's totally ok with using $_REQUEST. you have to take care with queries. simply use ajax get/post request for it. – Bhavin Jul 18 '16 at 13:34
  • 1
    It's not possible to tell from that code if it's vulnerable or not to sql-injection. But take a look at [How can I prevent SQL-injection in PHP?](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) – FirstOne Jul 18 '16 at 13:35
  • 1
    A side note is to use the 'correct' global according to the request. If you are seding a `post`, access the variables from `$_POST`, if `get`, then `$_GET`. Take a look at [What's wrong with using $_REQUEST[\]?](http://stackoverflow.com/questions/2142497/whats-wrong-with-using-request) – FirstOne Jul 18 '16 at 13:38

1 Answers1

1

My question is what would be the best-practice methods for doing the above differently/correctly?

The example JavaScript you gave used a GET request. The "correct" way to access the parameters would be through PHP's $_GET array. Using $_REQUEST is a bad habit because you lose control over how the data arrived. I'll give you a simple example:

Websites that use token base authentication often require that you send the token as POST data. If it is considered insecure to exchange private info through URL parameter, a PHP script that gets the data from $_REQUEST has no way to know how the data arrived, and will mistakenly accept a badly sent token. A better script would look for the token in $_POST. If it's not there, then there is no token; even if a user tried to send it in the url.

I read somewhere that this is not good in terms of being vulnerable to SQL injection attacks etc.

SQL injection doesn't have to do with $_REQUEST specifically. It can occur whenever you insert user submitted data directly in your SQL queries, whether the data came from $_REQUEST, $_GET, a file... This terrible code design allows an attacker to take control of your SQL and instruct your DB to execute whatever command he or she wishes (eg: to exfiltrate or delete your data). To protect yourself against it, learn about prepared statements and parameterized queries

BeetleJuice
  • 39,516
  • 19
  • 105
  • 165
  • Thanks for the answer! I will look into the correct usage of `$_GET` array. Okay I think luckily from a SQL injection point of view then I should be okay as I have: a) full up front JS field validation before it reaches this point to get data in line with what I want b) I use prepared statements with only parameters c) the MSSQL user I use to log in with may only read data from the DB. – BernardV Jul 18 '16 at 13:52
  • 1
    If my solution answered your question, select it! :-) You should think as front-end validation as a convenience service for the user, not a security feature for the server. This is because it's really easy to send http requests to your server using bad parameters without ever using your front-end site. – BeetleJuice Jul 18 '16 at 13:56
  • Thanks! I will do so! Just in terms of using `$_GET` correctly does the below look better? `$_GET["q"] = $q; $_GET["w"] = $w;` – BernardV Jul 18 '16 at 13:59
  • 1
    It's usually the other way around. Not `$_GET["q"]=$q` but `$q=$_GET['q']` because you're trying to read the values that PHP placed in `$_GET` when the browser sent data to the server, and save those values in local variables (eg: `$q`) – BeetleJuice Jul 18 '16 at 14:06
  • When I do that Netbeans says: `Do not Access Superglobal $_GET Array Directly. Use some filtering functions instead (e.g. filter_input(), conditions with is_*() functions, etc.).` – BernardV Jul 18 '16 at 14:13
  • 1
    That's not a message from PHP. It's your code editor trying to get you to build good habits. It's a good reminder that you should treat incoming data as untrusted. – BeetleJuice Jul 18 '16 at 14:16
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/117616/discussion-between-user3324415-and-beetlejuice). – BernardV Jul 18 '16 at 14:21