0
\$\begingroup\$

I have written a simple PHP Script that takes $_POST input and uses it to add data to a .json file. Given the security flaws in my last post, I wanted to post this just to make sure. I can't see anyway for someone to attack my server using this program (especially because this is a script for an internal website), but you guys have often proven me wrong in claims like this. So if you see ANY security flaws, please point them out.

<?php 
require "helper.php";
$dest = "../forms/additem.php";
if(!$sql) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
}
if(!$_POST || !isset($_POST["handout"]) || !isset($_POST["quantity"]) || !isset($_POST["item"])) {
    send(array("msg" => "ERROR: Did not receive Data"));
}
$hid = $_POST["handout"];
$quan = $_POST["quantity"];
$item = $_POST["item"];
$json = jread("../data.json");
if(!$json) {
    send(array("msg" => "ERROR: Cannot read JSON File"));
} 
foreach($json as $key => $property) {
    if($key == $hid) {
        $json[$key][0] = $item;
        $json[$key][1] = $quan;
        jwrite("../data.json", $json);
        send($dest, array("msg" => "SUCCESS: Your Item has been Added!"));
    }
}
send($dest, array("msg" => "ERROR: Either Handout doesn't exist, or item doesn't exist"));
?>
\$\endgroup\$
  • \$\begingroup\$ Not sure I get what you're trying to do, but why do you do a foreach instead of just getting the array element you want directly? Looking at this, I see many other potential problems but since I don't know where some of your functions come from, I can't be sure. Note that input validation and sanitizing is always important. \$\endgroup\$ – Julie Pelletier Apr 30 '16 at 20:49
1
\$\begingroup\$

I can't see anyway for someone to attack my server using this program (especially because this is a script for an internal website)

It's not that important that it is internal. You may be vulnerable to CSRF, XSS, etc (not in this piece of code, but maybe somewhere else), which may allow an attacker to attack your internal website (they would need some information about the infrastructure, but you already posted part of the source here, and your security shouldn't rely on the secrecy of script names, source code, etc).

But no, there shouldn't be any way to attack your server. You removed all your SQL queries (or did you just not post them?), the filename that is written to is hardcoded and cannot be overwritten, you don't echo anything, don't use any dangerous functions, etc.

The only thing you do with user input is to write it into a .json file. So as long as your server isn't configured to interpret .json files as something else - eg PHP files - (which it really shouldn't, and no server would be configured like that by default) there is no danger in the code you did post (but I'm guessing it's just a very small part of your program anyways).

Misc

  • your variable names could be improved. quan and hid are both unclear.
  • I would move the json check above the POST assignments. That way all the checks are at the top and not separated by the assignments, making it easier to see when what error is returned.
  • as @Julie Pelletier said, your foreach loop isn't needed and may impact performance.
\$\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.