How can I set a WHERE condition on UPDATE and DELETE?

iljoja

I need a safe solution: it needs to be done in PHP at server side.

I use Datatables Editor PHP Library.

I have a table like:
MY_CARS: id, make, model, plate, vin, owner_id

I want the user to be able view, add, edit and remove only their own cars.

SELECT:

I can limit the list of cars shown to the user with where conditions like this:

$editor->where( 'owner_id', $user_id );

This seems safe for SELECT.

But it is noted in the manual: "It is important to note that the conditions applied by Editor->where() are used only on data fetch."

INSERT:

When creating a new car I can set the field like this:

$editor->field(
new Field( 'owner_id' )
->set( Field::SET_CREATE )
->setValue( $user_id )
);

This seems safe for INSERT.

UPDATE & DELETE:

For DELETE I have no idea.

I could do this for UPDATE:

$editor->field(
new Field( 'owner_id' )
->set( Field::SET_EDIT )
->setValue( $user_id )
);

This doesn't seem safe at all. If user send a malicious request to the server with a modified id field of a car owned by another user this would mean that the row with that id would change to the malicious user. This would result in potential information disclosure and "a theft of a vehicle"

If I would use SQL directly, I would just add a additional WHERE condition to the SQL command like this (sorry for the PDO style):

UPDATE CARS .... WHERE id = :id AND owned_id = :owner_id;
DELETE FROM CARS WHERE id = :id AND owned_id = :owner_id;

How can I make this safely with Datatables Editor PHP Library?

Edited by Allan - Syntax highlighting. Details on how to highlight code using markdown can be found in this guide.

rf1234

The answer depends on whether or not you are also using the Editor client side libraries. Are you using these, too?

If you are using Editor client side as well you
- will only update the respective edited record(s), not all of them
- will only delete the selected record(s)

You do not need to specify a WHERE clause for purposes of DELETE and UP'DATE but you might want to have a WHERE clause to limit record selection when reading the database.

https://editor.datatables.net/manual/php/conditions

iljoja

Yes, I'm using the client side library also. But unfortunately it can not be trusted that a malicious user would be restricted by the JavaScript side in the browser

The ajax request to the server can be forged/edited using developer tools. It is not enough to restrict this on the browser side. It has to be in PHP on the server.

rf1234

Then it is a question to @allan, I guess. I don't think anyone else is able to tell whether this is prevented by suitable server side code.

allan

This doesn't seem safe at all. If user send a malicious request to the server with a modified id field of a car owned by another user this would mean that the row with that id would change to the malicious user.

That's not the case. If you use Field->setValue() then it doesn't matter what the end user sends for that field it will be ignored. The setValue() takes priority over it. This is the piece of code in question that does that.

Allan

rf1234

Ok, I didn't understand the question in a way that $user_id is the id of the user logged in. If this ID is not sent by the client then of course it can't be manipulated.

I have a similar requirement and resolve it like this, for example:

Field::inst( 'contract.updater_id' )->set(Field::SET_BOTH)
                                    ->setValue( $_SESSION['id'] ),
Field::inst( 'contract.creator_id' )->set(Field::SET_CREATE)
                                    ->setValue( $_SESSION['id'] ),

As usual in PHP I save the ID of the current user in a session variable.

iljoja

Thank you very much for your quick reply! And sorry for my very very long response.

Based on the code link you sent and tests I made in my app, it still seems that the "theft of a vehicle" scenario will happen

Let me give you an example:

Let's assume I have two row in the mentioned database:
MY_CARS: id, make, model, plate, vin, owner_id
1, "Land Rover", "Range Rover", "AAA-BBB", "SOMEBOGUSVIN1", 1
2, "Mini", "Mark I", "CCC-DDD", "SOMEBOGUSVIN2", 2

I have these conditions in the code (did not include the other field in this example snippet):

$editor
    ->field(
        new Field( 'owner_id' )
            ->set( true )
            ->setValue( $user_id )
    )
    ->where( 'owner_id', $user_id );

I am logged in with $user_id = 2;. When I open the Datatable page, I can only see "my" Mini.

If I am logged in with $user_id = 1;, I can only see "my" Land Rover.

But if I send a malicious forged ajax call (using DevTools) with the following update while still logged in with $user_id = 1;:

(This can be run from Edge DevTools - Console)

fetch("https://my.dev.site/handler.php", {
  "headers": {
    "accept": "application/json, text/javascript, */*; q=0.01",
    "accept-language": "en,fi;q=0.9,en-GB;q=0.8,en-US;q=0.7",
    "content-type": "application/x-www-form-urlencoded; charset=UTF-8",
    "sec-ch-ua": "\"Microsoft Edge\";v=\"111\", \"Not(A:Brand\";v=\"8\", \"Chromium\";v=\"111\"",
    "sec-ch-ua-mobile": "?0",
    "sec-ch-ua-platform": "\"Windows\"",
    "sec-fetch-dest": "empty",
    "sec-fetch-mode": "cors",
    "sec-fetch-site": "same-origin",
    "x-requested-with": "XMLHttpRequest"
  },
  "referrer": "https://my.dev.site/index.php",
  "referrerPolicy": "strict-origin-when-cross-origin",
  "body": "action=edit&data%5Brow_2%5D%5BMY_CARS%5D%5Bmake%5D=New",
  "method": "POST",
  "mode": "cors",
  "credentials": "include"
});

Ajax call equivalent in SQL:
UPDATE MY_CARS SET make = 'New' WHERE id = 2;

The result is:

1. "Land Rover", "Range Rover", "AAA-BBB", "SOMEBOGUSVIN1", 1
2. "New", "Mark I", "CCC-DDD", "SOMEBOGUSVIN2", 1

Please note the change of the owner_id in the table. This would mean that with $user_id = 2; I have no cars left. Somebody stole my Mini!!!

And with $user_id = 1; I would have 2 cars... Nice theft!

REASON:

The reason for this is that the ->setValue( $user_id ) overwrites the original value but doesn't check if the original row should have been available in the first place.

MY CONCLUSIONS AND SUGGESTED FIX

I would see this as a security issue in the Datatables PHP Library (and probably in other Datatables server libraries too). A malicious user can edit row that should have been inaccessible for the user. The current behaviour easily leads to information disclosure and failure of data integrity.

If the WHERE condition would be applied to UPDATE and DELETE the SQL equivalent of the ajax would be:
UPDATE MY_CARS SET make = 'New' WHERE owner_id =:owner_id AND id = 2;
(With bind :owner_id to $user_id)

This would have stopped the ajax query from updating the table on row(s) not visible for the user.

So, let's start applying the WHERE condition also for the UPDATE and DELETE.

IMHO: Including WHERE condition for UPDATE and DELETE should be the default behaviour. There might be also a need for a parameter/switch to disable this in some rare cases, but security first

Edited by Allan - Syntax highlighting. Details on how to highlight code using markdown can be found in this guide.

allan

Hi,

Many thanks for the details and explanation, I'm with you now and totally agree. There is however a number of valid use cases for this sort of behaviour - transferring something between departments for example.

I'm going to have to think about this more in depth so we don't break that sort of use case (I use it myself and I know plenty of others do as well), but 100% agree that there is a security implication here.

What I would suggest is adding a validator to make sure that the user who is editing the row has access rights to do so (this might actually be a better solution than adding a WHERE since that would not throw an error).

You could use the dbValue validator:

$editor
    ->field(
        new Field( 'owner_id' )
            ->set( true )
            ->setValue( $user_id )
            ->validator(
                Validate::dbValues(
                    ValidateOptions::inst()
                        ->message('ACCESS DENIED!'),
                    null,
                    null,
                    null,
                    [ $user_id ]
                )
            )
    )
    ->where( 'owner_id', $user_id );

There is one hitch - the PHP libraries do not currently do validation on the setValue if that is provided, which I think is wrong. I've corrected that here. You could patch your local Field.php file with that and I'll get a release with that change made soon.

Allan

rf1234

I think Allan's solution is great. But for your use case it isn't even required because there is no need to use the user_id returned from the client. There isn't even a need to return it from the client at all. There might be a need to send it to the client if something was done with it on the client side. But I doubt that, too.

Since user_id is the id of the user logged in you can always use the session variable with the id of the user. No need for client side values.

Let's assume it was needed nevertheless. In that case you can handle it with the current Editor release. You could use the cancellable events "preEdit", "preCreate" or "preRemove" and check whether $user_id deviates from the id of the user logged in.

https://editor.datatables.net/manual/php/events

iljoja

I'm very sorry, if I'm being a little unclear and maybe a pain in the ass too, but just to clarify (or confuse) the matter further:

$user_id is never coming from the client. It is not an input from POST or GET variables. It is a variable holding the user id that is logged in. In my case this is provided by an external authentication API. In a sense it is the same as $_SESSION['id'] mentioned in here or anything that identifies the user on server side.

Also, please note that the problem is not that the Editor interface or the malicious user's forged request would send a false or forged owner_id value. In my example I mentioned that the ajax call equivalent in SQL would be (note, no owner_id present):
UPDATE MY_CARS SET make = 'New' WHERE id = 2;

In ajax call:
"body": "action=edit&data%5Brow_2%5D%5BMY_CARS%5D%5Bmake%5D=New",

So there is no owner_id present in the update. The value that has been forged in the ajax call is the id field value. The car with id number 2 should not be visible or editable to he interface, because it is excluded during data fetch by ->where( 'owner_id', $user_id );.

Allan's suggestion:

Regarding the fix you made to the code, I totally agree. The validator should check the value set by setValue()!

But does this solve the root issue? Does it check the value that the row had before on owner_id column? If the row was not included when fetching the lines to the editor, should it be allow to be edited by a forged ajax call?

Please correct me if I misunderstood, but to me it seems it doesn't solve this issue. If I understand correctly, it checks the $user_id is one of the values from list [$user_id]. But since the $user_id is always guaranteed to be the one that the user is logged in with and is not in any way a user input, I see no need for this. Did I understand correctly??? I'm no very familiar with the dbValues validator, so it might be that I'm getting this wrong

Regarding:

There is however a number of valid use cases for this sort of behaviour - transferring something between departments for example.

I also make sometimes use of this kind of transfers. But I see no problem regarding it. Since the WHERE condition would be applied to the existing row values and not the new ones provided by the editor. The user would be able to transfer something that he can see in his own editor, but not something that he can't see, because he/she has no rights to it.

Let me rephrase: Usually you would not want a employee with access only to the department A file cabinet to break in and steal a department B folder from their cabinet and then transfer it to the file cabinet of department A. But you might ask an employee from department B (with suitable access) to hand over the files to department A, if needed.

Addition note:
There seems to be a deprecated funtion called whereSet(). This might do what is needed for the UPDATE, but to me it seems that it doesn't count in DELETE. So the user could still delete a row that should be protected by the where condition.

rf1234's suggestion

As I mentioned earlier in this message, you are absolutely right that we should not get the value of $user_id from the client, not even the owner_id is or should be included in the ajax call.

I have digged in to "preEdit", "preCreate" and "preRemove". I notice that these have some possibilities for a work around. I could use something similar to what is happening here:

$editor->on( 'preEdit', function ( $editor, $id, $values ) {
        $prevValues = $editor->db()->select( $editor->table()[0], '*', [ 'id' => $id ] )->fetch();
        ... // Check the value if owner_id equal $user_id and and cancel event if not
}

But this approach still doesn't fix the security implications of not including the WHERE condition in UPDATE and DELETE as default since it requires that every time that WHERE condition is used to limit user access to database rows something similar needs to be included at least for "preEdit" and "preRemove".

rf1234

But this approach still doesn't fix the security implications of not including the WHERE condition in UPDATE and DELETE as default since it requires that every time that WHERE condition is used to limit user access to database rows something similar needs to be included at least for "preEdit" and "preRemove".

I think you would need to catch those things that are security relevant and handle those using a validator or "preEdit", "preRemove" etc. I thought about it in my use case. The only thing that came to mind where user_ids - and that is being taken care of by using the session variable of the user logged in.

iljoja

There are plenty of use cases where this is a security issue. In fact any case where you shared a table with different parties:
- Tenant/Company seperation
- Department seperation
- User seperation
- Site seperation

I think Allan got my point and I hope that he might find a way to make the editing secure by default

I'm willing to help if needed! Just point me to the right direction.