Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Improve signed url signature verification #37432

Merged
merged 1 commit into from
May 20, 2021
Merged

Conversation

jelib3an
Copy link
Contributor

This was done in collaboration with @Krisell

This fixes an issue where a signed url that contains a querystring parameter with no value or empty string value will always be invalid. Additionally, a valid signed url will continue to be valid even after tampering with the url by appending empty querystring values.

This is because the validation code attempts to rebuild the original url for comparison using a combination of the normalized $request->query() array and Arr::query(). $request-query() is not able to differentiate between parameters with no value or empty string values. They all return as null. Furthermore, Arr::query() will strip away any nulls. The rebuilt url will not be the same as the original url and therefore fails validation.

Since the signature was created with the original url, we can simply get the original url by stripping the signature parameter (using regex) from the raw querystring.

The following were some of our considerations on the fix:

  • Ensure that parameters and values containing the string signature will not break our regex
  • Verified that it is safe to use $request->server->get('QUERY_STRING'). According to http://www.faqs.org/rfcs/rfc3875.html (Section 4.1.7), QUERY_STRING variable must always be provided by the server and we can expect it will always exist.

The signing code was unaffected and therefore this fix is backwards compatible with any already generated signed urls. We also added tests to:

  • Verify our regex pattern for detecting signature works in certain edge cases
  • Querystring parameters with null or empty string values are parsed correctly

@GrahamCampbell GrahamCampbell changed the title Improve signed url signature verification, fixes #37370 [8.x] Improve signed url signature verification, fixes #37370 May 20, 2021
@driesvints driesvints changed the title [8.x] Improve signed url signature verification, fixes #37370 [8.x] Improve signed url signature verification May 20, 2021
@Krisell
Copy link
Contributor

Krisell commented May 20, 2021

Thanks for the collaboration @jelib3an !

From a security perspective, it is definitely much better to base the signature verification on the raw URL string (as with this PR), instead of trying to rebuild it (as before) and thereby adding the chance of not considering all data (which was indeed the case and which is a security issue with the current implementation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants