Problem/Motivation

A security review is appropriate prior to bumping to 7.x-1.0 and falling under the security advisory policy.

Proposed resolution

One or more security-focused reviews by someone other than the author.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#6 security_review_for-2849322-6.patch3.27 KBbighappyface

Comments

bighappyface created an issue. See original summary.

WidgetsBurritos’s picture

WidgetsBurritos commented

http://cgit.drupalcode.org/resource_hints/tree/resource_hints.module?id=...

Should we make sure $value doesn't have an unsanitized '>' here, and anywhere else used like this?

In its current implementation, it's probably not a security issue, but it may protect against weird stuff happening in alter hooks if you decide to support that in the future.

WidgetsBurritos’s picture

WidgetsBurritos commented

Just tested it, and at the very least, I managed to botch my headers pretty good:
Link:<//www.google.com>;x;y;z;>; rel="dns-prefetch", <//www.google.com>;x;y;z;>; rel="preconnect", <//www.google.com>;x;y;z;>; rel="prefetch", <//www.google.com>;x;y;z;>; rel="prerender"

bighappyface’s picture

bighappyface commented

Perhaps just running all line items through valid_url to validate the input?

https://api.drupal.org/api/drupal/includes%21common.inc/function/valid_u...

php > echo valid_url('//www.rackspace.com/hello') ? 'valid' : 'invalid';
valid
php > echo valid_url('//www.rackspace.com/hello>') ? 'valid' : 'invalid';
invalid
php > echo valid_url('https://www.rackspace.com/hello') ? 'valid' : 'invalid';
valid

It looks like it will work for our needs.

WidgetsBurritos’s picture

WidgetsBurritos commented

+1

bighappyface’s picture

bighappyface commented
StatusFileSize
new security_review_for-2849322-6.patch3.27 KB
WidgetsBurritos’s picture

WidgetsBurritos commented
Status: Active » Needs review
WidgetsBurritos’s picture

WidgetsBurritos commented
Status: Needs review » Reviewed & tested by the community

+1

  • bighappyface committed 0066455 on 7.x-1.x 
    Issue #2849322 by bighappyface, WidgetsBurritos: Security review for 7.x...
bighappyface’s picture

bighappyface commented
Status: Reviewed & tested by the community » Fixed
bighappyface’s picture

bighappyface commented
Status: Fixed » Closed (fixed)