An end user can trigger a Warning by passing an array rather than simple string as the q param in the URL . e.g. ?q[]=x

This seems not to affect Drupal 8, but only Drupal 7 and below.

A simple fix is to either cast $_GET['q'] to a string, or set it to the empty string if it's not a string.

CommentFileSizeAuthor
#6 1576300-6.patch412 bytesalbert volkman
#1 1576300-1.patch533 bytespwolanin

Comments

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 1576300-1.patch533 bytes

Here's a simple fix to ignore the 'q' param if it's not a string.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Given the history of problems with Drupal responding to URLs it shouldn't...perhaps the answer should be to 404 in this case?

pwolanin’s picture

pwolanin commented

Well, I can put in any number of query params that Drupal ignores, so I think ignoring it is a reasonable reaction here. Short of casting to the string 'Array' I don't see any easy way to throw a 404.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Reviewed & tested by the community

Fair point. Thanks.

David_Rothstein’s picture

David_Rothstein commented
Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D6

Thanks! Committed to 7.x: http://drupalcode.org/project/drupal.git/commit/da11da0

This could potentially be backported to Drupal 6.

albert volkman’s picture

albert volkman commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1576300-6.patch412 bytes

Not sure if this is the proper place to test this?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Reviewed & tested by the community

I don't think it needs tests.

albert volkman’s picture

albert volkman commented

Eh, I meant test as in testing the value with the in_string() method.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 1576300-6.patch, failed testing.

Status: Needs work » Needs review

greggles queued 6: 1576300-6.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: 1576300-6.patch, failed testing.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Project: Drupal core » Drupal 6 Long Term Support
Version: 6.x-dev »
Component: base system » Code
Issue summary: View changes
Status: Closed (outdated) » Needs review

This issue is apparently still reported by security scanners as CVE-2012-2922, so it probably makes sense to address. That said, this is a pretty minor issue.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Reviewed & tested by the community

This patch is clearly safe / without side effects.

Plus it is growing a little 'less minor', because a request with ?q[]=x will now cause a Fatal error: Uncaught TypeError: trim(): Argument #1 ($string) must be of type string, array given in ... on PHP8 with unpatched code.

izmeez’s picture

izmeez commented

We have used the patch in #6 on sites for a long time now.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Reviewed & tested by the community » Fixed

Thanks!

Committed:

https://github.com/d6lts/drupal/commit/93769a5ec6f9f2f4e958ee755f5f8a58e...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.