Injection also in contrib/web/php-user/mlmmj.php?

Injection also in contrib/web/php-user/mlmmj.php?

[Ansgar Burchardt]

Hi,

the changelog for mlmmj 1.2.16 contains the entry

   o Fixed injection in contrib/web/perl-user (Gerd von Egidy)

and removed the line

   -               . "Cc: $from\n"

from contrib/web/perl-user/mlmmj.cgi.  But the same logic is present
in the PHP script as well:

   $addheader .= "Cc: ".$this->email."\n";

This should probably be removed for the same reasons.

While looking at this, I saw the regular expression used to try to
validate e-mail addresses in the PHP script:

   "^[a-z0-9\._-]+".chr(64)."+[a-z0-9\._-]+\.+[a-z]{2,4}$"

This does not allow a plus (+) in the local part which is permitted (and
also used by mlmmj itself).  There are also TLDs that have more than
four characters: .travel, .museum, not to begin with international TLDs
like .xn--zckzah (which are just in testing for now).  The PHP script
should *not* make (wrong) assumptions about what TLDs exist.

Regards,
Ansgar

PS: Please CC me on replies.

Re: Injection also in contrib/web/php-user/mlmmj.php?

[Morten Shearman Kirkegaard]

Hi Ansgar,

On Sun, 2010-01-10 at 16:28 +0900, Ansgar Burchardt wrote:
>    o Fixed injection in contrib/web/perl-user (Gerd von Egidy)
...
> But the same logic is present in the PHP script as well:
> 
>    $addheader .= "Cc: ".$this->email."\n";

As far as I can see, there is no injection here, because the
$this->email variable is protected by the is_email() function. But
Christoph Thiel is probably the guy to ask; He is the author of the
code.

It might still be a good idea to keep the two sets of code as much in
sync as possible, though.

> While looking at this, I saw the regular expression used to try to
> validate e-mail addresses in the PHP script:
> 
>    "^[a-z0-9\._-]+".chr(64)."+[a-z0-9\._-]+\.+[a-z]{2,4}$"
> 
> This does not allow a plus (+) in the local part which is permitted (and
> also used by mlmmj itself).  There are also TLDs that have more than
> four characters: .travel, .museum, not to begin with international TLDs
> like .xn--zckzah (which are just in testing for now).  The PHP script
> should *not* make (wrong) assumptions about what TLDs exist.

I fully agree. Would you be up for fixing this problem?


Morten

-- 
Morten Shearman Kirkegaard <mopo@fabletech.com>
CTO, FableTech
http://fabletech.com/

Re: Injection also in contrib/web/php-user/mlmmj.php?

[Gerd v. Egidy]

[-- Attachment #1: Type: Text/Plain, Size: 834 bytes --]

Hi,

> While looking at this, I saw the regular expression used to try to
> validate e-mail addresses in the PHP script:
> 
>    "^[a-z0-9\._-]+".chr(64)."+[a-z0-9\._-]+\.+[a-z]{2,4}$"
> 
> This does not allow a plus (+) in the local part which is permitted (and
> also used by mlmmj itself).  There are also TLDs that have more than
> four characters: .travel, .museum, not to begin with international TLDs
> like .xn--zckzah (which are just in testing for now).  The PHP script
> should *not* make (wrong) assumptions about what TLDs exist.

Good catch. Since the use is the same I think it we should use the same regex 
for perl and php. And then verify once if it fits all valid mail addrs.

The attached patch uses the same regex for php as I introduced for perl with 
1.2.16. Please have a deep look at it.

Kind regards,

Gerd

[-- Attachment #2: mlmmj-1.2.17-RC2-php-regex.patch --]
[-- Type: text/x-patch, Size: 575 bytes --]

diff -r -u mlmmj-1.2.17-RC2.orig/contrib/web/php-user/mlmmj.php mlmmj-1.2.17-RC2/contrib/web/php-user/mlmmj.php
--- mlmmj-1.2.17-RC2.orig/contrib/web/php-user/mlmmj.php	2008-10-30 21:06:16.000000000 +0100
+++ mlmmj-1.2.17-RC2/contrib/web/php-user/mlmmj.php	2010-01-10 15:19:50.000000000 +0100
@@ -37,7 +37,7 @@
 
     function is_email($string="") 
 	{
-	    if (eregi("^[a-z0-9\._-]+".chr(64)."+[a-z0-9\._-]+\.+[a-z]{2,4}$", $string)) 
+	    if (preg_match("/^[-!#$%&\'*+\.\/0-9=?A-Z^_a-z{|}~]+@[-0-9A-Za-z]+\.[-\.0-9A-Za-z]+$/", $string))
 	    { 
 		return TRUE; 
 	    }