Breaking change in PR #37432

[jonneroelofs]

• Laravel Version: v8.44.0
• PHP Version: 8.0
• Database Driver & Version:

Description:

I think PR #37432 introduced a breaking change.

Before the PR you could create a signed url like:
https://foo.bar.net/success/5e1564b9-26ce-4a49-95e2-43644e1d7d5f?transactionid=5e1564b9-26ce-4a49-95e2-43644e1d7d5f

And then you could go to that url with the query param duplicated (note transactionid is in there twice):
https://foo.bar.net/success/5e1564b9-26ce-4a49-95e2-43644e1d7d5f?transactionid=5e1564b9-26ce-4a49-95e2-43644e1d7d5f&transactionid=5e1564b9-26ce-4a49-95e2-43644e1d7d5f

And this would pass the hasCorrectSignature method. Now it has become invalid.

I know it seems like a stupid scenario but we actually have a payment provider that adds the transactionid to the query string whether you want them to or not.

Steps To Reproduce:

Create a signed Url with a query param.
Then go to that signed url and manually duplicated your query param.

[driesvints]

But that url doesn't matches the signed url. It totally defeats the purpose of a signed url and is exposed to security vulnerabilities. Surely you can't expect that to ever have been the expected scenario?

[driesvints]

/cc @jelib3an @Krisell

[driesvints]

What exactly is keeping you from appending the query param before signing the url?

[Krisell]

We definitely didn't consider that use case, and we could perhaps add logic to detect such duplicated parameters, but one way to look at it is that this is precisely what a signed URL should detect and reject. Any deviation from verifying the actual raw data might introduce a vulnerability.

For instance, perhaps the order of the parameters matter in some weird implementation, and then being able to append a duplicated parameter should be prevented.

[jonneroelofs]

I completely agree it is a very hacky solution. However it was supported and now it is not, so I figured it might be good to mention it so you could make an informed decision whether it the pr should not be put into Laravel 9x instead of 8x. For the record I agree that it would be better if future versions of Laravel would not support this :).

I am not so sure about the security implications actually. The project has testing that ensures that changing the query param to anything else still failed validation.

The query string is already in the signed url. The problem is that the payment provider adds the exact same query param once more. So I could add it once more , but then they would do the same. So there would always be 1 (duplicate query param to many).

I guess I could probably strip they unneeded extra query param before it gets picked up by the signed middleware.

[Krisell]

Could you not generate a signed URL for the double parameter, and then remove one occurrence before giving them then URL? They will then append back the extra parameter and make the signature valid again.

[jonneroelofs]

Could you not generate a signed URL for the double parameter, and then remove one occurrence before giving them then URL? They will then append back the extra parameter and make the signature valid again.

Thanks that is probably the cleanest solution!

[Krisell]

Thanks that is probably the cleanest solution!

I realized now that that may actually not work, since the signer probably ignores the double parameter. You could of course still generate the valid hmac yourself, if giving them the URL is a manual process, like a webhook URL.

[jonneroelofs]

Ah no, you actually can't because you pass an assoicative array to the signed url, so you cannot have the same key twice .
Well I'll figure something out sure enough.

[Krisell]

Ah no, you actually can't because you pass an assoicative array to the signed url, so you cannot have the same key twice .
Well I'll figure something out sure enough.

If it is indeed a manual process, feel free to send me an email of help with generating a valid hmac (or you can see in the verification code how that is done).

Edit: And of course the hmac can be calculated automatically as well, it is currently as simple as this:
$signature = hash_hmac('sha256', 'full-url-without-signature-parameter', config('app.key'))

And then append the query parameter $signature.

[driesvints]

Since we only gotten one report so far and there's a workaround, I'm going to close this. We could maybe revisit if lots of other people run into this but for now I think the PR was valid. Thanks for reporting though.

[sublime392]

In case somebody else finds this while searching for a solution (like I did)...
I use a signed URL for a service that also adds additional parameters. I know they get added, and I don't care because it only matters that MY parameters are correct. I used to do this by duplicating a request with just the parameters that were/are part of the signature:

$vReq = $request->duplicate($request->only('signature', 'expires', 'user'));
if (!$vReq->hasValidSignature()) ...

This no longer works after this update. Now, the solution looks like this:

$queryString = sprintf(
        'signature=%s&expires=%s&user=%s',
        $request->input('signature'),
        $request->input('expires'),
        $request->input('user')
      );
      $server = $request->server();
      $server['QUERY_STRING'] = $queryString;
      $vReq = $request->duplicate(null, null, null, null, null, $server);
      if (!$vReq->hasValidSignature()) ...

[Krisell]

@sublime392 Support for that use case has been added to 9.x
https://laravel.com/docs/master/urls#validating-signed-route-requests

Perhaps you could utilize that update locally until then, here's a relevant commit (more details around this commit):
ce27f30