Don't echo (unsanitized) OTP/NONCE values back to client when

sending error codes.  Reported by Paul van Empelen.

Makefile

@@ -1,4 +1,4 @@
-VERSION = 2.9
+VERSION = 2.10
 PACKAGE = yubikey-val
 CODE = COPYING Makefile NEWS ykval-checksum-clients.php			\
 	ykval-common.php ykval-config.php ykval-db.php ykval-db.sql	\

NEWS

@@ -1,3 +1,46 @@
+* Version 2.10 unreleased
+
+  * Don't echo (unsanitized) OTP/NONCE values back to client when
+    sending error codes.  Reported by Paul van Empelen.
+
+    Resolving this problem protects (arguably buggy) clients against
+    an attack.  Prior versions of the Yubico C and PHP clients do not
+    appear to exhibit this bug.  We provide an analysis of the issue
+    below so that you can review client implementations for the
+    problem.  Note that you do not have to fix clients if you are
+    using this server version (or later), although we recommend it
+    anyway.
+
+    If the client sends a OTP value that ends with '%0astatus=OK' the
+    server output will contain a line 'status=ok' before the real
+    status code status=MISSING_PARAMETER.  Note lower-casing of the
+    injected status code, so that it doesn't match a correct
+    'status=OK' response.  Note also that the OTP value would fail
+    normal input validation checks in the client.
+
+    If the client sends a NONCE value that ends with '%0astatus=OK'
+    the output will contain a line consisting of 'status=OK' before
+    the correct status=MISSING_PARAMETER.  However, the NONCE value is
+    generated by client code internally and does not come from any
+    untrusted source, thus the impact here is limited -- if an
+    attacker is able to trick a client into sending a crafted NONCE
+    value the attacker is normally able to modify the client code
+    somehow, and can thus trick the client in other ways as well.
+    Similar issues apply to the ID field, which is normally also under
+    control of the trusted client code and not something an attacker
+    could influence.
+
+    Thus, this server-side fix solve a client-side issue that we
+    believe would only occur when both of these conditions are true:
+
+    1) the client does not do proper input validation of the OTP, and
+    2) the client incorrectly parses 'status=ok' as 'status=OK'.
+
+    or when the following condition is true
+
+    A) the client can be tricked into sending a crafted NONCE or ID
+    value.
+
 * Version 2.9 released 2011-05-09
 
   * Support multiple IP authorizations in ykval-revoke.php.

ykval-verify.php

@@ -56,7 +56,7 @@ if ($protocol_version>=2.0) {
   /* Nonce is required from protocol 2.0 */
   if(!$nonce) {
     $myLog->log(LOG_NOTICE, 'Nonce is missing and protocol version >= 2.0');
-    sendResp(S_MISSING_PARAMETER, $apiKey, $extra);
+    sendResp(S_MISSING_PARAMETER);
     exit;
   }
  }
@@ -76,51 +76,51 @@ if ($protocol_version>=2.0) {
 
 if ($otp == '') {
   $myLog->log(LOG_NOTICE, 'OTP is missing');
-  sendResp(S_MISSING_PARAMETER, $apiKey, $extra);
+  sendResp(S_MISSING_PARAMETER);
   exit;
 }
 
 if (strlen($otp) < TOKEN_LEN || strlen ($otp) > OTP_MAX_LEN) {
   $myLog->log(LOG_NOTICE, 'Incorrect OTP length: ' . $otp);
-  sendResp(S_BAD_OTP, $apiKey, $extra);
+  sendResp(S_BAD_OTP);
   exit;
 }
 
 if (preg_match("/^[cbdefghijklnrtuv]+$/", $otp)==0) {
   $myLog->log(LOG_NOTICE, 'Invalid OTP: ' . $otp);
-  sendResp(S_BAD_OTP, $apiKey, $extra);
+  sendResp(S_BAD_OTP);
   exit;
 }
 
 if (preg_match("/^[0-9]+$/", $client)==0){
   $myLog->log(LOG_NOTICE, 'id provided in request must be an integer');
-  sendResp(S_MISSING_PARAMETER, $apiKey, $extra);
+  sendResp(S_MISSING_PARAMETER);
   exit;
- }
+}
 
 if ($timeout && preg_match("/^[0-9]+$/", $timeout)==0) {
   $myLog->log(LOG_NOTICE, 'timeout is provided but not correct');
-  sendResp(S_MISSING_PARAMETER, $apiKey, $extra);
+  sendResp(S_MISSING_PARAMETER);
   exit;
- }
+}
 
 if ($nonce && preg_match("/^[A-Za-z0-9]+$/", $nonce)==0) {
   $myLog->log(LOG_NOTICE, 'NONCE is provided but not correct');
-  sendResp(S_MISSING_PARAMETER, $apiKey, $extra);
+  sendResp(S_MISSING_PARAMETER);
   exit;
- }
+}
 
 if ($nonce && (strlen($nonce) < 16 || strlen($nonce) > 40)) {
   $myLog->log(LOG_NOTICE, 'Nonce too short or too long');
-  sendResp(S_MISSING_PARAMETER, $apiKey, $extra);
+  sendResp(S_MISSING_PARAMETER);
   exit;
 }
    
 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, $apiKey, $extra);
+  sendResp(S_MISSING_PARAMETER);
   exit;
- }
+}
 
 // NOTE: Timestamp parameter is not checked since current protocol says that 1 means request timestamp
 // and anything else is discarded. 
@@ -129,7 +129,7 @@ if ($sl && (preg_match("/^[0-9]+$/", $sl)==0 || ($sl<0 || $sl>100))) {
 //
 if ($client <= 0) {
   $myLog->log(LOG_NOTICE, 'Client ID is missing');
-  sendResp(S_MISSING_PARAMETER, $apiKey, $extra);
+  sendResp(S_MISSING_PARAMETER);
   exit;
 }
 
@@ -142,14 +142,14 @@ $sync->addField('ip', $_SERVER['REMOTE_ADDR']);
 $sync->addField('otp', $otp);
 
 if (! $sync->isConnected()) {
-  sendResp(S_BACKEND_ERROR, $apiKey, $extra);
+  sendResp(S_BACKEND_ERROR);
   exit;
  }
 
 $cd=$sync->getClientData($client);
 if(!$cd) {
   $myLog->log(LOG_NOTICE, 'Invalid client id ' . $client);
-  sendResp(S_NO_SUCH_CLIENT, $apikey, $extra);
+  sendResp(S_NO_SUCH_CLIENT);
   exit;
  }
 $myLog->log(LOG_DEBUG,"Client data:", $cd);
@@ -173,7 +173,7 @@ if ($h != '') {
   // Compare it
   if ($hmac != $h) {
     $myLog->log(LOG_DEBUG, 'client hmac=' . $h . ', server hmac=' . $hmac);
-    sendResp(S_BAD_SIGNATURE, $apiKey, $extra);
+    sendResp(S_BAD_SIGNATURE, $apiKey);
     exit;
   }
 }
@@ -190,7 +190,7 @@ if ($protocol_version<2.0) {
 //
 $urls = otp2ksmurls ($otp, $client);
 if (!is_array($urls)) {
-  sendResp(S_BACKEND_ERROR, $apiKey, $extra);
+  sendResp(S_BACKEND_ERROR, $apiKey);
   exit;
 }
 
@@ -198,7 +198,7 @@ if (!is_array($urls)) {
 //
 $otpinfo = KSMdecryptOTP($urls);
 if (!is_array($otpinfo)) {
-  sendResp(S_BAD_OTP, $apiKey, $extra);
+  sendResp(S_BAD_OTP, $apiKey);
   exit;
 }
 $myLog->log(LOG_DEBUG, "Decrypted OTP:", $otpinfo);
@@ -210,14 +210,14 @@ $yk_publicname=$devId;
 $localParams = $sync->getLocalParams($yk_publicname);
 if (!$localParams) {
   $myLog->log(LOG_NOTICE, 'Invalid Yubikey ' . $yk_publicname);
-  sendResp(S_BACKEND_ERROR, $apiKey, $extra);
+  sendResp(S_BACKEND_ERROR, $apiKey);
   exit;
  }
 
 $myLog->log(LOG_DEBUG, "Auth data:", $localParams);
 if ($localParams['active'] != 1) {
   $myLog->log(LOG_NOTICE, 'De-activated Yubikey ' . $devId);
-  sendResp(S_BAD_OTP, $apiKey, $extra);
+  sendResp(S_BAD_OTP, $apiKey);
   exit;
 }
 
@@ -254,7 +254,7 @@ if ($sync->countersHigherThanOrEqual($localParams, $otpParams)) {
 
 if(!$sync->updateDbCounters($otpParams)) {
   $myLog->log(LOG_CRIT, "Failed to update yubikey counters in database");
-  sendResp(S_BACKEND_ERROR, $apiKey, $extra);
+  sendResp(S_BACKEND_ERROR, $apiKey);
   exit;
  }
 
@@ -262,7 +262,7 @@ if(!$sync->updateDbCounters($otpParams)) {
 
 if (!$sync->queue($otpParams, $localParams)) {
   $myLog->log(LOG_CRIT, "ykval-verify:critical:failed to queue sync requests");
-  sendResp(S_BACKEND_ERROR, $apiKey, $extra);
+  sendResp(S_BACKEND_ERROR, $apiKey);
   exit;
  }