Is QueryBuilder's paginate() method vulnerable to SQL injection ?

@Sinnbeck They said that substituting the string 1%00%C0%A7%C0%A2%252527%252522 in perpage parameter would do this, i.e.

http://www.example.com/users/fetch?page=1&perpage=1%00%C0%A7%C0%A2%252527%252522

@jlrdw perPage is used in the query for the offset and limit 🤷‍♂️

@tykus I realize, I have a guide for lengthaware: https://laracasts.com/discuss/channels/guides/length-aware-paginator

But perpage is int. It should error out if some "bogus" SQL injection is passed instead.

@jlrdw there is an echo in here

But perpage is int. It should error out if some "bogus" SQL injection is passed instead.

the perPage value is cast as an int in the limit operation; and mathematical operation in take operation will not pass thru non-numeric values.

I know, I don't understand the reply where you told me about perpage? I tested, and sure enough passing non numeric gives:

TypeError PHP 8.1.1 9.6.0 Unsupported operand types: string * int

If anything I agree.

Edit

Sorry if I misunderstood something.

@jlrdw probably your comment

The pagination has nothing to do with a query

when clearly it does

@jlrdw If we try to substitute the string 15%00%C0%A7%C0%A2%252527%252522 in perpage parameter, it actually throws the following error:

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1

select * from users where deleted_at is null order by id desc offset 0

And the generated query seems to be missing the LIMIT clause

@Eyad Mohammed Osama I don't understand how that should fall all the way through to the database - the mathematical operation should fail in PHP.

What are your Laravel and PHP versions?

@tykus

PHP version is 8.0.17 Laravel version is 9.2

@Snapey Thanks for the reply

In fact, i have a table that views users, and there's an option to control how many users should be returned per page

I'm reading the parameters from the request and passing them to the paginate() method

Since i prefer readability and cannot memorize parameters order, i use PHP 8 named arguments to pass the arguments

@MohamedTammam What exactly do you need to know ?

@Eyad Mohammed Osama if you use the regular paginator or length aware paginator per documentation and API then:

No pagination (laravel paginate()) is not vulnerable to SQL injection.

Edit:

Why not try yourself to inject something?

@jlrdw What do you mean by regular ?

Is it the same paginator i'm using in my case ?

@Eyad Mohammed Osama

Regular = just using paginate() or simplepaginate().

Otherwise you have the lengthawarepaginator where you can customize.

All is in documentation and API.

Edit, where is

$builder->paginate(page: $page, perPage: $perpage)

in documentation? Where are you calling:

new LengthAwarePaginator

@jlrdw

It's here: https://laravel.com/docs/9.x/pagination#paginating-query-builder-results

@Eyad Mohammed Osama then this:

$users = $builder->paginate(page: $page, perPage: $perpage);

can be:

$builder->paginate($perpage);

It is safe as said by many above.

@jlrdw

Excuse me for over asking, but how it would be safe if i don't pass the page parameter ?

Even if i don't pass the page parameter, Laravel will automatically detect it and inserts it into the generated URL, so it happens implicitly

@Eyad Mohammed Osama I need to know how to duplicate that vulnerability.

@Eyad Mohammed Osama it's resolved in the paginators code, but if you feel safer pass it as well yourself.

@MohamedTammam

The report suggests that inserting 15%00%C0%A7%C0%A2%252527%252522 into perPage parameter is enough to produce the vulnerability

@Eyad Mohammed Osama I just tested locally and $perpage is just null, and causes the error you mentioned.

Try dd($perpage) and you should just see null

Adding a fallback fixes it right away :)

$perpage = $request->perpage ?? 15;

Or you can use validation

$validator = Validator::make($request->all(), [
        'perpage' => [\Illuminate\Validation\Rule::in([10,15,20])]
    ]);
    if ($validator->fails()) {
        return redirect()->back(); //replace with whatever you want
    }

Did they report it as they managed to get it to throw an error perhaps?

@Sinnbeck Thanks for your effort and time I applied your solution because the best practice is to sanitize input and make sure it's a positive integer