PHP decode, alter and encode JSON

Question

I'm guessing this has already been answered somewhere, but I can't find the solution I'm tearing my hair out. I have a JSON array stored in MySQL like this:

[{"ip":"8.8.8.8","name":"Bob"},{"ip":"","name":""},{"ip":"","name":""},{"ip":"","name":""}]

I want to replace the "ip" and "name" of a specific object. So I set $slot_num to something like 0 and try to alter the values and UPDATE the database. The SELECT clause below should be fine because it's used several times elsewhere.

//Recieves POST info such as ip=1.1.1.1&group=204&slot=3&name=help
$ip = $_POST['ip'];
$group_id = $_POST['group'];
$slot_num = $_POST['slot'] -1; //PHP receives slot num increased by 1. IE- $slot_num 1 would be array[0]
$name = $_POST['name'];

if($result = $mysqli->query("SELECT * FROM `open_groups` WHERE `group_id` = $group_id")) {
    $row = mysqli_fetch_array($result);
    $slot_ar = json_decode($row['players'], true);
    //Check if array has correct number slots
    if($slot_num => count($slot_ar) || !is_int($slot_num)){
        die('Injection attempt');
    }

    $slot_ar[$slot_num]['ip'] = $ip;
    $slot_ar[$slot_num]['name'] = $name;
    $players = json_encode($slot_ar);
    $players = $mysqli->real_escape_string($players);
    if(!$mysqli->query("UPDATE `open_group` SET players = '$players' WHERE group_id = $group_id")) {
        echo $mysqli->error;
        exit;
    }
    if(!$mysqli->query("INSERT INTO `occupied`(`ip`, `group`) VALUES ('$ip', '$group_id')")) {
        echo $mysqli->error;
        exit;
    }
    echo "Success";
}
else echo $mysqli->error;

Am I accessing the array incorrectly or something?

Fixed code

$ip = $_POST['ip'];
$group_id = $_POST['group'];
$slot_num = $_POST['slot']; //PHP receives slot num increased by 1. IE- $slot_num 1 would be array[0]
$name = $_POST['name'];

if($result = $mysqli->query("SELECT * FROM `open_groups` WHERE `group_id` = $group_id")) {
$row = mysqli_fetch_array($result);
$slot_ar = json_decode($row['players'], true);
//Check if array has correct number slots
if($slot_num-1 >= count($slot_ar) || !is_numeric($slot_num)){
    echo "Injection attempt";
    exit;
}
$slot_ar[$slot_num-1]['ip'] = "$ip";
$slot_ar[$slot_num-1]['name'] = "$name";
$players = json_encode($slot_ar);
$players = $mysqli->real_escape_string($players);
if(!$mysqli->query("UPDATE `open_groups` SET players = '$players' WHERE `group_id` = $group_id")) {
    echo "Update error";
    exit;
}
if(!$mysqli->query("INSERT INTO `occupied`(`ip`, `group`) VALUES ('$ip', '$group_id')")) {
    echo "Occupied error";
    exit;
}
echo "Success";
}
else echo "Fail";

You are not escaping `$players` anywhere, thus you have a syntax error and likely a SQL injection vulnerablity. — Alexander O'Mara, Feb 07 '15 at 19:29
@AlexanderO'Mara Wouldn't it be better to escape $name and $ip, not $player? Also, if you want to consider injection you need to check that $_POST['num'] isn't higher than the amount of places in the json array else people will create new entries through injection. — Charles Forest, Feb 07 '15 at 19:37
I escape `$name`, which I added to the question. The problem is that the data doesn't get changed in the db. — Snailer, Feb 07 '15 at 19:37
@CharlesForest No! The whole JSON string needs to be escaped, not just some of the indexes. It's true that `$slot_num` could be higher than it should be, and that should probably be guarded against, but that's not nearly as dangerous an issue. — Alexander O'Mara, Feb 07 '15 at 19:39
I've updated my code with new, complete code. All I get for a return from calling this is "a blank string." Wha??? — Snailer, Feb 07 '15 at 20:30
Also, if it helps, using Firebug I can check that my request contains valid info like `ip=1.1.1.1&group=204&slot=3&name=help` — Snailer, Feb 07 '15 at 20:43
DANGER : I just thought about this but what if the $slot sent is a string? You need to check that it's indeed an integer value. (Else it would go in the Json array and work through a foreach loop) — Charles Forest, Feb 08 '15 at 00:34
Solved it myself.. it was a combination of small mistakes and errors not reporting. The `is_int()` function was stopping my code every time and not outputting the error from `die()`, it finally did when I just replaced it with a basic `echo`. — Snailer, Feb 09 '15 at 16:41

Jafar Akhondali · Answer 1 · 2015-02-07T20:16:05.840

2

you need to escape $players because json array is full of quotations so when you are inserting it into database using a query, mysql would have trouble in executing and understanding it.
also try to place variables inside single quotes after escaping it. Try this:

$players = $mysqli->real_escape_string($players);
$mysqli->query("UPDATE `open_group` SET players = '$players' WHERE group_id = '$group_id' ");

edited Feb 07 '15 at 20:16

answered Feb 07 '15 at 19:36

Jafar Akhondali

1,552
1
11
25

@AlexanderO'Mara I escaped $players in first line of my code, how can it be valnerable to SQL Injection? are you sure? can you provide an example? – Jafar Akhondali Feb 07 '15 at 19:55
Never mind, guess I just went blind for a second. Have an up-vote then! – Alexander O'Mara Feb 07 '15 at 19:56
I actually liked the warning from @AlexanderO'Mara. http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string – Charles Forest Feb 07 '15 at 20:05
@CharlesForest He's using MySQLi's `real_escape_string`, which is decent enough. Variable binding is usually better though. – Alexander O'Mara Feb 07 '15 at 20:06
"Decent Enough" != Good. I changed the question reference, my bad on that one. – Charles Forest Feb 07 '15 at 20:09
I've added single quotes to the UPDATE clause but no success! – Snailer Feb 07 '15 at 20:11
@CharlesForest if you read the answer to your question, it says "The fact that you use single quotes (' ') around your variables inside your query is what protects you against this" so this code is Good, don't worry it tested it in several places – Jafar Akhondali Feb 07 '15 at 20:13
@Snailer Try debugging your query: if(!$mysqli->query("UPDATE `open_group` SET players = '$players' WHERE group_id = '$group_id' ")) echo $mysqli->error; – Jafar Akhondali Feb 07 '15 at 20:16
@Snailer What is the output now? – Jafar Akhondali Feb 09 '15 at 02:12

score 2 · Answer 2 · edited May 23 '17 at 12:28

2

As AlexanderO'Mara pointed out, you really should go with bind_param() over mysqli_escape_string() which has been proven unreliable on some occasions. I use PDO statement so this is untested.

Source : mysqli real escape string, should I use it?

$slot_num = $_POST['num'];
$name = $_POST['name'];

// Initial select
$sql = "SELECT * FROM `open_groups` WHERE `group_id` = ?";
$stmt = $mysqli->prepare($sql);
$stmt->bind_param('d', $group_id);
$stmt->execute();

if ($stmt->errno) {
  die("Error : " . $stmt->error);
}

$result = $stmt->get_result();
while ($row = $result->fetch_assoc()) {
    $slot_ar = json_decode($row['players'], true); //Get JSON array

    // Make sure $slot_num isn't bigger than desired
    if($slot_num > count($slot_ar) || !is_int($slot_num)){
        die('Injection attempt');
    }
    // Store new variables in the array
    $slot_ar[$slot_num]['ip'] = "$ip";
    $slot_ar[$slot_num]['name'] = "$name";

    // encode the new $players
    $players = json_encode($slot_ar);

    // new SQL query
    $sql = "UPDATE `open_group` SET players = ? WHERE group_id = ?";
    $stmt = $mysqli->prepare($sql);
    $stmt->bind_param('sd', $players, $group_id);
    $stmt->execute();
    if ($stmt->errno) {
      die("Error : " . $stmt->error);
    }

}

edited May 23 '17 at 12:28

Community

1
1

answered Feb 07 '15 at 20:02

Charles Forest

1,035
1
12
30

Did you actually read your SO link? at the end of answer it says : "or there will be no difference between PDO and mysql_escape_string in terms of acting for some extremely rare encodings.' – Jafar Akhondali Feb 07 '15 at 20:09
Skimmed it, my bad on that one. I changed the link. – Charles Forest Feb 07 '15 at 20:17
Then in your new link, it said: "The fact that you use single quotes (' ') around your variables inside your query is what protects you against this" which is why i used, believe me my code is safe i made many researches on it – Jafar Akhondali Feb 07 '15 at 20:20
The kind of mistakes that leads to having unprepared statements insecure are the kind of mistakes I'd expect someone who doesn't protect himself against SQL injection in the first place to make. You're right though, your code is safe. I +1ed it. – Charles Forest Feb 07 '15 at 20:24
Well, you are right too. Sometimes it's better to not dig the code and use prepared functions! – Jafar Akhondali Feb 07 '15 at 20:26
`is_int` was giving me trouble. Check out http://php.net/manual/en/function.is-int.php.. it doesn't work on strings! – Snailer Feb 09 '15 at 16:39

PHP decode, alter and encode JSON

Fixed code

2 Answers2