From 9d84244156e8abd93a7c1d4326231bcd1abb909d Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 9 Sep 2016 15:27:42 +0200 Subject: C++ test update (compilation fix) Signed-off-by: Szczepan Zalega --- unittest/test.cc | 12 ++++++------ unittest/test_HOTP.cc | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/unittest/test.cc b/unittest/test.cc index 6267512..6744b45 100644 --- a/unittest/test.cc +++ b/unittest/test.cc @@ -18,7 +18,7 @@ std::string getSlotName(Stick10 &stick, int slotNo) { auto slot_req = get_payload(); slot_req.slot_number = slotNo; auto slot = ReadSlot::CommandTransaction::run(stick, slot_req); - std::string sName(reinterpret_cast(slot.slot_name)); + std::string sName(reinterpret_cast(slot.data().slot_name)); return sName; } @@ -47,18 +47,18 @@ TEST_CASE("Slot names are correct", "[slotNames]") { { auto resp = GetPasswordRetryCount::CommandTransaction::run(stick); - REQUIRE(resp.password_retry_count == 3); + REQUIRE(resp.data().password_retry_count == 3); } { auto resp = GetUserPasswordRetryCount::CommandTransaction::run(stick); - REQUIRE(resp.password_retry_count == 3); + REQUIRE(resp.data().password_retry_count == 3); } { auto slot = get_payload(); slot.slot_number = 0; auto resp2 = GetPasswordSafeSlotName::CommandTransaction::run(stick, slot); - std::string sName(reinterpret_cast(resp2.slot_name)); + std::string sName(reinterpret_cast(resp2.data().slot_name)); REQUIRE(sName == std::string("web1")); } @@ -67,7 +67,7 @@ TEST_CASE("Slot names are correct", "[slotNames]") { slot.slot_number = 0; auto resp2 = GetPasswordSafeSlotPassword::CommandTransaction::run(stick, slot); - std::string sName(reinterpret_cast(resp2.slot_password)); + std::string sName(reinterpret_cast(resp2.data().slot_password)); REQUIRE(sName == std::string("pass1")); } @@ -75,7 +75,7 @@ TEST_CASE("Slot names are correct", "[slotNames]") { auto slot = get_payload(); slot.slot_number = 0; auto resp2 = GetPasswordSafeSlotLogin::CommandTransaction::run(stick, slot); - std::string sName(reinterpret_cast(resp2.slot_login)); + std::string sName(reinterpret_cast(resp2.data().slot_login)); REQUIRE(sName == std::string("login1")); } diff --git a/unittest/test_HOTP.cc b/unittest/test_HOTP.cc index f25bad4..d31df55 100644 --- a/unittest/test_HOTP.cc +++ b/unittest/test_HOTP.cc @@ -84,7 +84,7 @@ TEST_CASE("Test HOTP codes according to RFC", "[HOTP]") { auto gh = get_payload(); gh.slot_number = 0x10; auto resp = GetHOTP::CommandTransaction::run(stick, gh); - REQUIRE( resp.code == code); + REQUIRE( resp.data().code == code); } //checking slot programmed before with nitro-app /* -- cgit v1.2.3 From 77ea27f25165302491a693051bea05c67e6dfbed Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 9 Sep 2016 10:18:46 +0200 Subject: Add hex to binary converting function Signed-off-by: Szczepan Zalega --- include/misc.h | 4 +++- misc.cc | 26 ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/include/misc.h b/include/misc.h index cccd830..5fcd16d 100644 --- a/include/misc.h +++ b/include/misc.h @@ -2,6 +2,7 @@ #define MISC_H #include #include +#include namespace nitrokey { namespace misc { @@ -16,7 +17,8 @@ typename T::CommandPayload get_payload(){ std::string hexdump(const char *p, size_t size, bool print_header=true); -uint32_t stm_crc32(const uint8_t *data, size_t size); + uint32_t stm_crc32(const uint8_t *data, size_t size); + std::vector hex_string_to_byte(const char* hexString); } } diff --git a/misc.cc b/misc.cc index d004d0f..5d7c387 100644 --- a/misc.cc +++ b/misc.cc @@ -2,10 +2,36 @@ #include #include "misc.h" #include "inttypes.h" +#include +#include +#include namespace nitrokey { namespace misc { +std::vector hex_string_to_byte(const char* hexString){ + const size_t s_size = strlen(hexString); + const size_t d_size = (s_size+1)/2; // add 1 for odd, ignore for even + assert(s_size%2==0); + assert(s_size<256); //arbitrary 'big' number + auto data = std::vector(d_size, 0); + + char buf[2]; + for(int i=0; i Date: Sat, 10 Sep 2016 10:50:59 +0200 Subject: Assume secret is coded in hex for OTP slot write #31 Signed-off-by: Szczepan Zalega --- NitrokeyManager.cc | 12 ++++++++++-- unittest/test_bindings.py | 11 +++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/NitrokeyManager.cc b/NitrokeyManager.cc index 5b648b5..c88f717 100644 --- a/NitrokeyManager.cc +++ b/NitrokeyManager.cc @@ -157,6 +157,12 @@ namespace nitrokey{ return erase_slot(slot_number, temporary_password); } + #include + template + void vector_copy(T& dest, std::vector vec){ + assert(sizeof(dest)>=vec.size()); + std::copy(vec.begin(), vec.end(), dest); + } bool NitrokeyManager::write_HOTP_slot(uint8_t slot_number, const char *slot_name, const char *secret, uint8_t hotp_counter, bool use_8_digits, bool use_enter, bool use_tokenID, const char *token_ID, @@ -166,7 +172,8 @@ namespace nitrokey{ slot_number = get_internal_slot_number_for_hotp(slot_number); auto payload = get_payload(); payload.slot_number = slot_number; - strcpyT(payload.slot_secret, secret); + auto secret_bin = misc::hex_string_to_byte(secret); + vector_copy(payload.slot_secret, secret_bin); strcpyT(payload.slot_name, slot_name); strcpyT(payload.slot_token_id, token_ID); payload.slot_counter = hotp_counter; @@ -188,7 +195,8 @@ namespace nitrokey{ slot_number = get_internal_slot_number_for_totp(slot_number); payload.slot_number = slot_number; - strcpyT(payload.slot_secret, secret); + auto secret_bin = misc::hex_string_to_byte(secret); + vector_copy(payload.slot_secret, secret_bin); strcpyT(payload.slot_name, slot_name); strcpyT(payload.slot_token_id, token_ID); payload.slot_interval = time_window; //FIXME naming diff --git a/unittest/test_bindings.py b/unittest/test_bindings.py index eeda247..377203e 100644 --- a/unittest/test_bindings.py +++ b/unittest/test_bindings.py @@ -5,8 +5,15 @@ from enum import Enum ffi = cffi.FFI() gs = ffi.string -RFC_SECRET = '12345678901234567890' +def to_hex(s): + return "".join("{:02x}".format(ord(c)) for c in s) + + +RFC_SECRET_HR = '12345678901234567890' +RFC_SECRET = to_hex(RFC_SECRET_HR) #'12345678901234567890' + +# print( repr((RFC_SECRET, RFC_SECRET_, len(RFC_SECRET))) ) class DefaultPasswords(Enum): ADMIN = '12345678' @@ -214,7 +221,7 @@ def test_invalid_slot(C): invalid_slot = 255 assert C.NK_erase_totp_slot(invalid_slot, 'some password') == LibraryErrors.INVALID_SLOT assert C.NK_write_hotp_slot(invalid_slot, 'long_test', RFC_SECRET, 0, False, False, False, "", - 'aaa') == LibraryErrors.INVALID_SLOT + 'aaa') == LibraryErrors.INVALID_SLOT assert C.NK_get_hotp_code_PIN(invalid_slot, 'some password') == 0 assert C.NK_get_last_command_status() == LibraryErrors.INVALID_SLOT assert C.NK_erase_password_safe_slot(invalid_slot) == LibraryErrors.INVALID_SLOT -- cgit v1.2.3 From a46491a97da08e495c92bba8046426678b5564f7 Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 9 Sep 2016 16:42:31 +0200 Subject: Remove asserts in favor of exceptions or warnings. Test changes in Python. On possible data truncation return LibraryError(exception) instead of silently truncating and logging warning Signed-off-by: Szczepan Zalega --- NitrokeyManager.cc | 10 +++++++--- include/LibraryException.h | 41 +++++++++++++++++++++++++++++++++++++++++ misc.cc | 14 ++++++++------ unittest/test_bindings.py | 24 +++++++++++++++++++++++- 4 files changed, 79 insertions(+), 10 deletions(-) diff --git a/NitrokeyManager.cc b/NitrokeyManager.cc index c88f717..d827292 100644 --- a/NitrokeyManager.cc +++ b/NitrokeyManager.cc @@ -2,6 +2,7 @@ #include #include "include/NitrokeyManager.h" #include "include/LibraryException.h" +#include namespace nitrokey{ @@ -157,10 +158,13 @@ namespace nitrokey{ return erase_slot(slot_number, temporary_password); } - #include template - void vector_copy(T& dest, std::vector vec){ - assert(sizeof(dest)>=vec.size()); + void vector_copy(T& dest, std::vector &vec){ + const size_t d_size = sizeof(dest); + if(d_size < vec.size()){ + throw TargetBufferSmallerThanSource(vec.size(), d_size); + } + std::fill(dest, dest+d_size, 0); std::copy(vec.begin(), vec.end(), dest); } diff --git a/include/LibraryException.h b/include/LibraryException.h index 5036320..72891fb 100644 --- a/include/LibraryException.h +++ b/include/LibraryException.h @@ -12,6 +12,47 @@ public: +class TargetBufferSmallerThanSource: public LibraryException { +public: + virtual uint8_t exception_id() override { + return 203; + } + +public: + size_t source_size; + size_t target_size; + + TargetBufferSmallerThanSource( + size_t source_size, size_t target_size + ) : source_size(source_size), target_size(target_size) {} + + virtual const char *what() const throw() override { + std::string s = " "; + auto ts = [](int x){ return std::to_string(x); }; + std::string msg = std::string("Target buffer size is smaller than source: [source size, buffer size]") + +s+ ts(source_size) +s+ ts(target_size); + return msg.c_str(); + } + +}; + +class InvalidHexString : public LibraryException { +public: + virtual uint8_t exception_id() override { + return 202; + } + +public: + uint8_t invalid_char; + + InvalidHexString (uint8_t invalid_char) : invalid_char( invalid_char) {} + + virtual const char *what() const throw() override { + return "Invalid character in hex string"; + } + +}; + class InvalidSlotException : public LibraryException { public: virtual uint8_t exception_id() override { diff --git a/misc.cc b/misc.cc index 5d7c387..9b339cd 100644 --- a/misc.cc +++ b/misc.cc @@ -4,25 +4,27 @@ #include "inttypes.h" #include #include -#include +#include "LibraryException.h" namespace nitrokey { namespace misc { std::vector hex_string_to_byte(const char* hexString){ + const size_t big_string_size = 256; //arbitrary 'big' number const size_t s_size = strlen(hexString); - const size_t d_size = (s_size+1)/2; // add 1 for odd, ignore for even - assert(s_size%2==0); - assert(s_size<256); //arbitrary 'big' number + const size_t d_size = s_size/2; + if (s_size%2!=0 || s_size==0 || s_size>big_string_size){ + throw InvalidHexString(0); + } auto data = std::vector(d_size, 0); char buf[2]; for(int i=0; i 0 print(('Serial number of the device: ', sn)) + + +@pytest.mark.parametrize("invalid_hex_string", + ['text', '00 ', '0xff', 'zzzzzzzzzzzz', 'fff', '', 'f' * 257, 'f' * 258]) +def test_invalid_secret_hex_string_for_OTP_write(C, invalid_hex_string): + """ + Tests for invalid secret hex string during writing to OTP slot. Invalid strings are not hexadecimal number, + empty or longer than 255 characters. + """ + assert C.NK_write_hotp_slot(1, 'slot_name', invalid_hex_string, 0, True, False, False, '', + DefaultPasswords.ADMIN_TEMP) == LibraryErrors.INVALID_HEX_STRING + assert C.NK_write_totp_slot(1, 'python_test', invalid_hex_string, 30, True, False, False, "", + DefaultPasswords.ADMIN_TEMP) == LibraryErrors.INVALID_HEX_STRING + + +def test_warning_binary_bigger_than_secret_buffer(C): + invalid_hex_string = to_hex('1234567890') * 3 + assert C.NK_write_hotp_slot(1, 'slot_name', invalid_hex_string, 0, True, False, False, '', + DefaultPasswords.ADMIN_TEMP) == LibraryErrors.TARGET_BUFFER_SIZE_SMALLER_THAN_SOURCE -- cgit v1.2.3