Issues

ZF-12160: Zend_Validate_EmailAddress Deep MX Checking Logic Flaw

Description

Zend_Validate_EmailAddress::_isReserved fails on MX IP addresses that are public. Some are because of public IPs in the $_invalidIp array, and some are because there's something wrong with the logic.

These two domains return false negatives: - http://dnsquery.org/dnsquery/harn.ufl.edu/MX - http://dnsquery.org/dnsquery/martinhealth.org/MX

Comments

My next comment will be a patch, using my solution. I decided that the simplest (clearest?) way to compare IP addresses was to convert them to their decimal integer equivalents, and I restructured the reserved IP array to accommodate that.


Index: EmailAddress.php
===================================================================
--- EmailAddress.php    (revision 1376)
+++ EmailAddress.php    (working copy)
@@ -63,27 +63,27 @@
     );
 
     /**
-     * @see http://en.wikipedia.org/wiki/IPv4
-     * @var array
+     * @see http://en.wikipedia.org/wiki/…
+     * @var array [first octet] => [[CIDR] => [[range start], [range end]]]
      */
-    protected $_invalidIp = array(
-        '0'   => '0.0.0.0/8',
-        '10'  => '10.0.0.0/8',
-        '127' => '127.0.0.0/8',
-        '128' => '128.0.0.0/16',
-        '169' => '169.254.0.0/16',
-        '172' => '172.16.0.0/12',
-        '191' => '191.255.0.0/16',
+    protected $_reservedIps = array(
+        '0' => array('0.0.0.0/8' => array('0.0.0.0', '0.255.255.255',),),
+        '10' => array('10.0.0.0/8' => array('10.0.0.0', '10.255.255.255',),),
+        '127' => array('127.0.0.0/8' => array('127.0.0.0', '127.255.255.255',),),
+        '169' => array('169.254.0.0/16' => array('169.254.0.0', '169.254.255.255',),),
+        '172' => array('172.16.0.0/12' => array('172.16.0.0', '172.31.255.255',),),
         '192' => array(
-            '192.0.0.0/24',
-            '192.0.2.0/24',
-            '192.88.99.0/24',
-            '192.168.0.0/16'
+            '192.0.2.0/24' => array('192.0.2.0', '192.0.2.255',),
+            '192.88.99.0/24' => array('192.88.99.0', '192.88.99.255',),
+            '192.168.0.0/16' => array('192.168.0.0', '192.168.255.255',),
         ),
-        '198' => '198.18.0.0/15',
-        '223' => '223.255.255.0/24',
-        '224' => '224.0.0.0/4',
-        '240' => '240.0.0.0/4'
+        '198' => array(
+            '198.18.0.0/15' => array('198.18.0.0', '198.19.255.255',),
+            '198.51.100.0/24' => array('198.51.100.0', '198.51.100.255',),
+        ),
+        '203' => array('203.0.113.0/24' => array('203.0.113.0', '203.0.113.255',),),
+        '224' => array('224.0.0.0/4' => array('224.0.0.0', '239.255.255.255',),),
+        '240' => array('240.0.0.0/4' => array('240.0.0.0', '255.255.255.255',),),
     );
 
     /**
@@ -337,64 +337,52 @@
      * @param string $host
      * @return boolean
      */
-    private function _isReserved($host){
+    private function _isReserved($host)
+    {
         if (!preg_match('/^([0-9]{1,3}\.){3}[0-9]{1,3}$/', $host)) {
             $host = gethostbyname($host);
         }
 
-        $octet = explode('.',$host);
-        if ((int)$octet[0] >= 224) {
+        $octets = explode('.', $host);
+        if (224 <= (int) $octets[0]) {
+            // IP Addresses beginning with 224 or greater are all reserved, short-circuit range checks
             return true;
-        } else if (array_key_exists($octet[0], $this->_invalidIp)) {
-            foreach ((array)$this->_invalidIp[$octet[0]] as $subnetData) {
-                // we skip the first loop as we already know that octet matches
-                for ($i = 1; $i < 4; $i++) {
-                    if (strpos($subnetData, $octet[$i]) !== $i * 4) {
-                        break;
-                    }
-                }
+        } elseif (array_key_exists($octets[0], $this->_reservedIps)) {
+            // for integer comparisons
+            $intIp = $this->_ipToInt($host);
 
-                $host       = explode("/", $subnetData);
-                $binaryHost = "";
-                $tmp        = explode(".", $host[0]);
-                for ($i = 0; $i < 4 ; $i++) {
-                    $binaryHost .= str_pad(decbin($tmp[$i]), 8, "0", STR_PAD_LEFT);
-                }
-
-                $segmentData = array(
-                    'network'   => (int)$this->_toIp(str_pad(substr($binaryHost, 0, $host[1]), 32, 0)),
-                    'broadcast' => (int)$this->_toIp(str_pad(substr($binaryHost, 0, $host[1]), 32, 1))
-                );
-
-                for ($j = $i; $j < 4; $j++) {
-                    if ((int)$octet[$j] < $segmentData['network'][$j] ||
-                        (int)$octet[$j] > $segmentData['broadcast'][$j]) {
-                        return false;
+            // loop over reserved IP addresses
+            foreach ($this->_reservedIps as $ranges) {
+                foreach ($ranges as $range) {
+                    if (($this->_ipToInt($range[0]) <= $intIp)
+                            && ($this->_ipToInt($range[1]) >= $intIp)) {
+                        // the IP address falls in a reserved range
+                        return true;
                     }
                 }
             }
 
-            return true;
+            // the IP address did not fall in a reserved range
+            return false;
         } else {
             return false;
         }
     }
 
     /**
-     * Converts a binary string to an IP address
+     * Convert a dot-decimal IP address to it's decimal integer equivalent
      *
-     * @param string $binary
-     * @return mixed
+     * @param string $ip
+     * @return integer
      */
-    private function _toIp($binary)
+    private function _ipToInt($ip)
     {
-        $ip  = array();
-        $tmp = explode(".", chunk_split($binary, 8, "."));
-        for ($i = 0; $i < 4 ; $i++) {
-            $ip[$i] = bindec($tmp[$i]);
+        $octets = explode('.', $ip);
+        foreach ($octets as $key => $octet) {
+            $octets[$key] = str_pad(decbin($octet), 8, '0', STR_PAD_LEFT);
         }
-
-        return $ip;
+        $bin = implode('', $octets);
+        return bindec($bin);
     }
 
     /**