+- CodeIgniter Forums (https://forum.codeigniter.com)
+-- Forum: CodeIgniter 4 (https://forum.codeigniter.com/forumdisplay.php?fid=28)
+--- Forum: CodeIgniter 4 Discussion (https://forum.codeigniter.com/forumdisplay.php?fid=31)
+--- Thread: Model::update() is dangerous (/showthread.php?tid=84833)
Model::update() is dangerous - kenjis - 11-15-2022
The current Model::update() is very dangerous.
I created a sample vulnerable app.
https://github.com/kenjis/ci4-model-update-danger
PHP Code:
public function update()
{
$id = $this->request->getPost('id');
if ($this->request->getMethod() === 'post' && $this->validate([
'title' => 'required|min_length[3]|max_length[255]',
'body' => 'required',
])) {
$title = $this->request->getPost('title');
$slug = url_title($title, '-', true);
$data = [
'title' => $title,
'slug' => $slug,
'body' => $this->request->getPost('body'),
];
$this->model->update($id, $data);
return $this->response->redirect(site_url('news/' . $slug));
}
return $this->edit($id);
}
The above code has vulnerability. Do you know what's wrong?
RE: Model::update() is dangerous - ozornick - 11-15-2022
No valid ID? If empty id update all rows?
RE: Model::update() is dangerous - iRedds - 11-15-2022
There is no vulnerability here.
This is an application design error. All input data must be validated.
RE: Model::update() is dangerous - ikesela - 11-15-2022
maybe can fix by making model update not allow null input as id. seen this complain on fb group ( id == null, will update all rows)
RE: Model::update() is dangerous - ozornick - 11-15-2022
haha, I just got this bug a couple of days ago. Gone to fix my model wrappers by checking id
RE: Model::update() is dangerous - kenjis - 11-15-2022
(11-15-2022, 06:21 AM)iRedds Wrote: There is no vulnerability here.
This is an application design error. All input data must be validated.
I mean vulnerability is a bug or unintended behavior that an attacker can abuse.
RE: Model::update() is dangerous - iRedds - 11-15-2022
And what solution do you propose?
RE: Model::update() is dangerous - kenjis - 11-15-2022
Okay, I wrote another controller. You cannot set $id to null. But it is still vulnerable.
https://github.com/kenjis/ci4-model-update-danger/blob/297f1d43ff7af5eac3a873a5a6bd0922543f7d04/app/Controllers/News2.php#L103-L123
PHP Code:
public function postUpdate($id = false)
{
if ($this->validate([
'title' => 'required|min_length[3]|max_length[255]',
'body' => 'required',
])) {
$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('news2/view/' . $slug));
}
return $this->getEdit($id);
}
RE: Model::update() is dangerous - InsiteFX - 11-15-2022
Your passing the body into the database just using required so it will pass as long as something is in it.
RE: Model::update() is dangerous - kenjis - 11-16-2022
(11-15-2022, 11:02 PM)InsiteFX Wrote: Your passing the body into the database just using required so it will pass as long as something is in it.
Okay, I added max and min length validation rules.
https://github.com/kenjis/ci4-model-update-danger/blob/8aeff23fd3e2a8be4234956da697076e8276bc2d/app/Controllers/News2.php#L16-L19
PHP Code:
private array $rules = [
'title' => 'required|min_length[3]|max_length[255]',
'body' => 'required|min_length[10]|max_length[5000]',
];