Here's the patch from the security team to prevent DoS on filter_url().

From linclark (Discovered by):
"Today QA created a piece of content on our D7 site in order to test text-wrapping for extremely long words. In the body, he created a since word easily in the thousands of characters (I didn't count). After saving that, the admin/content page would no longer load as it would hit the PHP max execution time limit. I changed it to 60 seconds and it was still hitting that limit.

A developer then traced the bug to _filter_url:

After a little more investigation, the _filter_url() function is where all of the time is being used... Granted, it is invalid content but still a DoS vulnerability."

Please give commit credit to chx, jwineinger, and linclark. See http://stackoverflow.com/questions/386294/maximum-length-of-a-valid-emai... for some background. Private tracker #69603

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
1.6 KB
1.08 KB

Ported and working on adding a test. Completely untested, let's see how this goes.

Anonymous’s picture

Hello,

If you want to insert long url then you can shorten them by using bitly (https://bitly.com/), It may solve the problem of long url.

Berdir’s picture

both passed? Well, that's not what should have happened... Looks like it's because I forgot to add the mailto:.

@viswanath_polaki: This is not making long mails work, this is to prevent a security issue when there is one.

Status: Needs review » Needs work

The last submitted patch, long-email-with-tests-1558468-3.patch, failed testing.

MustangGB’s picture

Shouldn't the entire length of the email address be less than or equal to 254, rather than just the part before the @ sign?

Berdir’s picture

@akamustang, Yeah, looks like it, after reading the referenced documentation. It looks like the whole thing can be up to 254 characters and the local part only 64. Interestingly, $domain is actually limited to 64.

Anyway, here are tests for the current behavior of the code.

Berdir’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good to me.

Please note the additional commit credits in the OP.

catch’s picture

Title: SA-CORE-2012-002 - Denial of Service » SA-CORE-2012-002 - Denial of Service (D7 test coverage)
Version: 8.x-dev » 7.x-dev
Category: bug » task
Priority: Critical » Major
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Novice, +Needs backport to D7

Committed/pushed to 8.x, we should backport the tests to 7.x, tagging novice for the backport.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.23 KB
1.74 KB

The first patch reverts the fix in D7 to show the tests fail, the second is the tests and the one to commit.

jthorson’s picture

Status: Needs review » Reviewed & tested by the community

Yup, that's the same test. If it was good enough for D8 (despite the double blank lines), then I'll play along and say it's good enough for D7 too.

Though I wonder if the 500+ character array key might have been overkill ... "person@example.com or mailto:person2@example.com or (254-character-username)@example.com but not (255-character-username)@example.com" may have been enough. ;)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

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

Anonymous’s picture

Issue summary: View changes

x