Modifications for protection against relay spam exploits

Looks fine to me - my only concern was case sensitivity ("To:" vs. "to:"), but that's handled nicely by the stristr call.

hmm why cant we just use the captcha module?

Patch still applies, I think it is needed. it looks ok, but I did not test it.

marked http://drupal.org/node/32107 a duplicate of this. Settign to critical.

Code does not look good, IMO. Please review more carefully.

Code does not escape data properly (vulnerable to XSS attacks): use theme('placeholder') to escape data shown to the user. This also makes error messages themable and thus more consistent.

The form's error handling should use form_set_error() rather than drupal_set_message() + drupal_goto().

The contact_check_exploit() function should not take an array; it should check individual fields so it is easy to reuse that function. It makes too many assumptions now.

The contact_check_exploit() function needs PHPdoc to describe the exploit. The function itself could do with a more descriptive name. 'check exploit' is like 'handle data'; non-descriptive.

Hmm, I'm no hacker but I tried putting something like "blah%0a%0dBcc: myemail@address.com" into the subject field and was not able to do the e-mail injection. Perhaps something in the Forms API automtically converts these characters over? Or maybe I'm not doing email injection properly?

First, this patch now needs a reroll .

Second. If the problem is with line feeds, then instead of the complicated check, blow away \r and \n in all header fields and be done. str_replace(array('\n', '\r'), array(), $header)

In an effort to learn more about this attack, I wrote a Perl script to inject a \nBcc: into the subject. It worked. However, the mime_header_encode function effectively blocked this exploit on my server by converting the subject line to UTF-8. See the user_mail function.

But other drupal installations that have the smtp_library installed will bypass the mime_header_encode function. However, I can't find anywhere where the smtp_library variable is set. Is the code at the top of user_mail used?

OK, after looking at this problem, here's what I've concluded. Please let me know if you disagree:

1) Malicious user can only inject malicious code through the subject field in the contact form.
2) Any new lines in the subject field will get stripped out via the mime_header_encode function for many Drupal users.
3) Other Drupal users who have their own smtp_library are still vulnerable.
4) To solve the problem for folks with their own smtp_library, a simple one liner to strip the subject of newlines will do. Maybe a watchdog entry would be desirable, also.

Great work! Steve, congratulations for the deep analysis. Could you maybe provide a patch? :)

OK, patch attached.

My (custom) contact form just got hit by a spammer using this exploit yesterday. I applied Steve's changes on my site but I'm not certain they did the trick. After I reenabled the module I got another email from the spammer. I'm now using a rewriterule to block him so I can't really test further.

In any case, let's make sure this fix lands by 4.7.

tangent,

The patch doesn't stop spam to you. It stops your site from spammers who will try to spam others through your site.

I knew that but I guess I didn't think it through. Sorry for the false alarm.

Is that rewrite rule worth adding to our .htaccess file?

That is probably debatable. I have a rule with a bunch of conditions that block certain user-agent strings.

  # Block unwanted user-agents (bots, screen scrapers, dubious visitors)
  RewriteCond %{HTTP_USER_AGENT} ^$                     [OR]
  RewriteCond %{HTTP_USER_AGENT} ^BlackWidow            [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Bot\ mailto:craftbot@yahoo.com [OR]
  RewriteCond %{HTTP_USER_AGENT} ^ChinaClaw             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Crescent              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^CherryPicker          [OR]
  RewriteCond %{HTTP_USER_AGENT} ^CherryPickerSE        [OR]
  RewriteCond %{HTTP_USER_AGENT} ^CherryPickerElite     [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Comaneci              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^DISCo                 [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Download\ Demon       [OR]
  RewriteCond %{HTTP_USER_AGENT} ^eCatch                [OR]
  RewriteCond %{HTTP_USER_AGENT} ^EirGrabber            [OR]
  RewriteCond %{HTTP_USER_AGENT} ^EmailCollector        [OR]
  RewriteCond %{HTTP_USER_AGENT} ^EmailSiphon           [OR]
  RewriteCond %{HTTP_USER_AGENT} ^EmailWolf             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Express\ WebPictures  [OR]
  RewriteCond %{HTTP_USER_AGENT} ^ExtractorPro          [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Exabot                [OR]
  RewriteCond %{HTTP_USER_AGENT} ^EyeNetIE              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^FlashGet              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^GetRight              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Go!Zilla              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Go-Ahead-Got-It       [OR]
  RewriteCond %{HTTP_USER_AGENT} ^GrabNet               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Grafula               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^HMView                [OR]
  RewriteCond %{HTTP_USER_AGENT} ^HTTrack               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^ia_archive            [OR]
  RewriteCond %{HTTP_USER_AGENT} ^ia_archiver           [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Image\ Stripper       [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Image\ Sucker         [OR]
  RewriteCond %{HTTP_USER_AGENT} ^InterGET              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Internet\ Ninja       [OR]
  RewriteCond %{HTTP_USER_AGENT} ^JetCar                [OR]
  RewriteCond %{HTTP_USER_AGENT} ^JOC\ Web\ Spider      [OR]
  RewriteCond %{HTTP_USER_AGENT} ^larbin                [OR]
  RewriteCond %{HTTP_USER_AGENT} ^LeechFTP              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Mass\ Downloader      [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Microsoft.URL         [OR]
  RewriteCond %{HTTP_USER_AGENT} ^MIDown\ tool          [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Mister\ PiX           [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Mozilla.*NEWT         [OR]
  RewriteCond %{HTTP_USER_AGENT} ^MSFrontPage           [OR]
  RewriteCond %{HTTP_USER_AGENT} ^NaverBot              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Navroad               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^NearSite              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^NetAnts               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^NetSpider             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Net\ Vampire          [OR]
  RewriteCond %{HTTP_USER_AGENT} ^NetZIP                [OR]
  RewriteCond %{HTTP_USER_AGENT} ^NICErsPRO             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Octopus               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Offline\ Explorer     [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Offline\ Navigator    [OR]
  RewriteCond %{HTTP_USER_AGENT} ^PageGrabber           [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Papa\ Foto            [OR]
  RewriteCond %{HTTP_USER_AGENT} ^pcBrowser             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^RealDownload          [OR]
  RewriteCond %{HTTP_USER_AGENT} ^ReGet                 [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Siphon                [OR]
  RewriteCond %{HTTP_USER_AGENT} ^SiteSnagger           [OR]
  RewriteCond %{HTTP_USER_AGENT} ^SmartDownload         [OR]
  RewriteCond %{HTTP_USER_AGENT} ^SuperBot              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^SuperHTTP             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Surfbot               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^tAkeOut               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Teleport\ Pro         [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Telesoft              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^TV33_Mercator         [OR]
  RewriteCond %{HTTP_USER_AGENT} ^VoidEYE               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebAuto               [OR]
  RewriteCond %{HTTP_USER_AGENT} ^[Ww]eb[Bb]andit       [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebCopier             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebEMailExtrac.*      [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebFetch              [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Web\ Image\ Collector [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Web\ Sucker           [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebReaper             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebSauger             [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Website\ eXtractor    [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebStripper           [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebWhacker            [OR]
  RewriteCond %{HTTP_USER_AGENT} ^WebZIP                [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Wget                  [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Widow                 [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Xaldon\ WebSpider     [OR]
  RewriteCond %{HTTP_USER_AGENT} ^Zeus 
  RewriteRule ^.*$ - [F,L]

This spammer was using a blank user-agent string so I had to add the first condition. This rule is likely one that all sites need but it could cause problems for some if enabled by default.

Dries - no, it wouldn't be. For one, those sorts of lists need constant maintenance, and are out of date quite frequently. For another, it gives the illusion that Drupal is "complete" in its bot blockage, when that will never be the case: many many bots fake the referrers of legitimate UAs or, for that matter, don't send a referer at all.

Patch applies, needed and solves the problem. See #9-#13 for a good review of the problem and the solution.

Oh, I expected a one or two line patch against .htaccess. Clearly this is not an option.

As for the patch, should we only check the subject? How does this compare with known injection preventions mechanisms?

Apologies to Steve and everyone for dragging this out.

After taking another look at this I decided that it would be far more elegant to use the new formsapi to validate the input instead of handling this in the submit. That way, users who somehow add a linebreak in the subject (possible with copy/paste in some user agents) would get a nice warning. Even better though, any offending messages would be completely blocked instead of sent through to the contact with a watchdog warning.

I've created a new patch following adrian's recent commit.

I moved the 'mail' check outside of the category check in contact_mail_page_validate() because it seemed an unnecessary dependency but I might have been in error. I've tested this patch with both /contact and /user/1/contact though and it seems fine. Please test this at your earliest convenience.

On a sidenote, I also discovered that the email validation regexpr accepts addresses of the form x@y which seems like a bad address to me (unless you are only using drupal on an intranet or single system perhaps).

If anyone has the ability to test this form by posting actual offending values that is what is really needed. I don't have the capability to do so very easily.

Ok. I did a real test of an injection attack against the form with my latest patch and it was successful.

@Dries: Here is a decent explanation of the issue and how to work around it. http://securephp.damonkohler.com/index.php/Email_Injection. The synopsis is that detecting and preventing the exploit involves removing linebreak characters ("\r", "\n") from any input fields that could be placed before the message body.

I tested the form using wget by emulating the same headers sent by Firefox and sending an appropriate POST. Here is what I used (censored) if anyone wants to confirm.

$ wget --post-file=post.txt --user-agent="Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1" --header="Cookie: PHPSESSID=3584a73009b3eb9abd64cb461f412b27; cprelogin=no" --header="Referer: http://www.example.com/contact" --header="Content-Type: application/x-www-form-urlencoded" --output-document=post_output.txt http://www.example.com/contact

post.txt contains

edit%5Bname%5D=tangent&edit%5Bmail%5D=tangent%40example.com&edit%5Bsubject%5D=Foo%0ABcc%3Aspamvictim%40example.com&edit%5Bmessage%5D=test&op=Send+e-mail&edit%5Bform_token%5D=67596c90e75690907eb8c4f539e8a7d3&edit%5Bform_id%5D=contact_mail_page

You would need to change all the cookie header, urls and emails (obviously) but also the form_token value I suspect.

Attempting to insert linebreak characters into the browser form was unsuccessful because Firefox and IE encode them and they are sent through in the message subject.

Finally, I have changed the test in the validation functions to use regexpr instead of string manipulation because it should be better.

This should be my final input on the issue. This patch final patch works exactly as I would like. Please review and set back to ready to commit asap.

If you use regexps, then at least please use this. even one ereg is slower than preg and two eregs are definitely slower than one preg :)

This has been fixed in CVS head.

i can't find any checks for newlines in the subject in current head, so i suppose users with a custom smtp_library are still be vulnerable -> setting back to active.

Oops, this patch didn't make it in. It needs to be re-rolled though. Should be easy.

Contact.module uses user_mail() to send the email, which passes the subject through mime_header_encode(). That function should use "=?UTF-8.." style escapes for anything which contains non-printable ascii characters:

function mime_header_encode($string) {
  if (!preg_match('/^[\x20-\x7E]*$/', $string)) {

So where is the problem? It's in fact rather nasty. That regular expression in mime_header_encode() gets confused around linebreaks, because of the ^ and $ operators, which also match at the beginning and end of a newline.

Patch attached which fixes this. Newlines are no longer a problem, they simply get encoded along with any other non-printables.

function mime_header_encode($string) {
  if (preg_match('/[^\x20-\x7E]/', $string)) {

I'd much rather prefer this patch.

In addition to this, chx has provided a patch that strips \r and \n instances from textfields.

-K

This looks like the right fix. Great job, Steven. I patched drupal.org with this patch so we can some testing. Ideally, we'd also have a test script that attempts to inject linebreaks.

both recommended patches were committed.