From 7b35b58ae671ac7da66c3407923c3b032fce22c2 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 17 Jul 2014 01:51:59 -0400 Subject: [PATCH] Fix EVP_DecodeBlock and add tests. Another signedness error. Leave a TODO to possibly resolve EVP_DecodeBlock's ignoring padding. Document some of the Init/Update/Finish versions' behavior. Change-Id: I78a72c3163f8543172a7008b2d09fb10e003d957 Reviewed-on: https://boringssl-review.googlesource.com/1230 Reviewed-by: Adam Langley --- crypto/base64/CMakeLists.txt | 8 +++ crypto/base64/base64.c | 3 +- crypto/base64/base64_test.c | 104 +++++++++++++++++++++++++++++++++++ include/openssl/base64.h | 20 +++++-- util/all_tests.sh | 1 + 5 files changed, 130 insertions(+), 6 deletions(-) create mode 100644 crypto/base64/base64_test.c diff --git a/crypto/base64/CMakeLists.txt b/crypto/base64/CMakeLists.txt index 2601abd9..dec67ea2 100644 --- a/crypto/base64/CMakeLists.txt +++ b/crypto/base64/CMakeLists.txt @@ -7,3 +7,11 @@ add_library( base64.c ) + +add_executable( + base64_test + + base64_test.c +) + +target_link_libraries(base64_test crypto) diff --git a/crypto/base64/base64.c b/crypto/base64/base64.c index 58e6151b..f5525b1b 100644 --- a/crypto/base64/base64.c +++ b/crypto/base64/base64.c @@ -63,6 +63,7 @@ static const unsigned char data_bin2ascii[65] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; #define conv_bin2ascii(a) (data_bin2ascii[(a) & 0x3f]) +/* TODO(davidben): This doesn't error on bytes above 127. */ #define conv_ascii2bin(a) (data_ascii2bin[(a) & 0x7f]) /* 64 char lines @@ -357,7 +358,7 @@ int EVP_DecodeFinal(EVP_ENCODE_CTX *ctx, uint8_t *out, int *outl) { } } -size_t EVP_DecodeBlock(uint8_t *dst, const uint8_t *src, size_t src_len) { +ssize_t EVP_DecodeBlock(uint8_t *dst, const uint8_t *src, size_t src_len) { int a, b, c, d; uint32_t l; size_t i, ret = 0; diff --git a/crypto/base64/base64_test.c b/crypto/base64/base64_test.c new file mode 100644 index 00000000..ea0d3d14 --- /dev/null +++ b/crypto/base64/base64_test.c @@ -0,0 +1,104 @@ +/* Copyright (c) 2014, Google Inc. + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY + * SPECIAL, DIRECT, 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 +#include + +#include +#include + +typedef struct { + const char *decoded; + const char *encoded; +} TEST_VECTOR; + +/* Test vectors from RFC 4648. */ +static const TEST_VECTOR test_vectors[] = { + { "", "" }, + { "f" , "Zg==" }, + { "fo", "Zm8=" }, + { "foo", "Zm9v" }, + { "foob", "Zm9vYg==" }, + { "fooba", "Zm9vYmE=" }, + { "foobar", "Zm9vYmFy" }, +}; +static const size_t kNumTests = sizeof(test_vectors) / sizeof(test_vectors[0]); + +static int test_encode() { + uint8_t out[8]; + size_t i; + ssize_t len; + + for (i = 0; i < kNumTests; i++) { + const TEST_VECTOR *t = &test_vectors[i]; + len = EVP_EncodeBlock(out, (const uint8_t*)t->decoded, strlen(t->decoded)); + if (len != strlen(t->encoded) || + memcmp(out, t->encoded, len) != 0) { + fprintf(stderr, "encode(\"%s\") = \"%.*s\", want \"%s\"\n", + t->decoded, (int)len, (const char*)out, t->encoded); + return 0; + } + } + return 1; +} + +static int test_decode() { + uint8_t out[6]; + size_t i; + ssize_t len; + + for (i = 0; i < kNumTests; i++) { + const TEST_VECTOR *t = &test_vectors[i]; + size_t expected_len = strlen(t->decoded); + len = EVP_DecodeBlock(out, (const uint8_t*)t->encoded, strlen(t->encoded)); + /* TODO(davidben): EVP_DecodeBlock doesn't take padding into account. Is + * this behavior we can change? */ + if (expected_len % 3 != 0) { + len -= 3 - (expected_len % 3); + } + if (len != strlen(t->decoded) || + memcmp(out, t->decoded, len) != 0) { + fprintf(stderr, "decode(\"%s\") = \"%.*s\", want \"%s\"\n", + t->encoded, (int)len, (const char*)out, t->decoded); + return 0; + } + } + + if (EVP_DecodeBlock(out, (const uint8_t*)"a!bc", 4) >= 0) { + fprintf(stderr, "Failed to reject invalid characters in the middle.\n"); + return 0; + } + + if (EVP_DecodeBlock(out, (const uint8_t*)"abc", 3) >= 0) { + fprintf(stderr, "Failed to reject invalid input length.\n"); + return 0; + } + + return 1; +} + +int main() { + ERR_load_crypto_strings(); + + if (!test_encode()) { + return 1; + } + + if (!test_decode()) { + return 1; + } + + printf("PASS\n"); + return 0; +} diff --git a/include/openssl/base64.h b/include/openssl/base64.h index afafaefa..c4d312c6 100644 --- a/include/openssl/base64.h +++ b/include/openssl/base64.h @@ -75,8 +75,12 @@ typedef struct evp_encode_ctx_st EVP_ENCODE_CTX; /* Encoding */ -/* EVP_EncodeInit initialises |*ctx|, which is typically stack allocated, for - * an encoding operation. */ +/* EVP_EncodeInit initialises |*ctx|, which is typically stack + * allocated, for an encoding operation. + * + * NOTE: The encoding operation breaks its output with newlines every + * 64 characters of output (48 characters of input). Use + * EVP_EncodeBlock to encode raw base64. */ void EVP_EncodeInit(EVP_ENCODE_CTX *ctx); /* EVP_EncodeUpdate encodes |in_len| bytes from |in| and writes an encoded @@ -98,7 +102,10 @@ size_t EVP_EncodeBlock(uint8_t *dst, const uint8_t *src, size_t src_len); /* Decoding */ /* EVP_DecodeInit initialises |*ctx|, which is typically stack allocated, for - * a decoding operation. */ + * a decoding operation. + * + * TODO(davidben): This isn't a straight-up base64 decode either. Document + * and/or fix exactly what's going on here; maximum line length and such. */ void EVP_DecodeInit(EVP_ENCODE_CTX *ctx); /* EVP_DecodeUpdate decodes |in_len| bytes from |in| and writes the decoded @@ -117,8 +124,11 @@ int EVP_DecodeUpdate(EVP_ENCODE_CTX *ctx, uint8_t *out, int *out_len, int EVP_DecodeFinal(EVP_ENCODE_CTX *ctx, uint8_t *out, int *out_len); /* EVP_DecodeBlock encodes |src_len| bytes from |src| and writes the result to - * |dst|. It returns the number of bytes written. */ -size_t EVP_DecodeBlock(uint8_t *dst, const uint8_t *src, size_t src_len); + * |dst|. It returns the number of bytes written or -1 on error. + * + * WARNING: EVP_DecodeBlock's return value does not take padding into + * account. TODO(davidben): Possible or worth it to fix or add new API? */ +ssize_t EVP_DecodeBlock(uint8_t *dst, const uint8_t *src, size_t src_len); struct evp_encode_ctx_st { diff --git a/util/all_tests.sh b/util/all_tests.sh index def97ff7..a7e961e4 100644 --- a/util/all_tests.sh +++ b/util/all_tests.sh @@ -19,6 +19,7 @@ TESTS=" ./crypto/cipher/aead_test aes-256-gcm ../crypto/cipher/aes_256_gcm_tests.txt ./crypto/cipher/aead_test chacha20-poly1305 ../crypto/cipher/chacha20_poly1305_tests.txt ./crypto/cipher/aead_test rc4-md5 ../crypto/cipher/rc4_md5_tests.txt +./crypto/base64/base64_test ./crypto/bio/bio_test ./crypto/bn/bn_test ./crypto/cipher/cipher_test ../crypto/cipher/cipher_test.txt