The following code:
tab_focus = $(window.location.hash, this).closest('.horizontaltabspane');
gets picked up by security scanners because it passes "window.location.hash" directly to JQuery. This is mitigated by the check above it which is why I'm making this an open ticket and not going through the security team process:
if (hash !== '#' && $(hash, this).length
which should require that the "window.location.hash" is actually present in the form before processing. However it would be better to escape the has before sending directly to JQuery.
We can use the variable already created:
var hash = window.location.hash.replace(/[=%;,\/]/g, "");
for this purpose. The following patch replaces:
tab_focus = $(window.location.hash, this).closest('.horizontaltabspane');
with:
tab_focus = $(hash, this).closest('.horizontaltabspane');
so that the input to the jQuery call is sanitized.
Comment | File | Size | Author |
---|---|---|---|
hash-location-sanitization.diff | 699 bytes | acouch |
Comments
Comment #2
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis was unpublished briefly while it was brought to the attention of the Drupal Security Team. However, I'm republishing it now because our review found that there is no vulnerability.
The main mitigation of this actually comes from Drupal core, which added protection for this kind of problem a while ago via https://www.drupal.org/SA-CORE-2013-001. (Without that protection, the module would be vulnerable.) It is possible that switching to the "pre-sanitized"
hash
variable is sufficient to deal with that, but in general my understanding is that the best way to avoid these types of vulnerabilities (and, likely, to avoid false positives on security scans) is to switch to something like$(this).find(window.location.hash)
rather than$(window.location.hash, this)
.Comment #3
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #4
nils.destoop CreditAttribution: nils.destoop as a volunteer and at Wunder commentedThx for the patch. This was committed to the dev branch