Current:
The function user_mail() does not check the passed arguments correctly to avoid email injections.

Should be:
Everything argument that becomes part of the email's header must be checked to not contain "\r" or "\n".
These are:
- $mail (TO:)
- $subject (SUBJECT:), although mime_header_encode may already accidentely do it
- $header (mostly contains FROM:)

Details:
By placing "\r" and/or "\n" into for example the from field in the feedback module, an attacker can inject any information into the email header, effectivly turning a web mail form into an open relay.

See here for a brief description of the attack and a solution: http://securephp.damonkohler.com/index.php/Email_Injection

The problem already did occur for drupal installations: http://drupal.org/node/30995

Solution, Patch to user_mail():
This is just a sketch, not a full blown path. In real live, the function check_email_header() should set a drupal error message or atl east do something more meaningfull then just jumping of the cliff, I guess.

/**
 * Send an e-mail message.
 */
function user_mail($mail, $subject, $message, $header) {
  if (variable_get('smtp_library', '') && file_exists(variable_get('smtp_library', ''))) {
    include_once variable_get('smtp_library', '');
    return user_mail_wrapper($mail, $subject, $message, $header);
  }
  else {
    return mail(
      check_email_header($mail),
      mime_header_encode(check_email_header($subject)),
      str_replace("\r", '', $message),
      "MIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8; format=flowed\nContent-transfer-encoding: 8Bit\n" . check_email_header($header)
    );
  }
}

function check_email_header($val)
{
   if (strpos($value, "\r") !== false || strpos($value, "\n") !== false)
   {
     die("Why ?? :(");
   }

   return $val;
}
CommentFileSizeAuthor
#22 mail.injection.diff.txt736 bytesbeginner
#14 teste.php.txt1002 bytesmagico
#13 user.module.patch_0.txt1.84 KBmagico

Comments

David Lesieur’s picture

David Lesieur commented
Project: Mail » Drupal core
Version: 4.6.x-1.x-dev » 4.6.3
Component: Code » user.module

If a fix is going to be applied in user_mail(), then this is a Drupal core issue. And this is an important one!

kbahey’s picture

kbahey commented

feedback module already implements a fix for this.

See here
http://drupal.org/node/29927
http://drupal.org/node/34068

I submitted a patch for contact.module
http://drupal.org/node/34002

I am not sure if your patch is the right approach because the headers can be more than one field separated by newlines. So the presence of a newline does not mean that it is a spam attempt.

For example, contact.module has:

user_mail($to, $subject, $body, "From: $from\nReply-to: $from\nX-Mailer: Drupal\nReturn-path: $from\nErrors-to: $from");

Unless we disallow headers altogether, this cannot be done the way you did it.

henk@sharewareblogs.com’s picture

henk@sharewareblogs.com commented

I can confirm that the patch doesn't work. I tried it and I'm still getting successful attacks. For now I disabled the feedback module, I will look into the other patches first.

kbahey’s picture

kbahey commented

Henk

To clarify, the latest version of feedback prevents relay spam, but will not prevent you getting email from anyone via feedback. The patch was intended for the former, not the latter.

Please open an issue against feedback describing your problem, since this issue is against user.module in Drupal core.

gerd riesselmann’s picture

gerd riesselmann commented

I am not sure if your patch is the right approach because the headers can be more than one field separated by newlines. So the presence of a newline does not mean that it is a spam attempt.

You're right. And even worse: A check against multiple new lines, either \r or \n, in this case may not be enough, though adding it surely does no harm.

Example:

The header is totally valid :-(.

As far as I can see, the only chance is to encourage module writers to use the validating function I proposed (which should be named user_mail_check_email_header() rather than check_email_header()) before passing user input as $header to user_mail().

However, one can additionally think about an "overloaded" version of user_mail() which also takes "from" as a parameter. Either as a seperate function:

function user_mail_from($mail, $subject, $message, $from = "", $header = "") {
  if (empty($from)) {
    $from = [DEFAULT DRUPAL EMAIL ADDRESS];
  }
  $from = check_email_header($from);
  if (!empty($header)) {
    $header .= "\n" . $from;
  }
  else {
    $header = $from;
  }
  user_mail($mail, $subject, $message, $header);
}

Or by adding an additional, optional parameter to user_mail():

function user_mail($mail, $subject, $message, $header, $from = "") {
  if (!empty($from)) {
    $from = check_email_header($from);
    if (!empty($header)) {
      $header .= "\n" . $from;
    }
    else {
      $header = $from;
    }
  }
  ... Here goes original code of function user_mail() ...
}

This, although provided mainly for convenience, covers an usual use case (no mail without from) and therefore avoids duplication of code within modules. And it implicitly serves better security, since checking the from field cannot be forgotten.

henk@sharewareblogs.com’s picture

henk@sharewareblogs.com commented

kbahey:

After using the patch that was originally proposed, there were still successful attacks, as I could see from the delivery failures being returned to me. I have temporarily disabled the feedback module until I find a solution that I know works. But meanwhile, now the attacker is trying to abuse the "email this page" link, so far without success.

I see that there is a fix for the feedback module, but how do I install that?

kbahey’s picture

kbahey commented

Henk

I think the definition of attack is what is missing:

- Relay spam. This is when your form on your site is used to send spam to others. This has been solved in feedback. Just download the latest version and it has that fixed.

- Getting spam. People can still send you spam using the form. This is something that is sort of by design, but can be improved (e.g. require a confirmation form, captcha, ...etc.)

We should really not be discussing this here, since it is about another non-core module.

henk@sharewareblogs.com’s picture

henk@sharewareblogs.com commented

I do know the difference between relaying and just sending a message to me. Sending mesages to me doesn't bother me. But the attacker is specifically trying to relay. Today I got "email this page" messages with a BCC included, but that didn't work. So that means they are still experimenting. Is the emailpage module vulnerable?
To get the feedback module securred, I assume I just have to get the latest CVS version? Will it work with the latest stable release of drupal? Last time I tried to install mudules from the CVS they didn't work because some core functions have changed signatures.

Cvbge’s picture

Cvbge commented
Version: 4.6.3 » x.y.z

This bug also happens for cvs version, changing to such (might get fixed faster ;))

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Version: x.y.z » 4.6.5

For 4.7/HEAD, this would be a duplicate of http://drupal.org/node/34002

For 4.6.5 it is nice to keep this issue around as a tracker to take any patch from 34002 and modify it to work (if possible) with this issue.

beginner’s picture

beginner commented
Version: 4.6.5 » 4.6.8

reviewing the thread, I am not sure if the issue has been fixed or not.
The title mentions "email injection" which is another name for spam relay, no?
Yet, the spam relay bug is said to be fixed in pre-4.7 cvs, as well as 4.6.
The spammer using the contact form to send the site administrator some spam is not actually a bug, no?
So: what exactly needs to be fixed?

beginner’s picture

beginner commented

http://drupal.org/node/34002 has patched the contact.module against relay exploits.
Looking at the cvs message, it seems that the 4-6 branch has not been patched at all, so if it's not a serious bug that got introduced with the new 4.7 FAPI, then 4.6 needs to be patched urgently.

It's getting late, here. I'll review more in details and test this issue later.

magico’s picture

magico commented
StatusFileSize
new user.module.patch_0.txt1.84 KB

This patch backports some code from drupal_mail() in CVS and tries to get around the $header parameter by creating an internal loop to test it.
At least, by my tests it does not allow to fake the "From" field.

magico’s picture

magico commented
StatusFileSize
new teste.php.txt1002 bytes

The file I used to test some headers injection!

magico’s picture

magico commented
Status: Active » Needs review

Please review it.
Thanks

beginner’s picture

beginner commented

Thanks for tackling this issue! :)
Can you point out which cvs/4.7 patch you have based your patch on? I've been looking but didn't find it.

Steven’s picture

Steven commented

-1 for this patch.

Splitting $headers by linebreak is really incorrect, as MIME headers can span multiple lines (by appending a space character immediately after the linebreak).

This patch also limits functionality quite a bit, as you can no longer use differing Reply-To, Errors-To and From addresses.

Finally, this patch is really the wrong fix. Linebreaks are only a problem because mime_header_encode() is not used by the calling module when assembling $headers. Because many people do not realize it is necessary, it is easily forgotten. That's why we moved this transformation into drupal_mail() in HEAD/4.8.

However, in 4.6/4.7 mime_header_encode() must still be called manually by the caller. You simply cannot replace this transformation by a linebreak based check. 4.6/4.7 modules that call user_mail() should be fixed instead. Security safeguards should only be applied where they do not disrupt normal operation of the code.

It is not hard to grep a module for user_mail() and fix it.

beginner’s picture

beginner commented
Version: 4.6.8 » 4.6.9

@steven,
What do you mean by "4.6/4.7 modules"?
There is a email injection vulnerability that has been fixed in head BEFORE 4.7 came out, no? (at least this is my understanding of the whole thread). It means that 4.6 has a critical flaw that 4.7 doesn't have (that's why this issue is tagged 4.6.9).
In that regard, I don't understand your comment.

I have not been able to track the exact code that went into pre-4.7 HEAD.

beginner’s picture

beginner commented

both the following patches have been committed 1 month before 4.7 came out:
http://drupal.org/files/issues/contact_header_fix.patch
http://drupal.org/files/issues/header.newlines.patch
(see comment #10 above and http://drupal.org/node/34002)

Can you confirm that they are the patches that need to be backported to 4.6?

Steven’s picture

Steven commented

The patches you linked to fix the email injection vulnerability and should be applied to 4.6 yes.

A later patch to HEAD/4.8 (which also renamed user_mail() to drupal_mail()) moved all required mime_header_encode() calls into drupal_mail(), thus relieving the caller from the responsibility of encoding the header values.

The patch in this issue tries to do something similar (making user_mail() safe), but does so in a way which is neither robust nor smart.

beginner’s picture

beginner commented
Status: Needs review » Needs work

thanks.
I'll submit a patch soon.

beginner’s picture

beginner commented
Status: Needs work » Needs review
StatusFileSize
new mail.injection.diff.txt736 bytes

backporting the two patches mentioned earlier:

function contact_mail_page() does not exist in 4.6.
patches function contact_mail_user().

includes/unicode.inc didn't exist in 4.6.

I cannot believe I'm left with such a tiny patch...

@magico: I would like to particularly thank you for pushing this issue ahead. Without you this issue may have remained unsolved for a few more weeks/months :)

magico’s picture

magico commented

@beginner: no problem. I had 15 minutes of spare time and I thought I should "play" a little it. (OT: I really would like a clear 4.6 bug list)

@steven: do you think it's possible to make a better patch within user_mail() or we should quit (and save time to other things) by now and just use "mime_header_encode()" in the caller module?

beginner’s picture

beginner commented

obviously, nobody is really interested in 4.6 anymore.
Is the patch #22 enough to close a serious flaw in 4.6, for those who still use it?
Can it be RTBC'ed and the issue closed?

Steven’s picture

Steven commented
Status: Needs review » Needs work

beginner: again, wrong fix. Use mime_header_encode().

magico’s picture

magico commented
Version: 4.6.9 » 4.6.x-dev

What should we do with critical bugs, that exist but nobody cares?
I think they should stay open... but at this time almost everyone already updated their Drupal version.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Needs work » Closed (won't fix)

This version is not supported. Reopen or create a new issue if the problem exist in any recent version (version equal or above Drupal 5)