The callback function apachesolr_autocomplete_callback does not sanitize the $_GET query. This can have serious security implications since the callback apachesolr_autocomplete is open to public.

Proposed solution: call check_plain (line 93 of apachesolr_autocomplete.module) over the $_GET, making it

$keys = check_plain($_GET['query']);

Sorry for not submitting a patch directly.

Ratul.

Comments

Ratul Saha’s picture

Ratul Saha CreditAttribution: Ratul Saha commented
Issue summary: View changes
greggles’s picture

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

Are you worried about cross site scripting or sql injection?

drupal_json_output will filter for xss issues and is the only way data is output by this function.

I don't see any queries directly, but those values should be escaped at the place of the query and check_plain wouldn't fix that anyway.

Ratul Saha’s picture

Ratul Saha CreditAttribution: Ratul Saha commented

It gets passed through apachesolr_autocomplete_suggest and then apachesolr_drupal_query and then various other solr related functions.

I am not sure, but isn't this vulnerable to pass $_GET params to other modules' functions directly?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Title: Possible security bug in sanitizing the query » Possible security bug in sanitizing the querystring parameter

Not at all. Take a look at Why does Drupal filter on output? for an explanation. The proposed change you've made will filter the input for use in an HTML environment but it doesn't add any security related to sql injection or for use in an XML request. Any function that interfaces with a database or xml interface or...something else should do context-appropriate escaping at the moment that it builds up the request. Only when you know the context can you know what filtering should be done.

By the way, if this were a security issue it should be reported at https://www.drupal.org/security-team/report-issue

Updating the title to clarify that this is about a querystring parameter, not about an sql query. In my opinion this deserves some level of research from someone familiar with solr, but it can probably closed as "works as designed."

Ratul Saha’s picture

Ratul Saha CreditAttribution: Ratul Saha commented

I totally agree with the point of context-specific filtering. I don't know much about how the solr module handles the input. Now that I dig more, I can see that in some of cases, the $keys has been sanitized when directly passing to database call in the same file (thus validating sanitize by context).

It is entirely possible that the solr module does sanitize any and all incoming $query and thus it is not needed to sanitize beforehand. It'd be great if anyone knowing the drupal solr integration confirms this and closes this issue.

janusman’s picture

janusman CreditAttribution: janusman at Acquia commented
Status: Active » Closed (works as designed)

At least in apachesolr_autocomplete.module, things are sanitized well enough. Closing.