Adding an item to database + JSON

Question

Is this code good? Or is it the noobiest PHP you've ever seen?

<?php
//Initialization Section
require "helper.php";
$dest = "../forms/addhandout.php";

//Check for $_POST Data 
if(!$_POST) {
    send(array("msg" => "ERROR: No Data Recieved"));
} else if(!isset($_POST["name"]) || $_POST["name"] == "") {
    send(array("msg" => "ERROR: Name field cannot be left blank"));
} else if(!isset($_POST["class"])) {
    send(array("msg" => "ERROR: Class field cannot be left blank"));
}

//Set $_POST Data 
$name = $_POST["name"];
$cid = $_POST["class"];

//Check if PHP is connected to database
if(!$sql) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
}

//Check for Existing Handout
$query = "SELECT id FROM Handouts WHERE name='$name' AND cid=$cid";
$result = mysqli_query($sql, $query);
if(!$result) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
} else if(mysqli_num_rows($result) > 0) {
    send(array("msg" => "ERROR: Handout already exists"));
}

//Add Handout to DB
$query = "INSERT INTO Handouts (name, cid) VALUES ('$name', $cid)";
$result = mysqli_query($sql, $query);
if(!$result) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
}

//Get Handout ID
$query = "SELECT id FROM Handouts WHERE name='$name' AND cid=$cid";
$result = mysqli_query($sql, $query);
if(!$result) {
    send(array("msg" => "ERROR: Cannot Connect to Database"));
}
$hid = mysqli_fetch_assoc($result)["id"]; 

//Get JSON from file
$json = jread("../data.json");

//Add handout to JSON
$json[$hid] = array();

//Write JSON to file
jwrite("../data.json", $json);

//Close Connection to DB 
mysqli_close($sql);

//Return to Web Form
send(array("msg" => "SUCCESS: Your Handout has been Created!"));
?>

<?php 
define("HOST", "localhost");
define("USR", "web");
define("PSWD", "thundercats...thundercats...HO!!!!!");
define("DB", "Inventory");

$sql = mysqli_connect(HOST, USR, PSWD, DB);
$dest = "";

function jread($file) {
    $handle = fopen($file, "r");
    if(!($data = fread($handle, filesize($file)))) {
        return FALSE;
    }
    $json = json_decode($data, TRUE);
    return $json;
}

function jwrite($file, $data) {
    $handle = fopen($file, "w");
    if(fwrite($handle, json_encode($data))) {
        return TRUE;
    } else {
        return FALSE;
    }
}

function redir() {
    global $dest;
    header("Location: " . $dest);
    exit();
}

function send($params) {
    global $dest;
    $get = http_build_query($params);
    header("Location: $dest?$get");
    exit();
}
?>

Answer 1

SQL Injection

To expand a bit about what @Åna said: Yes, you are open to SQL injection. mysqli_query doesn't actually support multi-queries, so the described attack doesn't work, and I would also suggest a different defense than escaping.

Even though multi-queries do not work, an attacker can still read out any values from the database. There are two possibilities for this:

• Blind: An attacker can inject their own query into your first select query. They would use a non-existing name and cid, and then append a true/false statement to the cid (eg does the database password start with an a? no? does it start with a b? and so on). The attack is a bit noisy as it takes a couple of requests, and a false guess creates new entries, but it's still a serious attack vector.
• Via your insert query: select queries are allowed as subqueries of insert queries. So an attacker would select eg the database password via the cid parameter, and then insert it into the handouts table via your insert query, and then read it out from your json files (if they have access to them, which seems to be the case here).

Additionally, an attacker may be able to read from and write to any file in your system, depending on the database permissions.

The proper defense against this is not escaping, but using prepared statements.

Comments

Your comments are largely unnecessary or could be unnecessary with better code structure.

Something like Set $_POST Data or Get JSON from file is not needed, as the code is self-descriptive.

Something like Check for Existing Handout or Add Handout to DB should be in its own function, eg selectHandout($name, $cid) or insertHandout($name, $cid). This would also get rid of the duplication you have (the select queries).

Misc

• as mentioned above, introducing more functions will get rid of the duplication you have.
• you shouldn't hard-code your database password inside a file that does other stuff. Create a specific config file instead (to avoid accidentally exposing your password when posting your code, in version control, etc). [fyi, you might want to change your password if it's your real one, and use a more secure one (6 characters aren't enough)]
• don't shorten names. redir is unclear (does it maybe work on a directory? ) redirect is better.
• you want to avoid globals. Just pass the variables on via function arguments.

Answer 2

You have a major security vulnerability.

As in, serious enough that if I knew your web address, I could delete your entire database.

Near the top of your code, you set $cid to the raw value of $_POST["cid"]. That's bad. You need to escape this data before you put it anywhere near a database.

As of rit now, if I send a POST request to this page with the cid parameter set to ; DROP DATABASE Inventory; --, your entire database is gone. That string will be put into the $cid variable, and then when you execute these lines:

$query = "SELECT id FROM Handouts WHERE name='$name' AND cid=$cid";
$result = mysqli_query($sql, $query);

the following query will be executed on your database:

SELECT id FROM Handouts WHERE name='' AND cid=; DROP DATABASE Inventory; --

The first query will throw a syntax error, the second will delete your database, and any subsequent queries are commented out.

You need to fix this before you do anything else.