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
– Alexander O'Mara, Commented Feb 7, 2015 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
– Charles Forest, Commented Feb 7, 2015 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
– Snailer, Commented Feb 7, 2015 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
– Alexander O'Mara, Commented Feb 7, 2015 at 19:39

Jafar · Accepted Answer · 2015-02-07 20:16:05Z

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 7, 2015 at 20:16

answered Feb 7, 2015 at 19:36

Jafar

1,5291 gold badge11 silver badges27 bronze badges

Sign up to request clarification or add additional context in comments.

9 Comments

Jafar Over a year ago

@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?

Alexander O'Mara Over a year ago

Never mind, guess I just went blind for a second. Have an up-vote then!

Charles Forest Over a year ago

I actually liked the warning from @AlexanderO'Mara. stackoverflow.com/questions/5741187/…

Alexander O'Mara Over a year ago

@CharlesForest He's using MySQLi's real_escape_string, which is decent enough. Variable binding is usually better though.

Charles Forest Over a year ago

"Decent Enough" != Good. I changed the question reference, my bad on that one.

|

Community · Accepted Answer · 2017-05-23 12:28:29Z

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, 2017 at 12:28

CommunityBot

11 silver badge

answered Feb 7, 2015 at 20:02

Charles Forest

1,0451 gold badge13 silver badges30 bronze badges

6 Comments

Jafar Over a year ago

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.'

Charles Forest Over a year ago

Skimmed it, my bad on that one. I changed the link.

Jafar Over a year ago

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

Charles Forest Over a year ago

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.

Jafar Over a year ago

Well, you are right too. Sometimes it's better to not dig the code and use prepared functions!

|

Collectives™ on Stack Overflow

PHP decode, alter and encode JSON

Fixed code

2 Answers 2

9 Comments

6 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

Fixed code

2 Answers 2

9 Comments

6 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related