From 9f1d0201ec66cb8e6a2e6c457f6f074e0233068a Mon Sep 17 00:00:00 2001 From: Hardening Date: Sun, 11 May 2014 22:42:01 +0200 Subject: [PATCH] Changes for base64 This patch changes the prototype for decode_base64 so that the encode / decode method are consistant (encode(BYTE *) => char* and decode(char*) => BYTE*). It also does some improvements with unrolling loops so that end conditions are tested only at the end. The patch also adds some unitary tests. Before the patch base64_decode() made valgrind complain about uninitialized bits, after valgrind is happy and very quiet. --- client/common/cmdline.c | 4 +- include/freerdp/crypto/crypto.h | 2 +- libfreerdp/core/gateway/ncacn_http.c | 4 +- libfreerdp/crypto/CMakeLists.txt | 4 + libfreerdp/crypto/base64.c | 139 ++++++++++++++++++++++------------ libfreerdp/crypto/test/CMakeLists.txt | 31 ++++++++ libfreerdp/crypto/test/TestBase64.c | 110 +++++++++++++++++++++++++++ 7 files changed, 242 insertions(+), 52 deletions(-) create mode 100644 libfreerdp/crypto/test/CMakeLists.txt create mode 100644 libfreerdp/crypto/test/TestBase64.c diff --git a/client/common/cmdline.c b/client/common/cmdline.c index d26206f..85847b3 100644 --- a/client/common/cmdline.c +++ b/client/common/cmdline.c @@ -1772,8 +1772,8 @@ int freerdp_client_settings_parse_command_line_arguments(rdpSettings* settings, { BYTE *base64; int length; - crypto_base64_decode((BYTE *) (arg->Value), - (int) strlen(arg->Value), &base64, &length); + crypto_base64_decode((const char *) (arg->Value), (int) strlen(arg->Value), + &base64, &length); if ((base64 != NULL) && (length == sizeof(ARC_SC_PRIVATE_PACKET))) { memcpy(settings->ServerAutoReconnectCookie, base64, length); diff --git a/include/freerdp/crypto/crypto.h b/include/freerdp/crypto/crypto.h index a022dde..e642deb 100644 --- a/include/freerdp/crypto/crypto.h +++ b/include/freerdp/crypto/crypto.h @@ -144,6 +144,6 @@ FREERDP_API void crypto_reverse(BYTE* data, int length); FREERDP_API void crypto_nonce(BYTE* nonce, int size); FREERDP_API char* crypto_base64_encode(const BYTE* data, int length); -FREERDP_API void crypto_base64_decode(const BYTE* enc_data, int length, BYTE** dec_data, int* res_length); +FREERDP_API void crypto_base64_decode(const char* enc_data, int length, BYTE** dec_data, int* res_length); #endif /* FREERDP_CRYPTO_H */ diff --git a/libfreerdp/core/gateway/ncacn_http.c b/libfreerdp/core/gateway/ncacn_http.c index 76e76d6..270dafb 100644 --- a/libfreerdp/core/gateway/ncacn_http.c +++ b/libfreerdp/core/gateway/ncacn_http.c @@ -106,7 +106,7 @@ int rpc_ncacn_http_recv_in_channel_response(rdpRpc* rpc) goto out; ntlm_token_data = NULL; - crypto_base64_decode((BYTE*) token64, strlen(token64), &ntlm_token_data, &ntlm_token_length); + crypto_base64_decode(token64, strlen(token64), &ntlm_token_data, &ntlm_token_length); } ntlm->inputBuffer[0].pvBuffer = ntlm_token_data; @@ -238,7 +238,7 @@ int rpc_ncacn_http_recv_out_channel_response(rdpRpc* rpc) if (http_response && ListDictionary_Contains(http_response->Authenticates, "NTLM")) { char *token64 = ListDictionary_GetItemValue(http_response->Authenticates, "NTLM"); - crypto_base64_decode((BYTE*) token64, strlen(token64), &ntlm_token_data, &ntlm_token_length); + crypto_base64_decode(token64, strlen(token64), &ntlm_token_data, &ntlm_token_length); } ntlm->inputBuffer[0].pvBuffer = ntlm_token_data; diff --git a/libfreerdp/crypto/CMakeLists.txt b/libfreerdp/crypto/CMakeLists.txt index e0f93fc..e33200b 100644 --- a/libfreerdp/crypto/CMakeLists.txt +++ b/libfreerdp/crypto/CMakeLists.txt @@ -71,3 +71,7 @@ else() endif() set_property(TARGET ${MODULE_NAME} PROPERTY FOLDER "FreeRDP/libfreerdp") + +if(BUILD_TESTING) + add_subdirectory(test) +endif() diff --git a/libfreerdp/crypto/base64.c b/libfreerdp/crypto/base64.c index 3f2b617..d492e89 100644 --- a/libfreerdp/crypto/base64.c +++ b/libfreerdp/crypto/base64.c @@ -34,38 +34,56 @@ char* crypto_base64_encode(const BYTE* data, int length) char* p; char* ret; int i = 0; + int blocks; q = data; p = ret = (char*) malloc((length + 3) * 4 / 3 + 1); if (!p) return NULL; - while (i < length) + /* b1, b2, b3 are input bytes + * + * 0 1 2 + * 012345678901234567890123 + * | b1 | b2 | b3 | + * + * [ c1 ] [ c3 ] + * [ c2 ] [ c4 ] + * + * c1, c2, c3, c4 are output chars in base64 + */ + + /* first treat complete blocks */ + blocks = length - (length % 3); + for (i = 0; i < blocks; i += 3, q += 3) { - c = q[i++]; - - c <<= 8; - if (i < length) - c += q[i]; - i++; - - c <<= 8; - if (i < length) - c += q[i]; - i++; + c = (q[0] << 16) + (q[1] << 8) + q[2]; *p++ = base64[(c & 0x00FC0000) >> 18]; *p++ = base64[(c & 0x0003F000) >> 12]; + *p++ = base64[(c & 0x00000FC0) >> 6]; + *p++ = base64[c & 0x0000003F]; + } - if (i > length + 1) - *p++ = '='; - else - *p++ = base64[(c & 0x00000FC0) >> 6]; - - if (i > length) - *p++ = '='; - else - *p++ = base64[c & 0x0000003F]; + /* then remainder */ + switch (length % 3) + { + case 0: + break; + case 1: + c = (q[0] << 16); + *p++ = base64[(c & 0x00FC0000) >> 18]; + *p++ = base64[(c & 0x0003F000) >> 12]; + *p++ = '='; + *p++ = '='; + break; + case 2: + c = (q[0] << 16) + (q[1] << 8); + *p++ = base64[(c & 0x00FC0000) >> 18]; + *p++ = base64[(c & 0x0003F000) >> 12]; + *p++ = base64[(c & 0x00000FC0) >> 6]; + *p++ = '='; + break; } *p = 0; @@ -96,55 +114,82 @@ static int base64_decode_char(char c) return -1; } -static void* base64_decode(const BYTE* s, int length, int* data_len) +static void* base64_decode(const char* s, int length, int* data_len) { - char* p; int n[4]; BYTE* q; BYTE* data; + int nBlocks, i, outputLen; if (length % 4) return NULL; - n[0] = n[1] = n[2] = n[3] = 0; q = data = (BYTE*) malloc(length / 4 * 3); - for (p = (char*) s; *p; ) + /* first treat complete blocks */ + nBlocks = (length / 4); + outputLen = 0; + + for (i = 0; i < nBlocks-1; i++, q += 3) { - n[0] = base64_decode_char(*p++); - n[1] = base64_decode_char(*p++); - n[2] = base64_decode_char(*p++); - n[3] = base64_decode_char(*p++); - - if ((n[0] == -1) || (n[1] == -1)) - { - free(data); - return NULL; - } - - if ((n[2] == -1) && (n[3] != -1)) - { - free(data); - return NULL; - } + n[0] = base64_decode_char(*s++); + n[1] = base64_decode_char(*s++); + n[2] = base64_decode_char(*s++); + n[3] = base64_decode_char(*s++); + + if ((n[0] == -1) || (n[1] == -1) || (n[2] == -1) || (n[3] == -1)) + goto out_free; q[0] = (n[0] << 2) + (n[1] >> 4); + q[1] = ((n[1] & 15) << 4) + (n[2] >> 2); + q[2] = ((n[2] & 3) << 6) + n[3]; + outputLen += 3; + } - if (n[2] != -1) - q[1] = ((n[1] & 15) << 4) + (n[2] >> 2); + /* treat last block */ + n[0] = base64_decode_char(*s++); + n[1] = base64_decode_char(*s++); + if ((n[0] == -1) || (n[1] == -1)) + goto out_free; + n[2] = base64_decode_char(*s++); + n[3] = base64_decode_char(*s++); + + q[0] = (n[0] << 2) + (n[1] >> 4); + if (n[2] == -1) + { + /* XX== */ + outputLen += 1; if (n[3] != -1) - q[2] = ((n[2] & 3) << 6) + n[3]; + goto out_free; - q += 3; + q[1] = ((n[1] & 15) << 4); + } + else if (n[3] == -1) + { + /* yyy= */ + outputLen += 2; + q[1] = ((n[1] & 15) << 4) + (n[2] >> 2); + q[2] = ((n[2] & 3) << 6); + } + else + { + /* XXXX */ + outputLen += 3; + q[0] = (n[0] << 2) + (n[1] >> 4); + q[1] = ((n[1] & 15) << 4) + (n[2] >> 2); + q[2] = ((n[2] & 3) << 6) + n[3]; } - *data_len = q - data - (n[2] == -1) - (n[3] == -1); + *data_len = outputLen; return data; +out_free: + free(data); + return NULL; } -void crypto_base64_decode(const BYTE* enc_data, int length, BYTE** dec_data, int* res_length) +void crypto_base64_decode(const char* enc_data, int length, BYTE** dec_data, int* res_length) { *dec_data = base64_decode(enc_data, length, res_length); } diff --git a/libfreerdp/crypto/test/CMakeLists.txt b/libfreerdp/crypto/test/CMakeLists.txt new file mode 100644 index 0000000..4333b01 --- /dev/null +++ b/libfreerdp/crypto/test/CMakeLists.txt @@ -0,0 +1,31 @@ + +set(MODULE_NAME "TestFreeRDPCrypto") +set(MODULE_PREFIX "TEST_FREERDP_CRYPTO") + +set(${MODULE_PREFIX}_DRIVER ${MODULE_NAME}.c) + +set(${MODULE_PREFIX}_TESTS + TestBase64.c) + +create_test_sourcelist(${MODULE_PREFIX}_SRCS + ${${MODULE_PREFIX}_DRIVER} + ${${MODULE_PREFIX}_TESTS}) + +add_executable(${MODULE_NAME} ${${MODULE_PREFIX}_SRCS}) + +set_complex_link_libraries(VARIABLE ${MODULE_PREFIX}_LIBS + MONOLITHIC ${MONOLITHIC_BUILD} + MODULE freerdp + MODULES freerdp-crypto) + +target_link_libraries(${MODULE_NAME} ${${MODULE_PREFIX}_LIBS}) + +set_target_properties(${MODULE_NAME} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${TESTING_OUTPUT_DIRECTORY}") + +foreach(test ${${MODULE_PREFIX}_TESTS}) + get_filename_component(TestName ${test} NAME_WE) + add_test(${TestName} ${TESTING_OUTPUT_DIRECTORY}/${MODULE_NAME} ${TestName}) +endforeach() + +set_property(TARGET ${MODULE_NAME} PROPERTY FOLDER "FreeRDP/Test") + diff --git a/libfreerdp/crypto/test/TestBase64.c b/libfreerdp/crypto/test/TestBase64.c new file mode 100644 index 0000000..5c32ea4 --- /dev/null +++ b/libfreerdp/crypto/test/TestBase64.c @@ -0,0 +1,110 @@ +/** + * Copyright © 2014 Thincast Technologies GmbH + * Copyright © 2014 Hardening + * + * Permission to use, copy, modify, distribute, and sell this software and + * its documentation for any purpose is hereby granted without fee, provided + * that the above copyright notice appear in all copies and that both that + * copyright notice and this permission notice appear in supporting + * documentation, and that the name of the copyright holders not be used in + * advertising or publicity pertaining to distribution of the software + * without specific, written prior permission. The copyright holders make + * no representations about the suitability of this software for any + * purpose. It is provided "as is" without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include + +struct Encode64test { + const char *input; + int len; + const char *output; +}; + +struct Encode64test encodeTests[] = { + {"\x00", 1, "AA=="}, + {"\x00\x00", 2, "AAA="}, + {"\x00\x00\x00", 3, "AAAA"}, + {"0123456", 7, "MDEyMzQ1Ng=="}, + {"90123456", 8, "OTAxMjM0NTY="}, + {"890123456", 9, "ODkwMTIzNDU2"}, + {"7890123456", 10, "Nzg5MDEyMzQ1Ng=="}, + + {NULL, -1, NULL}, /* /!\ last one /!\ */ +}; + + +int TestBase64(int argc, char* argv[]) +{ + int i, testNb = 0; + int outLen; + BYTE *decoded; + + testNb++; + fprintf(stderr, "%d:encode base64...", testNb); + + for (i = 0; encodeTests[i].input; i++) + { + char *encoded = crypto_base64_encode((const BYTE *)encodeTests[i].input, encodeTests[i].len); + + if (strcmp(encodeTests[i].output, encoded)) + { + fprintf(stderr, "ko, error for string %d\n", i); + return -1; + } + + free(encoded); + } + fprintf(stderr, "ok\n"); + + testNb++; + fprintf(stderr, "%d:decode base64...", testNb); + for (i = 0; encodeTests[i].input; i++) + { + crypto_base64_decode(encodeTests[i].output, strlen(encodeTests[i].output), &decoded, &outLen); + + if (!decoded || (outLen != encodeTests[i].len) || memcmp(encodeTests[i].input, decoded, outLen)) + { + fprintf(stderr, "ko, error for string %d\n", i); + return -1; + } + + free(decoded); + } + fprintf(stderr, "ok\n"); + + testNb++; + fprintf(stderr, "%d:decode base64 errors...", testNb); + crypto_base64_decode("000", 3, &decoded, &outLen); + if (decoded) + { + fprintf(stderr, "ko, badly padded string\n"); + return -1; + } + + crypto_base64_decode("0=00", 4, &decoded, &outLen); + if (decoded) + { + fprintf(stderr, "ko, = in a wrong place\n"); + return -1; + } + + crypto_base64_decode("00=0", 4, &decoded, &outLen); + if (decoded) + { + fprintf(stderr, "ko, = in a wrong place\n"); + return -1; + } + fprintf(stderr, "ok\n"); + + + return 0; +} -- 2.7.4