mailform checking correct email input

[svasti]

Hi developers,

while working on memberspages I realised that mailform.php has an overly restricted check on entered sender's email.

Code: Select all

preg_match("/^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[_a-z0-9-]+(\.[_a-z0-9-]+)*(\.([a-z]{2,4}))+$/i", $sender)

I have a client with the domain "sachverständiger-bestattungswesen.de", any email of this domain wouldn't be accepted.

At least we have to allow accented letters, cyrillic and greek etc.
What about simplifying the regex to:

Code: Select all

preg_match('!^[^@]+@[^@|^\s]+$!',$sender)

Something to consider for 1.6

svasti

[cmb]

Hi Frank,

I never thought about IDN in mail addresses before. But indeed that's something that might gain more relevance in the future. There are even plans to allow the local part of the mail address to use UTF-8 characters (International email). I have no idea though, if this is widely implemented for MTAs and MUAs currently.

But changing the overly restricted regex might be a good idea anyway. I've found an interesting article about the subject: http://www.linuxjournal.com/article/9585 (it seems it doesn't cater for UTF-8 characters in the mail addresses though). But it shows that even Abc\@def@example.com is a valid mail address! (I didn't know that before)

I haven't read the article yet, but it seems, it's quite some work to get the check right, and it still does allow the visitor to enter a mail address, that doesn't exist. A domain lookup could be implemented too, but that might be overkill. So sticking with an overly permissive regex seems to be reasonable; perhaps even a simple !empty($sender) might do.

Christoph

[Gert]

Hi Svasti,

nice idea to simplify the regexp, I have realized it in CMSimpleCoAuthors, thank you

By playing around I found, that there is an unclosed div (in the mailform.php) in case of some messages, so div-based templates may be destroyed.

Here is the corrected code for mailform.php, have a look at the last (commented) lines:

Code: Select all

...
    if (!(preg_match('!^[^@]+@[^@|^\s]+$!', $sender)))
    {
        $e .= '<li>' . $tx['mailform']['notaccepted'] . '</li>';
    }
    if (!$e && !(@mail_utf8($cf['mailform']['email'], $tx['menu']['mailform'] . ' ' . sv('SERVER_NAME'), $msg, "From: " . stsl($sender) . "\r\n" . "X-Remote: " . sv('REMOTE_ADDR') . "\r\n")))
    {
        $e .= '<li>' . $tx['mailform']['notsend'] . '</li>' . "\n";
    }
    else
    {
        $t = '<p>' . $tx['mailform']['send'] . '</p>' . "\n"; // ADDED . "\n"
    }
    $t.= '</div>' . "\n"; // NEW LINE with the closing div
} 

You should correct it also in CMSimple_XH (put it to the roadmap) - or download CMSimpleCoAuthors new and replace the old mailform.php by the new one from CMSimpleCoAuthors,

Gert

[cmb]

Hi Gert,

By playing around I found, that there is an unclosed div (in the mailform.php) in case of some messages

Indeed! The <div id="cmsimple_mailform"> won't be closed, if the mail could be sent successfully. The problem: in line 34 the <div> is opened unconditionally, but in line 126 the <div> is closed only, when ($t == '' || $e != ''). So perhaps it's the cleanest solution to close the <div> in line 130 (instead of 126):

Code: Select all

$o .= '</div>' . "\n";

Christoph

[Gert]

Ok, now I have closed the div on first level, added "\n" after </h1> (for nice html output) and made new downloads of CMSimpleCoAuthors.

You can use the new mailform.php for CMSimple_XH, if you want,

Gert

[cmb]

Hi Frank, hi Gert,

perhaps even a simple !empty($sender) might do.

That would have resulted in an email injection vulnerability!

So Frank's suggestion might be a a good idea. But it would reject a valid email address such as "Abc\@def@example.com". So I suggest:

Code: Select all

preg_match('!^([^@\s]|(?<=\\\\)@)+@[^@\s]+$!u', $sender) 

Adding the u-Modifier is probably not necessary, but definitely does not hurt.

There are even plans to allow the local part of the mail address to use UTF-8 characters (International email). I have no idea though, if this is widely implemented for MTAs and MUAs currently.

This is because most, if not all, email servers, at the time of this writing[when?], do not support these characters.

So it's probably a good idea to escape the From header field. I'm not sure, if it can be done the same way as the base64 encoding of the subject line, though.

Christoph

PS: It seems that base64 encoding the From header shouldn't be a problem.

[svasti]

Ha, ha, now it's getting tricky, not only /@ is allowed but

Local part:
Special characters are allowed with restrictions. They are:
Space and "(),:;<>@[\] (ASCII: 32, 34, 40, 41, 44, 58, 59, 60, 62, 64, 91–93)

The restrictions for special characters are that they must only be used when contained between quotation marks, and that 2 of them (the backslash \ and quotation mark " (ASCII: 32, 92, 34)) must also be preceded by a backslash \ (e.g. "\\\"").

Comments are allowed with parentheses, e.g. "john.smith(comment)@example.com", "john(comment).smith@example.com", and "joh(comment)n.smith@example.com" are all equivalent to "john.smith@example.com"
International characters above U+007F are permitted

valid email address such as "Abc\@def@example.com".

seems not to be valid according to the above wikipedia quote. But may be Wikipedia is wrong?

http://www.ex-parrot.com/~pdw/Mail-RFC822-Address.html gives a regular expression to validate mail. Seems nearly to be longer than the CMSimple core

svasti

[cmb]

http://www.ex-parrot.com/~pdw/Mail-RFC822-Address.html gives a regular expression to validate mail.

That's a nice example of a simple and graspable regular expression! Unfortunately it implements RFC 822 which was superseeded by RFC 2822 which was superseeded by RFC 5322. Regarding international mail additionally RFC 6531 has to be consulted.

I've found several email validation regexps, but none of them seem to permit all valid email addresses (even if the claimy to be overly permissive). My suggestion fails too, as it rejects "Abc@def"@example.com and "Fred Bloggs"@example.com for example. Ah... well, I found http://isemail.info/about which seems to be very good, but it's more than 50KB of PHP -- so it probably doesn't fit well to CMSimple(_XH).

As it's clear now, that checking a valid email adress is no simple task, and that doing it exactly might be overkill for CMSimple_XH's built-in mailform, it seems to be necessary to declare the priorities. These are IMO:

• do not allow any email injection vulnerability
• do not reject any valid email address

The following should allow any valid email address (and many many more), except those containing embedded newlines or carriage returns (which can't be entered to an <input type="text"> anyway):

Code: Select all

^[^\r\n]+@[^\s]+$

Additionally we might check the domain part with gethostbyname(). I do not consider this a good solution, but it's probably sufficient according to the necessities stated earlier. It's simple and secure (if I don't overlook something).

I would be glad, if anybody comes up with a better solution!

[svasti]

Code: Select all

    ^[^\r\n]+@[^\s]+$

I think that's a nice and sufficient one

[cmb]

If we allow non ASCII characters for the email address, we have to consider, that traditionally only ASCII characters are allowed in mail headers. RFC 4952 is not established as standard yet, and I'm not sure, if the downgrade mechanism described there is already widely implemented. So we might consider to apply the ToASCII conversion to the IDN before sending the mail.