From fb506d02380fe32664e0aa2d2bb91cd23c22596f Mon Sep 17 00:00:00 2001 From: Simon Josefsson Date: Thu, 18 Aug 2011 12:19:15 +0000 Subject: [PATCH] Don't echo (unsanitized) OTP/NONCE values back to client when sending error codes. Reported by Paul van Empelen. --- Makefile | 2 +- NEWS | 43 +++++++++++++++++++++++++++++++++++++++++++ ykval-verify.php | 46 +++++++++++++++++++++++----------------------- 3 files changed, 67 insertions(+), 24 deletions(-) diff --git a/Makefile b/Makefile index cacc21d..2935a25 100644 --- a/Makefile +++ b/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 \ diff --git a/NEWS b/NEWS index 8cf3af7..d37e86f 100644 --- a/NEWS +++ b/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. diff --git a/ykval-verify.php b/ykval-verify.php index 7b0d1ac..fcd0d36 100644 --- a/ykval-verify.php +++ b/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; }