Welcome Guest, Not a member yet? Register   Sign In
Model::update() is dangerous
#11

(This post was last modified: 11-16-2022, 07:39 AM by ikesela.)

maybe can fix this, byass id checking if it is NULL , there should be no if($id), no checking need so WhereIn to be work
Code:
protected function doUpdate($id = null, $data = null): bool
    {
        $escape       = $this->escape;
        $this->escape = [];

        $builder = $this->builder();

        if ($id) {
            $builder = $builder->whereIn($this->table . '.' . $this->primaryKey, $id);
        }

        // Must use the set() method to ensure to set the correct escape flag
        foreach ($data as $key => $val) {
            $builder->set($key, $val, $escape[$key] ?? null);
        }

        return $builder->update();
    }
Reply
#12

There is no good solution for this for now. Any change would be a BC break.
Developers may use composite keys with the model.

PHP Code:
$model->where(['key1' => 'value1''key2' => 'value2'])->update(null$data); 

I believe this was a design choice.



Whether we like the current model behavior or not, the code presented here and in the repo lacks any basic checks in update() / postUpdate() method:
  • why is null/false acceptable as a value at all?
  • I can perform an update query on every single row in the table without checking if row exists or not.

Meanwhile, the code responsible for showing the view: getEdit() has these checks. There should be no difference when validating the $id for getEdit() and postUpdate().

I get your reasoning with update(null, $data), but the real danger here is poorly-written code, not the framework. In this case, simple value validation for $id would solve all the problems.
Reply
#13

(This post was last modified: 11-16-2022, 02:49 PM by kenjis.)

Certainly, the current API cannot be changed without a BC break. However, an API that is not well designed should be changed to something more secure.

All inputs should be validated. Yes, that is correct. But have you guys ever forgotten to validate input values?

With the current API, developers create a vulnerability that if they forget to validate $id even once, all records will be updated.

I can't think of a use case like this sample where you want to update all records at the same time when updating a single record.

UPDATE table and UPDATE table WHERE id = ? are fundamentally different use cases. There is no need to provide both in the same method.
Reply
#14

kenjis Wrote:But have you guys ever forgotten to validate input values?

Ok, this is hard to argue. This probably happened to everyone.

kenjis Wrote:I can't think of a use case like this sample where you want to update all records at the same time when updating a single record.

As I said before. We have to think of it from the point of view of the entire API that comes with the Model.
Developers may write queries that don't necessarily update all the records when the null is set, like:

PHP Code:
$model->where(...)->set(...)->update(); 

So, if we suddenly start to require an $id, this will break people's code. Not sure what is more damaging.

The only workaround I see for this is checking if the where part was set before or not. Something like:

PHP Code:
    protected function doUpdate($id null$data nullbool $allowNullId false): bool
    
{
        $escape      $this->escape;
        $this->escape = [];

        $builder $this->builder();

        if ($id === null && ! $allowNullId && empty($builder->getCompiledQBWhere())) {
            throw new DatabaseException(
                'Updates are not allowed unless they contain a "where" or "like" clause.'
            );
        }

        if ($id) {
            $builder $builder->whereIn($this->table '.' $this->primaryKey$id);
        }

        // Must use the set() method to ensure to set the correct escape flag
        foreach ($data as $key => $val) {
            $builder->set($key$val$escape[$key] ?? null);
        }

        return $builder->update();
    

It would still be a BC break, but the overall functionality would remain. Still, I'm not sure this is the best thing to do.
Reply
#15

(This post was last modified: 11-17-2022, 06:30 PM by kenjis.)

I created another controller. I added the validation for id.
But it is still vulnerable.

https://github.com/kenjis/ci4-model-upda...#L101-L123
PHP Code:
    public function update()
    {
        $id $this->request->getPost('id');

        // Adds validation rule for id.
        $rules array_merge($this->rules, ['id' => 'required|is_natural_no_zero']);

        if ($this->validate($rules)) {
            $title $this->request->getVar('title');
            $slug  url_title($title'-'true);

            $data = [
                'title' => $title,
                'slug'  => $slug,
                'body'  => $this->request->getVar('body'),
            ];
            $this->model->update($id$data);

            return $this->response->redirect(site_url('news3/' $slug));
        }

        return $this->edit($id);
    
Reply
#16

(This post was last modified: 11-17-2022, 05:31 PM by kenjis.)

(11-17-2022, 09:12 AM)michalsn Wrote: The only workaround I see for this is checking if the where part was set before or not. Something like:

PHP Code:
    protected function doUpdate($id null$data nullbool $allowNullId false): bool
    
{
        $escape      $this->escape;
        $this->escape = [];

        $builder $this->builder();

        if ($id === null && ! $allowNullId && empty($builder->getCompiledQBWhere())) {
            throw new DatabaseException(
                'Updates are not allowed unless they contain a "where" or "like" clause.'
            );
        }

        if ($id) {
            $builder $builder->whereIn($this->table '.' $this->primaryKey$id);
        }

        // Must use the set() method to ensure to set the correct escape flag
        foreach ($data as $key => $val) {
            $builder->set($key$val$escape[$key] ?? null);
        }

        return $builder->update();
    

It would still be a BC break, but the overall functionality would remain. Still, I'm not sure this is the best thing to do.

It seems no problem. It prevents unexpected all record updates.
What's your concern?
Reply
#17

(11-17-2022, 05:26 PM)kenjis Wrote: I created another controller. I added the validation for id.
But it is still vulnerable.

https://github.com/kenjis/ci4-model-upda...#L101-L123
PHP Code:
    public function update()
    {
        $id $this->request->getPost('id');

        // Adds validation rule for id.
        $rules array_merge($this->rules, ['id' => 'required|is_natural_no_zero']);

        if ($this->validate($rules)) {
            $title $this->request->getVar('title');
            $slug  url_title($title'-'true);

            $data = [
                'title' => $title,
                'slug'  => $slug,
                'body'  => $this->request->getVar('body'),
            ];
            $this->model->update($id$data);

            return $this->response->redirect(site_url('news3/' $slug));
        }

        return $this->edit($id);
    

Ok, you got me here. How this is still a vulnerability?

kenjis Wrote:It seems no problem. It prevents unexpected all record updates.
What's your concern?

Not a big fan of the API I came up with.
Reply
#18

(11-15-2022, 09:12 PM)iRedds Wrote: And what solution do you propose?

See https://github.com/codeigniter4/CodeIgniter4/pull/6883
Reply
#19

Nver ever trust user input data!

If it's a bad vulnerability then we need to fix it before it gets out of control.
What did you Try? What did you Get? What did you Expect?

Joined CodeIgniter Community 2009.  ( Skype: insitfx )
Reply
#20

I would like to change the API:
  • [change] update($id, $data) only for updating a specific id or specific ids.
  • [add] updateAll($data) for updating without ids.
Reply




Theme © iAndrew 2016 - Forum software by © MyBB