From 4f6f4d6ca394ba8d5c0e47dc2d4b36050922e844 Mon Sep 17 00:00:00 2001 From: Alessio Leoncini Date: Wed, 15 Nov 2017 10:54:30 +0100 Subject: [PATCH 1/9] Added constant time strings comparison to avoid possible time-based attacks --- libraries/ArduinoOTA/ArduinoOTA.cpp | 23 ++++++++++++++++++++++- libraries/ArduinoOTA/ArduinoOTA.h | 1 + 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/libraries/ArduinoOTA/ArduinoOTA.cpp b/libraries/ArduinoOTA/ArduinoOTA.cpp index d3a4397c6d..5be2ab654f 100644 --- a/libraries/ArduinoOTA/ArduinoOTA.cpp +++ b/libraries/ArduinoOTA/ArduinoOTA.cpp @@ -229,7 +229,7 @@ void ArduinoOTAClass::_onRx(){ String result = _challengemd5.toString(); ota_ip.addr = (uint32_t)_ota_ip; - if(result.equals(response)){ + if(constantTimeEquals(result, response)){ _state = OTA_RUNUPDATE; } else { _udp_ota->append("Authentication Failed", 21); @@ -358,6 +358,27 @@ int ArduinoOTAClass::getCommand() { return _cmd; } +bool ArduinoOTAClass::constantTimeEquals(const String & string1, + const String & string2) { + // To avoid possible time-based attacks present function compares given + // strings in a constant time. + + // Preliminary check + if(string1.length() != string2.length()) { + return false; + } + + // Evaluates every character + bool equals = true; + size_t len = string1.length(); + for(size_t i = 0; i < len; i++){ + equals &= (string1.charAt(i) == string2.charAt(i)); + } + + // Return result + return equals; +} + #if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_ARDUINOOTA) ArduinoOTAClass ArduinoOTA; #endif diff --git a/libraries/ArduinoOTA/ArduinoOTA.h b/libraries/ArduinoOTA/ArduinoOTA.h index 9785d18f89..b9b3d7b1be 100644 --- a/libraries/ArduinoOTA/ArduinoOTA.h +++ b/libraries/ArduinoOTA/ArduinoOTA.h @@ -93,6 +93,7 @@ class ArduinoOTAClass void _onRx(void); int parseInt(void); String readStringUntil(char end); + bool constantTimeEquals(const String &string1, const String &string2); }; #if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_ARDUINOOTA) From 0a684ddc95fa9b27a8a7957b018b12409397848b Mon Sep 17 00:00:00 2001 From: Alessio Leoncini Date: Wed, 15 Nov 2017 11:50:05 +0100 Subject: [PATCH 2/9] Fixed data types --- libraries/ArduinoOTA/ArduinoOTA.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/ArduinoOTA/ArduinoOTA.cpp b/libraries/ArduinoOTA/ArduinoOTA.cpp index 5be2ab654f..1776588195 100644 --- a/libraries/ArduinoOTA/ArduinoOTA.cpp +++ b/libraries/ArduinoOTA/ArduinoOTA.cpp @@ -358,10 +358,10 @@ int ArduinoOTAClass::getCommand() { return _cmd; } +// To avoid possible time-based attacks present function compares given +// strings in a constant time. bool ArduinoOTAClass::constantTimeEquals(const String & string1, const String & string2) { - // To avoid possible time-based attacks present function compares given - // strings in a constant time. // Preliminary check if(string1.length() != string2.length()) { @@ -370,8 +370,8 @@ bool ArduinoOTAClass::constantTimeEquals(const String & string1, // Evaluates every character bool equals = true; - size_t len = string1.length(); - for(size_t i = 0; i < len; i++){ + unsigned int len = string1.length(); + for(unsigned int i = 0; i < len; i++){ equals &= (string1.charAt(i) == string2.charAt(i)); } From fe324cccfa483042104f244411c5611e06039192 Mon Sep 17 00:00:00 2001 From: Alessio Leoncini Date: Wed, 15 Nov 2017 13:48:48 +0100 Subject: [PATCH 3/9] Fixed indentation --- libraries/ArduinoOTA/ArduinoOTA.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/ArduinoOTA/ArduinoOTA.cpp b/libraries/ArduinoOTA/ArduinoOTA.cpp index 1776588195..4d486285d2 100644 --- a/libraries/ArduinoOTA/ArduinoOTA.cpp +++ b/libraries/ArduinoOTA/ArduinoOTA.cpp @@ -365,7 +365,7 @@ bool ArduinoOTAClass::constantTimeEquals(const String & string1, // Preliminary check if(string1.length() != string2.length()) { - return false; + return false; } // Evaluates every character From 1aad21ffefce4c9d64edfb6f94ad15698e2dee7d Mon Sep 17 00:00:00 2001 From: Alessio Leoncini Date: Wed, 15 Nov 2017 14:49:54 +0100 Subject: [PATCH 4/9] Moved string comnparison in constant time to String class; modified function body to assure constant time comparison despite compiler optimizations --- cores/esp8266/WString.cpp | 25 +++++++++++++++++++++++++ cores/esp8266/WString.h | 1 + libraries/ArduinoOTA/ArduinoOTA.cpp | 23 +---------------------- libraries/ArduinoOTA/ArduinoOTA.h | 1 - 4 files changed, 27 insertions(+), 23 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index acd62ce9bf..d0ae55e606 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -477,6 +477,31 @@ unsigned char String::equalsIgnoreCase(const String &s2) const { return 1; } +unsigned char String::equalsConstantTime(const String &s2) const { + // To avoid possible time-based attacks present function + // compares given strings in a constant time. + if(len != s2.len) + return 0; + if(len == 0) + return 1; + const char *p1 = buffer; + const char *p2 = s2.buffer; + unsigned int u1 = 0; + unsigned int u2 = 0; + while(*p1) { + if(*p1 == *p2) + ++u1; + else + ++u2; + ++p1; + ++p2; + } + if(u2 == 0) + return 0; + else + return 1; +} + unsigned char String::startsWith(const String &s2) const { if(len < s2.len) return 0; diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index a3bb40c130..fbf3c59b2f 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -194,6 +194,7 @@ class String { unsigned char operator <=(const String &rhs) const; unsigned char operator >=(const String &rhs) const; unsigned char equalsIgnoreCase(const String &s) const; + unsigned char equalsConstantTime(const String &s) const; unsigned char startsWith(const String &prefix) const; unsigned char startsWith(const String &prefix, unsigned int offset) const; unsigned char endsWith(const String &suffix) const; diff --git a/libraries/ArduinoOTA/ArduinoOTA.cpp b/libraries/ArduinoOTA/ArduinoOTA.cpp index 1776588195..aaf4c5ff53 100644 --- a/libraries/ArduinoOTA/ArduinoOTA.cpp +++ b/libraries/ArduinoOTA/ArduinoOTA.cpp @@ -229,7 +229,7 @@ void ArduinoOTAClass::_onRx(){ String result = _challengemd5.toString(); ota_ip.addr = (uint32_t)_ota_ip; - if(constantTimeEquals(result, response)){ + if(result.equalsConstantTime(response)) { _state = OTA_RUNUPDATE; } else { _udp_ota->append("Authentication Failed", 21); @@ -358,27 +358,6 @@ int ArduinoOTAClass::getCommand() { return _cmd; } -// To avoid possible time-based attacks present function compares given -// strings in a constant time. -bool ArduinoOTAClass::constantTimeEquals(const String & string1, - const String & string2) { - - // Preliminary check - if(string1.length() != string2.length()) { - return false; - } - - // Evaluates every character - bool equals = true; - unsigned int len = string1.length(); - for(unsigned int i = 0; i < len; i++){ - equals &= (string1.charAt(i) == string2.charAt(i)); - } - - // Return result - return equals; -} - #if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_ARDUINOOTA) ArduinoOTAClass ArduinoOTA; #endif diff --git a/libraries/ArduinoOTA/ArduinoOTA.h b/libraries/ArduinoOTA/ArduinoOTA.h index b9b3d7b1be..9785d18f89 100644 --- a/libraries/ArduinoOTA/ArduinoOTA.h +++ b/libraries/ArduinoOTA/ArduinoOTA.h @@ -93,7 +93,6 @@ class ArduinoOTAClass void _onRx(void); int parseInt(void); String readStringUntil(char end); - bool constantTimeEquals(const String &string1, const String &string2); }; #if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_ARDUINOOTA) From aca28323febfa40244faaf18eba6629de923160c Mon Sep 17 00:00:00 2001 From: Alessio Leoncini Date: Wed, 15 Nov 2017 14:59:30 +0100 Subject: [PATCH 5/9] Removed wrong code --- libraries/ArduinoOTA/ArduinoOTA.cpp | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/libraries/ArduinoOTA/ArduinoOTA.cpp b/libraries/ArduinoOTA/ArduinoOTA.cpp index 6b21aeede8..aaf4c5ff53 100644 --- a/libraries/ArduinoOTA/ArduinoOTA.cpp +++ b/libraries/ArduinoOTA/ArduinoOTA.cpp @@ -358,30 +358,6 @@ int ArduinoOTAClass::getCommand() { return _cmd; } -<<<<<<< HEAD -======= -// To avoid possible time-based attacks present function compares given -// strings in a constant time. -bool ArduinoOTAClass::constantTimeEquals(const String & string1, - const String & string2) { - - // Preliminary check - if(string1.length() != string2.length()) { - return false; - } - - // Evaluates every character - bool equals = true; - unsigned int len = string1.length(); - for(unsigned int i = 0; i < len; i++){ - equals &= (string1.charAt(i) == string2.charAt(i)); - } - - // Return result - return equals; -} - ->>>>>>> fe324cccfa483042104f244411c5611e06039192 #if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_ARDUINOOTA) ArduinoOTAClass ArduinoOTA; #endif From b378b6581722220faa136151a32c7917bd99f793 Mon Sep 17 00:00:00 2001 From: Alessio Leoncini Date: Wed, 15 Nov 2017 15:22:11 +0100 Subject: [PATCH 6/9] Fixed error and prevented compiler optimization to delete u1 local variable --- cores/esp8266/WString.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index d0ae55e606..4aec0399cf 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -496,10 +496,10 @@ unsigned char String::equalsConstantTime(const String &s2) const { ++p1; ++p2; } - if(u2 == 0) - return 0; - else + if((u1 == len) && (u2 == 0)) return 1; + else + return 0; } unsigned char String::startsWith(const String &s2) const { From f5df4755a5ec3d0a20e517d0827ca1fbced25bee Mon Sep 17 00:00:00 2001 From: Alessio Leoncini Date: Wed, 15 Nov 2017 16:46:15 +0100 Subject: [PATCH 7/9] Avoid timing attacks on string comparison --- libraries/ESP8266WebServer/src/ESP8266WebServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/ESP8266WebServer/src/ESP8266WebServer.cpp b/libraries/ESP8266WebServer/src/ESP8266WebServer.cpp index 6220dfe4df..2c56b5a9ab 100644 --- a/libraries/ESP8266WebServer/src/ESP8266WebServer.cpp +++ b/libraries/ESP8266WebServer/src/ESP8266WebServer.cpp @@ -119,7 +119,7 @@ bool ESP8266WebServer::authenticate(const char * username, const char * password return false; } sprintf(toencode, "%s:%s", username, password); - if(base64_encode_chars(toencode, toencodeLen, encoded) > 0 && authReq.equals(encoded)){ + if(base64_encode_chars(toencode, toencodeLen, encoded) > 0 && authReq.equalsConstantTime(encoded)){ authReq = String(); delete[] toencode; delete[] encoded; From 0322c9594df1637057902e247a3c140c07c7909e Mon Sep 17 00:00:00 2001 From: Alessio Leoncini Date: Thu, 16 Nov 2017 09:15:19 +0100 Subject: [PATCH 8/9] Minor --- libraries/ESP8266WebServer/src/ESP8266WebServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/ESP8266WebServer/src/ESP8266WebServer.cpp b/libraries/ESP8266WebServer/src/ESP8266WebServer.cpp index 2c56b5a9ab..6b97505ee1 100644 --- a/libraries/ESP8266WebServer/src/ESP8266WebServer.cpp +++ b/libraries/ESP8266WebServer/src/ESP8266WebServer.cpp @@ -119,7 +119,7 @@ bool ESP8266WebServer::authenticate(const char * username, const char * password return false; } sprintf(toencode, "%s:%s", username, password); - if(base64_encode_chars(toencode, toencodeLen, encoded) > 0 && authReq.equalsConstantTime(encoded)){ + if(base64_encode_chars(toencode, toencodeLen, encoded) > 0 && authReq.equalsConstantTime(encoded)) { authReq = String(); delete[] toencode; delete[] encoded; From c349b439d4cb81a33a566dc1d3dc11b576cbac0b Mon Sep 17 00:00:00 2001 From: Develo Date: Tue, 21 Nov 2017 00:08:22 -0300 Subject: [PATCH 9/9] changed counter names, removed else --- cores/esp8266/WString.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 4aec0399cf..6f2134e86f 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -482,24 +482,26 @@ unsigned char String::equalsConstantTime(const String &s2) const { // compares given strings in a constant time. if(len != s2.len) return 0; + //at this point lengths are the same if(len == 0) return 1; + //at this point lenghts are the same and non-zero const char *p1 = buffer; const char *p2 = s2.buffer; - unsigned int u1 = 0; - unsigned int u2 = 0; + unsigned int equalchars = 0; + unsigned int diffchars = 0; while(*p1) { if(*p1 == *p2) - ++u1; + ++equalchars; else - ++u2; + ++diffchars; ++p1; ++p2; } - if((u1 == len) && (u2 == 0)) - return 1; - else - return 0; + //the following should force a constant time eval of the condition without a compiler "logical shortcut" + unsigned char equalcond = (equalchars == len); + unsigned char diffcond = (diffchars == 0); + return (equalcond & diffcond); //bitwise AND } unsigned char String::startsWith(const String &s2) const {