Vulnerability - Arbitrary File Download

[arall]

The following controller method is vulnerable to arbitrary file download:

public function download(Request $request, ResponseFactory $response): BinaryFileResponse
{
    $data = $this->validate($request, [
        'path'     => 'required',
        'filename' => 'required',
    ]);

    return $response->download(
        $data['path'],
        $data['filename']
    )->deleteFileAfterSend($shouldDelete = true);
}

(No authentication is needed)

Example Proof of Concept:

https://testdomain.local/nova-vendor/maatwebsite/laravel-nova-excel/download?path=/etc/passwd&filename=file.txt

I think generated files should be stored in storage/app/laravel-nova-excel (or something similar) and the controller must ensure that files are only fetched from that directory (preventing moving to parent by injecting ../ in the filename etc).

Perhaps changing the path parameter into file (and enforce previous checks) would be a solution.

Also, authentication must be required.

[patrickbrouwers]

Thanks for reporting the issue. Would be awesome if you could PR a fix for that.

(Also, in future it's better to report security issues via mail or the security tab)

[arall]

(Also, in future it's better to report security issues via mail or the security tab)

Sorry, but I don't have permissions to draft advisories and there is no security policy defined.

I might try to do a PR with the fixes later.

[patrickbrouwers]

I thought you would be able to draft an advisory, that's weird. I've copied the security policy from Laravel Excel now. Thought it was already in place.

Thanks for looking into the PR!

[arall]

I was trying to do the PR, but I think the package maatwebsite/excel is also involved (to define the path). Looking into it, I think it should use Laravel Filesystem, instead of its own implementation.

For the authentication, would be enough to add this method in the controller:

/**
 * Instantiate a new controller instance.
 *
 * @return void
 */
public function __construct()
{
    $this->middleware('auth');
}

Of course, this will force to be authenticated in Laravel Nova (I'm not sure if that will conflict any scenario).

[patrickbrouwers]

@arall maatwebsite/excel does use laravel filesystem, however I think the personal who PR'd this did it like this, so he wouldn't have to clean-up after downloading (I believe)

Forced to be authenticated would be good I think.

[danigri84]

Given the lack of updates on this critical issue, we've forked the project and deployed our solution to https://github.com/pdmfc/Laravel-Nova-Excel

[patrickbrouwers]

Fixed in next release