On a page with 90 comments, 437 times to be precise, with time spent in self at 20658 (total 50,000) on one request according to cachegrind - putting it into our top 20 expensive functions.

Damien found the bad call to check_url() and dereine patched it at #272854: Allowing '#' as a link with the l() function, but I'm splitting it out to here since it deserves it's own issue and a backport.

Quick benchmarks on a taxonomy term page with 30 nodes.

HEAD:
6.38 reqs/sec

Patch:
6.44 reqs/sec

CommentFileSizeAuthor
#32 check_url_D6.patch822 bytessmk-ka
#26 check_url_7-2.patch2.05 KBmfer
#23 check_url.patch2.05 KBcatch
#19 check_url.patch11.35 KBcatch
#17 check_url.patch11.3 KBcatch
#13 check_url.patch13.84 KBcatch
#11 check_url.patch11.44 KBcatch
#9 build_xss.php_.txt638 bytesneclimdul
#6 check_url.patch3.3 KBcatch
#3 filter_HEAD.png88.7 KBcatch
#3 filter_patched.png83.39 KBcatch
check_url.patch806 bytescatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
filter_patched.png83.39 KB
filter_HEAD.png88.7 KB

I should mention that with the patch, the function calls on that same page go down to 2. Here's kcachegrind screenshots.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Normal » Critical
Status: Active » Needs review

Proper status.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +Needs tests

We don't have proper tests for l() and url() yet, unless I've missed them. Could do with some reviews on the logic though as well.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
check_url.patch3.3 KB

This would've been a security hole when $options['external'] was set. But here's a slightly modified version with some initial tests for l() - which as far as I can see looks quite solid. Could do with some serious review from someone with better XSS chops than me.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

interest

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

Discussed this with chx and lyricnz in #drupal - I think we should centralise all the XSS examples from http://ha.ckers.org/xss.html in either common.test or drupalWebTestCase so we can do foreach ($this->xss_examples as $xss) wherever we need to do unit tests with functions which are supposed to filter XSS.

That should reduce a lot of duplication. Will try to get a patch started later tonight, but of course anyone else is welcome to.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs work » Needs review
FileSize
build_xss.php_.txt638 bytes

Threw together a quick script that builds a test case that contains an array of all the test cases using the xml version of the ha.ckers.org list. It is fairly simple and expects the xml to be downloaded into the same directory the script is in and outputs the code to stdout not a file.

Its not really tested much past "makes valid php" but it should be a useful starting point.

Dries’s picture

Dries CreditAttribution: Dries commented

Great catch, catch. This looks fine to me.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
check_url.patch11.44 KB

Here it is with a better test using neclimdul's script. Both HEAD and the patch pass all tests. Adds CommonXSSTestCase() so we can use it for other functions.

$options['external'] isn't a valid option for l() - although it's used in core twice (powered by Drupal and a link to OpenID) which probably needs tidying up, so this is the same patch as #1 + the tests.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Needs work

Patch no longer applies.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
check_url.patch13.84 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
check_url.patch11.3 KB

grrr.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
check_url.patch11.35 KB

Discussed this with neclimidul in IRC - since some of these attacks assume they'll be partially filtered, just checking for the same string might not be sufficient. So instead I've modified the test so that instead we confirm that the output of check_url(url($path)) is found inside the output of l(). This pretty much ensures that the output from l() is identical to what we'd get in the existing code - and means that since the test still passes we really, really don't need the extra call.

Dries’s picture

Dries CreditAttribution: Dries commented

It seems to me that the new test is a bit brain dead. Even when the escaping is broken, they would return the same result.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Well the idea there was to confirm that whether we wrap the url() call in check_url() or not, we get the same result - confirming that the call is redundant. I've got my eye on some drupal_attributes() calls as well since we do that even if we've only added 'active' as a class - and again it'd be good to have a sanity check just to make sure there's no over-optimisation there.

We obviously need full unit tests for check_plain() check_url(), filter_xss_admin() etc. to ensure they actually escape properly - but while I'm hoping the examples in this one will be useful for those tests, I don't think they're needed for this patch.

Dries’s picture

Dries CreditAttribution: Dries commented

If the only goal is to validate whether we wrap the url() call in check_url(), we don't need to run all of the XSS tests. It would suffice to run one -- not?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
check_url.patch2.05 KB

Re-rolled with only one example. Will switch the XSSTestCase into a new issue so we can add it along with proper tests.

mfer’s picture

mfer CreditAttribution: mfer commented

If someone doesn't beat me to it I'll review this on Thursday.

mfer’s picture

mfer CreditAttribution: mfer commented
Status: Needs review » Reviewed & tested by the community
FileSize
check_url_7-2.patch2.05 KB

Updated to follow head. Exact same patch as #23 just different offsets. Ran tests and manually tested change. Looks good.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

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

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)

Marking to be ported to 6.x

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Patch (to be ported) » Needs work

This change caused an issue in HEAD, since filter_xss_bad_protocol was also check_plaining its argument. By implementing this, check_plain is no longer called, and the output can contain invalid markup (see #399488: Invalid markup generated by l().).

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs work » Patch (to be ported)

Sorry, I shouldn't have changed the status since there is no patch yet.

smk-ka’s picture

smk-ka CreditAttribution: smk-ka commented
Status: Patch (to be ported) » Needs review
Issue tags: -Needs tests
FileSize
check_url_D6.patch822 bytes

D6 backport combines this patch with #399488: Invalid markup generated by l()..

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented
Priority: Critical » Normal
Status: Needs review » Postponed (maintainer needs more info)

Has this been fixed in the latest D6?

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented
Priority: Normal » Major
Status: Postponed (maintainer needs more info) » Needs work
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs work » Needs review

Don't know why #354812-32: filter_xss_bad_protocol is called hundreds of times on some pages. can't be reviewed. patch visually looks correct and applies.

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented

Sometimes the testbot ignores the patch if the filename is non-standard.

[description]-[issue-number]-[comment-number].patch
http://drupal.org/node/707484

You will need to resubmit the patch and change the status to needs review.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

@dpearceMN I'm pretty sure there are no d6 tests for core so testbot won't run on it.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs review » Needs work

I think this optimization breaks the current behaviour of D6 in cases like this:
l('foo', 'baz://foo.bar', array('external' => TRUE))

According to #354812-11: filter_xss_bad_protocol is called hundreds of times on some pages., $options['external'] is not a valid option for l(), but according to the documentation of l() in D6 the function accepts all options supported by url().

So even though I think this is a good optimization for D7, I'm afraid we cannot use the same approach in D6 without breaking the existing behaviour in a way that may have some security impact.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Closed (won't fix)

I'm going to guess this is a won't fix for D6, since D6 commits focus on security fixes at this point.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Version: 6.x-dev » 7.x-dev
Status: Closed (won't fix) » Closed (fixed)

Changing issue status to reflect that it was fixed in 7.x.