Merge pull request #59 from Yubico/enhance_data_validation

Enhanced data validation to address YSA-2020-01

ykval-common.php

@@ -45,7 +45,11 @@ define('TS_REL_TOLERANCE', 0.3);
 define('TS_ABS_TOLERANCE', 20);
 
 define('TOKEN_LEN', 32);
-define('OTP_MAX_LEN', 48); // TOKEN_LEN plus public identity of 0..16
+define('PUBID_MAX_LEN', 16);
+define('OTP_MAX_LEN', TOKEN_LEN + PUBID_MAX_LEN);
+define('NONCE_MIN_LEN', 16);
+define('NONCE_MAX_LEN', 40);
+define('INT32_LEN', 10);
 
 function logdie ($logger, $str)
 {
@@ -70,6 +74,45 @@ function getHttpVal ($key, $default, $a)
     return $val;
 }
 
+// Verifies if a given string is modhex
+function is_modhex($s) {
+    if (preg_match('/^[cbdefghijklnrtuv]+$/', $s) === 0) {
+        return false;
+    } else {
+        return true;
+    }
+}
+
+// Verifies if a given string is a valid OTP
+function is_otp($otp) {
+    if ($otp == "") {
+        return false;
+    }
+    $otp_len = strlen($otp);
+    return $otp_len >= TOKEN_LEN && $otp_len <= OTP_MAX_LEN && is_modhex($otp);
+}
+
+// Verifies if a given string is a valid public id
+function is_pubid($id) {
+    $id_len = strlen($id);
+    return $id_len >= 0 && $id_len <= PUBID_MAX_LEN && is_modhex($id);
+}
+
+// Verifies a given string is a valid nonce
+function is_nonce($nonce) {
+    return strlen($nonce) >= NONCE_MIN_LEN
+                           && strlen($nonce) <= NONCE_MAX_LEN
+                           && ctype_alnum($nonce);
+}
+
+// Verifies if a given string is a valid client id
+function is_clientid($id) {
+    if ($id == "0") {
+        return false;
+    }
+    return strlen($id) <= INT32_LEN && ctype_digit($id);
+}
+
 // Sign a http query string in the array of key-value pairs
 // return b64 encoded hmac hash
 function sign($a, $apiKey, $logger)

ykval-db-oci.php

@@ -117,7 +117,7 @@ class DbImpl extends Db
     }
 
     /**
-   * function to close the cursor after having fetched rows
+     * Function to close the cursor after having fetched rows
      *
      * @param object $result Query result object or null to use the current one
      *
@@ -156,6 +156,16 @@ class DbImpl extends Db
         $query.= " FROM " . $table;
         if ($where!=null){
             foreach ($where as $key=>$value) {
+                if ($key != 'server' && !(ctype_alnum($value) || is_null($value)))
+                {
+                    $this->myLog->log(LOG_WARNING, "findByMultiple: attempted to use non-alphanumeric in WHERE: " . $table . "." . $key . " = " . $value);
+                    return false;
+                }
+                elseif ($key == 'server' && !filter_var($value, FILTER_VALIDATE_URL))
+                {
+                    $this->myLog->log(LOG_WARNING, "findByMultiple: attempted use invalid URL in WHERE: " . $table . "." . $key . " = " . $value);
+                    return false;
+                }
                 if ($key!=null) {
                     if ($value!=null) $match.= " ". $key . " = '" . $value . "' and";
                     else $match.= " ". $key . " is NULL and";
@@ -207,6 +217,16 @@ class DbImpl extends Db
         if ($where!=null){
             $query.= " WHERE";
             foreach ($where as $key=>$value) {
+                if ($key != 'server' && !ctype_alnum($value))
+                {
+                    $this->myLog->log(LOG_WARNING, "deleteByMultiple: attempted to write non-alphanumeric to the database: " . $value);
+                    return false;
+                }
+                elseif ($key == 'server' && !filter_var($value, FILTER_VALIDATE_URL))
+                {
+                    $this->myLog->log(LOG_WARNING, "deleteByMultiple: attempted to write invalid URL to the database: " . $value);
+                    return false;
+                }
                 $query.= " ". $key . " = '" . $value . "' and";
             }
             $query=rtrim($query, "and");

ykval-db-pdo.php

@@ -36,8 +36,6 @@ require_once('ykval-db.php');
 
 class DbImpl extends Db
 {
-
-
     /**
      * Constructor
      *
@@ -99,7 +97,6 @@ class DbImpl extends Db
         }
     }
 
-
     /**
      * function to get a row from the query result
      * Once all rows have been fetch, function closeCursor needs to be called
@@ -154,6 +151,16 @@ class DbImpl extends Db
         {
             foreach ($where as $key => $value)
             {
+                if ($key != 'server' && !(ctype_alnum($value) || is_null($value)))
+                {
+                    $this->myLog->log(LOG_WARNING, "findByMultiple: attempted to use non-alphanumeric in WHERE: " . $table . "." . $key . " = " . $value);
+                    return false;
+                }
+                elseif ($key == 'server' && !filter_var($value, FILTER_VALIDATE_URL))
+                {
+                    $this->myLog->log(LOG_WARNING, "findByMultiple: attempted to use invalid URL in WHERE: " . $table . "." . $key . " = " . $value);
+                    return false;
+                }
                 if ($key != NULL)
                 {
                     if ($value != NULL)
@@ -216,6 +223,15 @@ class DbImpl extends Db
         if ($where!=null){
             $query.= " WHERE";
             foreach ($where as $key=>$value) {
+                if ($key != 'server' && !ctype_alnum($value)) {
+                    $this->myLog->log(LOG_WARNING, "deleteByMultiple: attempted to write non-alphanumeric to the database: " . $value);
+                    return false;
+                }
+                elseif ($key == 'server' && !filter_var($value, FILTER_VALIDATE_URL))
+                {
+                    $this->myLog->log(LOG_WARNING, "deleteByMultiple: attempted to write invalid URL to the database: " . $value);
+                    return false;
+                }
                 $query.= " ". $key . " = '" . $value . "' and";
             }
             $query=rtrim($query, "and");

ykval-db.php

@@ -119,9 +119,22 @@ abstract class Db
      */
     public function updateBy($table, $k, $v, $values)
     {
+        if (!ctype_alnum($v) && !filter_var($v, FILTER_VALIDATE_URL)) {
+            $this->myLog->log(LOG_WARNING, "updateBy: attempted to use an invalid value: " . $v);
+            return false;
+        }
         $query = "";
 
-    foreach ($values as $key=>$value){
+        foreach ($values as $key=>$value) {
+            if ($key != 'server' && !(ctype_alnum($value) || is_null($value))) {
+                $this->myLog->log(LOG_WARNING, "updateBy: attempted to write non-alphanumeric to the database: " . $table . "." . $key . " <= " . $value);
+                return false;
+            }
+            elseif ($key == 'server' && !filter_var($value, FILTER_VALIDATE_URL))
+            {
+                $this->myLog->log(LOG_WARNING, "updateBy: attempted to write invalid URL to the database: " . $table . "." . $key . " <= " . $value);
+                return false;
+            }
             if (!is_null($value)) $query .= ' ' . $key . "='" . $value . "',";
             else $query .= ' ' . $key . '=NULL,';
         }
@@ -137,7 +150,6 @@ abstract class Db
         return $this->query($query, false);
     }
 
-
     /**
      * function to update row in database
      *
@@ -165,9 +177,23 @@ abstract class Db
      */
     public function conditionalUpdateBy($table, $k, $v, $values, $condition)
     {
+        if (!ctype_alnum($v) || preg_match('/^[a-zA-Z0-9><=()_ ]*$/', $condition) == 0)
+        {
+            $this->myLog->log(LOG_WARNING, "conditionalUpdateBy: attempted to use non-alphanumeric value: " . $v . " - " . $condition);
+            return false;
+        }
         $query = ""; /* quiet the PHP Notice */
 
         foreach ($values as $key=>$value){
+            if ($key != 'server' && !is_int($value) && !ctype_alnum($value)) {
+                $this->myLog->log(LOG_WARNING, "conditionalUpdateBy: attempted to write non-alphanumeric to the database: " . $value);
+                return false;
+            }
+            elseif ($key == 'server' && !filter_var($value, FILTER_VALIDATE_URL))
+            {
+                $this->myLog->log(LOG_WARNING, "conditionalUpdateBy: attempted to write invalid URL to the database: " . $value);
+                return false;
+            }
             $query = $query . " " . $key . "='" . $value . "',";
         }
         if (! $query) {
@@ -182,7 +208,6 @@ abstract class Db
         return $this->query($query, false);
     }
 
-
     /**
      * Function to update row in database based on a condition.
      * An ID value is passed to select the appropriate column
@@ -211,6 +236,24 @@ abstract class Db
     {
         $query= 'INSERT INTO ' . $table . " (";
         foreach ($values as $key=>$value){
+            if ($key == 'server') {
+                $v = filter_var($value, FILTER_VALIDATE_URL);
+                if (!$v) {
+                    $this->myLog->log(LOG_WARNING, "save: bad server URL provided: " . $value);
+                    return false;
+                }
+                $value = $v;
+            } else if ($key == 'info') {
+                if (preg_match('/[a-zA-Z0-9&_=,]+/', $value) == 0) {
+                    $this->myLog->log(LOG_WARNING, "save: bad info string provided: " . $value);
+                    return false;
+                }
+            } else {
+                if ($value != '' && !is_int($value) && !ctype_alnum($value)) {
+                    $this->myLog->log(LOG_WARNING, "save: attempted to write non-alphanumeric to the database: " . $value);
+                    return false;
+                }
+            }
             if (!is_null($value)) $query = $query . $key . ",";
         }
         $query = rtrim($query, ",") . ') VALUES (';
@@ -227,7 +270,7 @@ abstract class Db
      * @param string $table Database table to update row in
      * @param int $nr Number of rows to collect. NULL=>inifinity. DEFAULT=1.
      * @return mixed Array with values from Db row or 2d-array with multiple rows
-or false on failure.
+     or false on failure.
      *
      */
     public function last($table, $nr=1)

ykval-export

@@ -46,7 +46,7 @@ $db = Db::GetDatabaseHandle($baseParams, $logname);
 if (!$db->connect()) {
     $myLog->log(LOG_WARNING, "Could not connect to database");
     exit(1);
- }
+}
 
 $result=$db->customQuery("SELECT active, created, modified, yk_publicname, yk_counter, yk_use, yk_low, yk_high, nonce, notes FROM yubikeys ORDER BY yk_publicname");
 while($row = $db->fetchArray($result)){
@@ -61,7 +61,7 @@ while($row = $db->fetchArray($result)){
                              "," . $row['nonce'] .
                              "," . $row['notes'] .
                              "\n";
- }
+}
 
 $db->closeCursor($result);
 $db->disconnect();

ykval-export-clients

@@ -46,7 +46,7 @@ $db = Db::GetDatabaseHandle($baseParams, $logname);
 if (!$db->connect()) {
     $myLog->log(LOG_WARNING, "Could not connect to database");
     exit(1);
- }
+}
 
 $result = $db->customQuery("select id, active, created, secret, email, notes, otp from clients order by id");
 while($row = $db->fetchArray($result)) {

ykval-import-clients

@@ -47,7 +47,7 @@ if (!$db->connect()) {
     $myLog->log(LOG_WARNING, "Could not connect to database");
     error_log("Could not connect to database");
     exit(1);
- }
+}
 
 
 while ($res=fgetcsv(STDIN, 0, ",")) {
@@ -83,7 +83,7 @@ while ($res=fgetcsv(STDIN, 0, ",")) {
         }
     }
     $db->closeCursor($result);
- }
+}
 
 
 $myLog->log(LOG_NOTICE, "Successfully imported clients to database");

ykval-munin-ksmlatency.php

@@ -1,6 +1,5 @@
 #!/usr/bin/php
 <?php
-
 # Copyright (c) 2010-2015 Yubico AB
 # All rights reserved.
 #

ykval-munin-queuelength.php

@@ -38,7 +38,6 @@ require_once 'ykval-config.php';
 require_once 'ykval-common.php';
 require_once 'ykval-synclib.php';
 
-
 $urls = $baseParams['__YKVAL_SYNC_POOL__'];
 
 if ($argc == 2 && strcmp($argv[1], 'autoconf') == 0)

ykval-munin-vallatency.php

@@ -37,7 +37,6 @@ set_include_path(implode(PATH_SEPARATOR, array(
 require_once 'ykval-config.php';
 require_once 'ykval-common.php';
 
-
 $urls = $baseParams['__YKVAL_SYNC_POOL__'];
 
 if ($argc == 2 && strcmp($argv[1], 'autoconf') == 0)

ykval-resync.php

@@ -47,7 +47,7 @@ $yk = $_REQUEST["yk"];
 if (!$yk) {
     logdie($myLog, "ERROR Missing parameter");
 }
-if (!($yk == "all" || preg_match("/^([cbdefghijklnrtuv]{0,16})$/", $yk))) {
+if (!($yk == "all" || is_pubid($yk))) {
     logdie($myLog, "ERROR Unknown yk value: $yk");
 }
 $myLog->addField('yk', $yk);

ykval-revoke.php

@@ -47,7 +47,7 @@ $do = $_REQUEST["do"];
 if (!$yk || !$do) {
     logdie($myLog, "ERROR Missing parameter");
 }
-if (!preg_match("/^([cbdefghijklnrtuv]{0,16})$/", $yk)) {
+if (!is_pubid($yk)) {
     logdie($myLog, "ERROR Unknown yk value: $yk");
 }
 if ($do != "enable" && $do != "disable") {

ykval-sync.php

@@ -94,10 +94,8 @@ if (! $sync->isConnected())
 
 // at this point we should have the otp so let's add it to the logging module
 $myLog->addField('otp', $syncParams['otp']);
-$sync->addField('otp', $syncParams['otp']);
 
-
+// Verify correctness of numeric input parameters
-// verify correctness of input parameters
 foreach (array('modified','yk_counter', 'yk_use', 'yk_high', 'yk_low') as $param)
 {
     // -1 is valid except for modified
@@ -112,7 +110,26 @@ foreach (array('modified','yk_counter', 'yk_use', 'yk_high', 'yk_low') as $param
     sendResp(S_MISSING_PARAMETER, $myLog);
 }
 
+// Verify correctness of OTP input
+if (!is_otp($syncParams['otp'])) {
+    $myLog->log(LOG_NOTICE, "Input parameter " . $syncParams['otp'] . " not correct");
+    sendResp(S_MISSING_PARAMETER, $myLog);
+}
+
+// Verify correctness of pubid input
+if (!is_pubid($syncParams['yk_publicname'])) {
+    $myLog->log(LOG_NOTICE, "Input parameter " . $syncParams['yk_publicname'] . " not correct");
+    sendResp(S_MISSING_PARAMETER, $myLog);
+}
+
+// Verify correctness of nonce input
+if (!is_nonce($syncParams['nonce'])) {
+    $myLog->log(LOG_NOTICE, "Input parameter " . $syncParams['nonce'] . " not correct");
+    sendResp(S_MISSING_PARAMETER, $myLog);
+}
+
 // get local counter data
+$sync->addField('otp', $syncParams['otp']);
 $yk_publicname = $syncParams['yk_publicname'];
 if (($localParams = $sync->getLocalParams($yk_publicname)) === FALSE)
 {

ykval-verify.php

@@ -170,71 +170,46 @@ if ($protocol_version >= 2.0)
  */
 
 /* Change default protocol "strings" to numeric values */
-if (isset($sl) && strcasecmp($sl, 'fast') == 0)
+if (isset($sl) && $sl != '') {
-{
+    if (strcasecmp($sl, 'fast') == 0) {
         $sl = $baseParams['__YKVAL_SYNC_FAST_LEVEL__'];
-}
+    } else if (strcasecmp($sl, 'secure') == 0) {
-if (isset($sl) && strcasecmp($sl, 'secure') == 0)
-{
         $sl = $baseParams['__YKVAL_SYNC_SECURE_LEVEL__'];
-}
+    } else {
-if (!isset($sl) || $sl == '')
+        $sl = intval($sl); // non-numbers return zero
-{
+        if ($sl < 1) {
-	$sl = $baseParams['__YKVAL_SYNC_DEFAULT_LEVEL__'];
-}
-if ($sl && (preg_match("/^[0-9]+$/", $sl)==0 || ($sl<0 || $sl>100)))
-{
             $myLog->log(LOG_NOTICE, 'SL is provided but not correct');
             sendResp(S_MISSING_PARAMETER, $myLog);
+        }
+    }
+} else {
+    $sl = $baseParams['__YKVAL_SYNC_DEFAULT_LEVEL__'];
 }
 
-if (!isset($timeout) || $timeout == '')
+if (!isset($timeout) || $timeout == '') {
-{
     $timeout = $baseParams['__YKVAL_SYNC_DEFAULT_TIMEOUT__'];
-}
+} else if (!ctype_digit($timeout)) {
-if ($timeout && preg_match("/^[0-9]+$/", $timeout) == 0)
-{
     $myLog->log(LOG_NOTICE, 'timeout is provided but not correct');
     sendResp(S_MISSING_PARAMETER, $myLog);
 }
 
-if ($otp == '')
+if (!is_otp($otp))
-{
-	$myLog->log(LOG_NOTICE, 'OTP is missing');
-	sendResp(S_MISSING_PARAMETER, $myLog);
-}
-if (strlen($otp) < TOKEN_LEN || strlen($otp) > OTP_MAX_LEN)
-{
-	$myLog->log(LOG_NOTICE, "Incorrect OTP length: $otp");
-	sendResp(S_BAD_OTP, $myLog);
-}
-if (preg_match('/^[cbdefghijklnrtuv]+$/', $otp) == 0)
 {
     $myLog->log(LOG_NOTICE, "Invalid OTP: $otp");
     sendResp(S_BAD_OTP, $myLog);
 }
 
-if (preg_match("/^[0-9]+$/", $client) == 0)
+if (!is_clientid($client))
 {
-	$myLog->log(LOG_NOTICE, 'id provided in request must be an integer');
+    $myLog->log(LOG_NOTICE, 'Client ID is missing or invalid');
-	sendResp(S_MISSING_PARAMETER, $myLog);
-}
-if ($client === '0')
-{
-	$myLog->log(LOG_NOTICE, 'Client ID is missing');
     sendResp(S_MISSING_PARAMETER, $myLog);
 }
 
-if (isset($nonce) && preg_match("/^[A-Za-z0-9]+$/", $nonce) == 0)
+if (isset($nonce) && !is_nonce($nonce))
 {
     $myLog->log(LOG_NOTICE, 'NONCE is provided but not correct');
     sendResp(S_MISSING_PARAMETER, $myLog);
 }
-if (isset($nonce) && (strlen($nonce) < 16 || strlen($nonce) > 40))
-{
-	$myLog->log(LOG_NOTICE, 'Nonce too short or too long');
-	sendResp(S_MISSING_PARAMETER, $myLog);
-}
 
 /**
  * Timestamp parameter is not checked since current protocol