Note: Since this generally seems to be at worse a DoS vector, cleared by the security team to be public/normal bug fix
A security audit of a customer site found that for Drupal sites integrated with a Solr back-end that supports EDismax as the default query parser (generally Solr 3.x or 4.x, or 1.4.x if patched and custom-built), the user could enter "local" parameters in the search box and these override the ones passed to Solr by the module.
A simple example of why that could be a problem is a DoS attack that lets the user increase the number of results returned from 10 to 10000 causing either Solr or the webserver to have performance issues, and possibly making PHP go OOM.
This issue likely also affects search_api_solr module.
Speaking to the Solr maintainers, this is not really an advertised feature of the EDismax query parser (though looking at the code it may be an expected feature from the way the code works), which is suggested as suitable for direct user input. However, the docs do seem to suggest it's expected: http://wiki.apache.org/solr/ExtendedDisMax#Parameters
An example mitigation would be this change:
diff --git a/apachesolr_search.module b/apachesolr_search.module
index 51dc6d4..7003cd8 100644
--- a/apachesolr_search.module
+++ b/apachesolr_search.module
@@ -125,6 +125,13 @@ function apachesolr_search_search($op = 'search', $keys = NULL) {
$filters = isset($_GET['filters']) ? $_GET['filters'] : '';
$solrsort = isset($_GET['solrsort']) ? $_GET['solrsort'] : '';
$page = isset($_GET['page']) ? $_GET['page'] : 0;
+
+ // Don't allow local params to pass through to EDismax from the url.
+ if ($keys) {
+ $keys = preg_replace('/{![^}]*}/',' ', $keys);
+ // For good measure - disarm any that we missed in the regex.
+ $keys = str_replace('{!',' ', $keys);
+ }
try {
$results = apachesolr_search_execute($keys, $filters, $solrsort, 'search/' . arg(1), $page);
return $results;
Comment | File | Size | Author |
---|---|---|---|
#14 | 1966044-10-D6x-1x.patch | 866 bytes | pwolanin |
#11 | 1966044-11.patch | 906 bytes | pwolanin |
#9 | 1966044-9-D6x-1x.patch | 749 bytes | pwolanin |
#5 | 1966044-5-D6.patch | 739 bytes | pwolanin |
#3 | 1966044-3.patch | 735 bytes | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedan alternate fix that avoids touching user input:
Comment #2
pwolanin CreditAttribution: pwolanin commentedHere's one that actually works with the spellcheck.
However, I'm not sure I like the added complexity here - maybe worse DX too.
Comment #3
pwolanin CreditAttribution: pwolanin commentedHere's a simplified version of the one above for 7.x
Comment #4
pwolanin CreditAttribution: pwolanin commentedComment #5
pwolanin CreditAttribution: pwolanin commentedComment #7
pwolanin CreditAttribution: pwolanin commented#5: 1966044-5-D6.patch queued for re-testing.
Comment #8
pwolanin CreditAttribution: pwolanin commentedcommitted. needs 6.x-1.x
Comment #9
pwolanin CreditAttribution: pwolanin commentedComment #10
pwolanin CreditAttribution: pwolanin commentedlooks like a leading {! causes a parse error - maybe should go back to the extra stomp in the original post
Comment #11
pwolanin CreditAttribution: pwolanin commentedComment #12
pwolanin CreditAttribution: pwolanin commentedcommitted
Comment #13
pwolanin CreditAttribution: pwolanin commentedalso applied to 6.x-3.x with fix of + to *
Comment #14
pwolanin CreditAttribution: pwolanin commentedcommitted this to 6.x-1.x