From a517daa56f5848d25ba79617a1a7b82ed2b0a7c0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 20 Dec 2018 08:36:25 -0500 Subject: [PATCH 1/3] base32_decode(): Return number of bytes written on success. This makes it consistent with base64_decode(). Closes ticket 28913. --- changes/ticket28913 | 4 ++++ src/feature/control/control.c | 3 ++- src/feature/rend/rendcache.c | 3 +-- src/lib/encoding/binascii.c | 4 ++-- src/test/test_crypto.c | 8 ++++---- src/test/test_util_format.c | 4 ++-- 6 files changed, 15 insertions(+), 11 deletions(-) create mode 100644 changes/ticket28913 diff --git a/changes/ticket28913 b/changes/ticket28913 new file mode 100644 index 0000000000..e09847464d --- /dev/null +++ b/changes/ticket28913 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Make the base32_decode() API return the number of bytes written, + for consistency with base64_decode(). + Closes ticket 28913. diff --git a/src/feature/control/control.c b/src/feature/control/control.c index 7fae3b7a1b..1e3db6337e 100644 --- a/src/feature/control/control.c +++ b/src/feature/control/control.c @@ -4428,7 +4428,8 @@ handle_control_hsfetch(control_connection_t *conn, uint32_t len, } else if (strcmpstart(arg1, v2_str) == 0 && rend_valid_descriptor_id(arg1 + v2_str_len) && base32_decode(digest, sizeof(digest), arg1 + v2_str_len, - REND_DESC_ID_V2_LEN_BASE32) == 0) { + REND_DESC_ID_V2_LEN_BASE32) == + REND_DESC_ID_V2_LEN_BASE32) { /* We have a well formed version 2 descriptor ID. Keep the decoded value * of the id. */ desc_id = digest; diff --git a/src/feature/rend/rendcache.c b/src/feature/rend/rendcache.c index b851e71959..ecd85e4a5a 100644 --- a/src/feature/rend/rendcache.c +++ b/src/feature/rend/rendcache.c @@ -854,7 +854,7 @@ rend_cache_store_v2_desc_as_client(const char *desc, *entry = NULL; } if (base32_decode(want_desc_id, sizeof(want_desc_id), - desc_id_base32, strlen(desc_id_base32)) != 0) { + desc_id_base32, strlen(desc_id_base32)) < 0) { log_warn(LD_BUG, "Couldn't decode base32 %s for descriptor id.", escaped_safe_str_client(desc_id_base32)); goto err; @@ -1005,4 +1005,3 @@ rend_cache_store_v2_desc_as_client(const char *desc, tor_free(intro_content); return retval; } - diff --git a/src/lib/encoding/binascii.c b/src/lib/encoding/binascii.c index 067db075ad..a7662658f0 100644 --- a/src/lib/encoding/binascii.c +++ b/src/lib/encoding/binascii.c @@ -84,7 +84,7 @@ base32_encode(char *dest, size_t destlen, const char *src, size_t srclen) } /** Implements base32 decoding as in RFC 4648. - * Returns 0 if successful, -1 otherwise. + * Return the number of bytes decoded if successful; -1 otherwise. */ int base32_decode(char *dest, size_t destlen, const char *src, size_t srclen) @@ -147,7 +147,7 @@ base32_decode(char *dest, size_t destlen, const char *src, size_t srclen) memset(tmp, 0, srclen); /* on the heap, this should be safe */ tor_free(tmp); tmp = NULL; - return 0; + return i; } #define BASE64_OPENSSL_LINELEN 64 diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index 81d2fa6f33..e924cea423 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -1865,13 +1865,13 @@ test_crypto_base32_decode(void *arg) /* Encode and decode a random string. */ base32_encode(encoded, 96 + 1, plain, 60); res = base32_decode(decoded, 60, encoded, 96); - tt_int_op(res,OP_EQ, 0); + tt_int_op(res, OP_EQ, 60); tt_mem_op(plain,OP_EQ, decoded, 60); /* Encode, uppercase, and decode a random string. */ base32_encode(encoded, 96 + 1, plain, 60); tor_strupper(encoded); res = base32_decode(decoded, 60, encoded, 96); - tt_int_op(res,OP_EQ, 0); + tt_int_op(res, OP_EQ, 60); tt_mem_op(plain,OP_EQ, decoded, 60); /* Change encoded string and decode. */ if (encoded[0] == 'A' || encoded[0] == 'a') @@ -1879,12 +1879,12 @@ test_crypto_base32_decode(void *arg) else encoded[0] = 'A'; res = base32_decode(decoded, 60, encoded, 96); - tt_int_op(res,OP_EQ, 0); + tt_int_op(res, OP_EQ, 60); tt_mem_op(plain,OP_NE, decoded, 60); /* Bad encodings. */ encoded[0] = '!'; res = base32_decode(decoded, 60, encoded, 96); - tt_int_op(0, OP_GT, res); + tt_int_op(res, OP_LT, 0); done: ; diff --git a/src/test/test_util_format.c b/src/test/test_util_format.c index fd57125b86..f91171dc28 100644 --- a/src/test/test_util_format.c +++ b/src/test/test_util_format.c @@ -346,7 +346,7 @@ test_util_format_base32_decode(void *arg) const char *src = "mjwgc2dcnrswqmjs"; ret = base32_decode(dst, strlen(expected), src, strlen(src)); - tt_int_op(ret, OP_EQ, 0); + tt_int_op(ret, OP_EQ, 10); tt_str_op(expected, OP_EQ, dst); } @@ -357,7 +357,7 @@ test_util_format_base32_decode(void *arg) const char *src = "mjwgc2dcnrswq"; ret = base32_decode(dst, strlen(expected), src, strlen(src)); - tt_int_op(ret, OP_EQ, 0); + tt_int_op(ret, OP_EQ, 8); tt_mem_op(expected, OP_EQ, dst, strlen(expected)); } From 22c5ad682cdd2e6a9a2124585302b2335361ab80 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 20 Dec 2018 08:37:04 -0500 Subject: [PATCH 2/3] Add base32 to the round-trip fuzzer --- src/test/fuzz/fuzz_strops.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/test/fuzz/fuzz_strops.c b/src/test/fuzz/fuzz_strops.c index 5da590acfa..cfc7fc5063 100644 --- a/src/test/fuzz/fuzz_strops.c +++ b/src/test/fuzz/fuzz_strops.c @@ -86,15 +86,13 @@ b16_enc(const chunk_t *inp) return ch; } -#if 0 static chunk_t * b32_dec(const chunk_t *inp) { chunk_t *ch = chunk_new(inp->len);//XXXX int r = base32_decode((char *)ch->buf, ch->len, (char *)inp->buf, inp->len); if (r >= 0) { - ch->len = r; // XXXX we need some way to get the actual length of - // XXXX the output here. + ch->len = r; } else { chunk_free(ch); } @@ -108,7 +106,6 @@ b32_enc(const chunk_t *inp) ch->len = strlen((char *) ch->buf); return ch; } -#endif static chunk_t * b64_dec(const chunk_t *inp) @@ -222,10 +219,7 @@ fuzz_main(const uint8_t *stdin_buf, size_t data_size) ENCODE_ROUNDTRIP(b16_enc, b16_dec, chunk_free_); break; case 1: - /* - XXXX see notes above about our base-32 functions. ENCODE_ROUNDTRIP(b32_enc, b32_dec, chunk_free_); - */ break; case 2: ENCODE_ROUNDTRIP(b64_enc, b64_dec, chunk_free_); From f632335feb27b45a3ee5eb64690826bda52467bd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 17 Jan 2019 13:32:19 -0500 Subject: [PATCH 3/3] Fix users of base32_decode to check for expected length in return. Also, when we log about a failure from base32_decode(), we now say that the length is wrong or that the characters were invalid: previously we would just say that there were invalid characters. Follow-up on 28913 work. --- src/feature/hs/hs_client.c | 5 ++++- src/feature/hs/hs_common.c | 3 ++- src/feature/hs/hs_service.c | 3 ++- src/feature/rend/rendcache.c | 7 ++++--- src/feature/rend/rendcommon.c | 5 +++-- src/feature/rend/rendparse.c | 17 +++++++++++------ 6 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index dfad216abb..a8a4aa776f 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -1517,7 +1517,10 @@ parse_auth_file_content(const char *client_key_str) auth = tor_malloc_zero(sizeof(hs_client_service_authorization_t)); if (base32_decode((char *) auth->enc_seckey.secret_key, sizeof(auth->enc_seckey.secret_key), - seckey_b32, strlen(seckey_b32)) < 0) { + seckey_b32, strlen(seckey_b32)) != + sizeof(auth->enc_seckey.secret_key)) { + log_warn(LD_REND, "Client authorization encoded base32 private key " + "can't be decoded: %s", seckey_b32); goto err; } strncpy(auth->onion_address, onion_address, HS_SERVICE_ADDR_LEN_BASE32); diff --git a/src/feature/hs/hs_common.c b/src/feature/hs/hs_common.c index 8dbd9485ea..97229ac11e 100644 --- a/src/feature/hs/hs_common.c +++ b/src/feature/hs/hs_common.c @@ -926,7 +926,8 @@ hs_parse_address(const char *address, ed25519_public_key_t *key_out, } /* Decode address so we can extract needed fields. */ - if (base32_decode(decoded, sizeof(decoded), address, strlen(address)) < 0) { + if (base32_decode(decoded, sizeof(decoded), address, strlen(address)) + != sizeof(decoded)) { log_warn(LD_REND, "Service address %s can't be decoded.", escaped_safe_str(address)); goto invalid; diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 623a239d50..6f6cf01053 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -1179,7 +1179,8 @@ parse_authorized_client(const char *client_key_str) client = tor_malloc_zero(sizeof(hs_service_authorized_client_t)); if (base32_decode((char *) client->client_pk.public_key, sizeof(client->client_pk.public_key), - pubkey_b32, strlen(pubkey_b32)) < 0) { + pubkey_b32, strlen(pubkey_b32)) != + sizeof(client->client_pk.public_key)) { log_warn(LD_REND, "Client authorization public key cannot be decoded: %s", pubkey_b32); goto err; diff --git a/src/feature/rend/rendcache.c b/src/feature/rend/rendcache.c index ecd85e4a5a..699b5a4a48 100644 --- a/src/feature/rend/rendcache.c +++ b/src/feature/rend/rendcache.c @@ -593,10 +593,10 @@ rend_cache_lookup_v2_desc_as_dir(const char *desc_id, const char **desc) char desc_id_digest[DIGEST_LEN]; tor_assert(rend_cache_v2_dir); if (base32_decode(desc_id_digest, DIGEST_LEN, - desc_id, REND_DESC_ID_V2_LEN_BASE32) < 0) { + desc_id, REND_DESC_ID_V2_LEN_BASE32) != DIGEST_LEN) { log_fn(LOG_PROTOCOL_WARN, LD_REND, "Rejecting v2 rendezvous descriptor request -- descriptor ID " - "contains illegal characters: %s", + "has wrong length or illegal characters: %s", safe_str(desc_id)); return -1; } @@ -854,7 +854,8 @@ rend_cache_store_v2_desc_as_client(const char *desc, *entry = NULL; } if (base32_decode(want_desc_id, sizeof(want_desc_id), - desc_id_base32, strlen(desc_id_base32)) < 0) { + desc_id_base32, strlen(desc_id_base32)) != + sizeof(want_desc_id)) { log_warn(LD_BUG, "Couldn't decode base32 %s for descriptor id.", escaped_safe_str_client(desc_id_base32)); goto err; diff --git a/src/feature/rend/rendcommon.c b/src/feature/rend/rendcommon.c index 15e4534fca..80edf549f1 100644 --- a/src/feature/rend/rendcommon.c +++ b/src/feature/rend/rendcommon.c @@ -171,9 +171,10 @@ rend_compute_v2_desc_id(char *desc_id_out, const char *service_id, } /* Convert service ID to binary. */ if (base32_decode(service_id_binary, REND_SERVICE_ID_LEN, - service_id, REND_SERVICE_ID_LEN_BASE32) < 0) { + service_id, REND_SERVICE_ID_LEN_BASE32) != + REND_SERVICE_ID_LEN) { log_warn(LD_REND, "Could not compute v2 descriptor ID: " - "Illegal characters in service ID: %s", + "Illegal characters or wrong length for service ID: %s", safe_str_client(service_id)); return -1; } diff --git a/src/feature/rend/rendparse.c b/src/feature/rend/rendparse.c index e2378e340f..c79f861b51 100644 --- a/src/feature/rend/rendparse.c +++ b/src/feature/rend/rendparse.c @@ -143,8 +143,9 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out, goto err; } if (base32_decode(desc_id_out, DIGEST_LEN, - tok->args[0], REND_DESC_ID_V2_LEN_BASE32) < 0) { - log_warn(LD_REND, "Descriptor ID contains illegal characters: %s", + tok->args[0], REND_DESC_ID_V2_LEN_BASE32) != DIGEST_LEN) { + log_warn(LD_REND, + "Descriptor ID has wrong length or illegal characters: %s", tok->args[0]); goto err; } @@ -174,8 +175,10 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out, log_warn(LD_REND, "Invalid secret ID part: '%s'", tok->args[0]); goto err; } - if (base32_decode(secret_id_part, DIGEST_LEN, tok->args[0], 32) < 0) { - log_warn(LD_REND, "Secret ID part contains illegal characters: %s", + if (base32_decode(secret_id_part, DIGEST_LEN, tok->args[0], 32) != + DIGEST_LEN) { + log_warn(LD_REND, + "Secret ID part has wrong length or illegal characters: %s", tok->args[0]); goto err; } @@ -429,8 +432,10 @@ rend_parse_introduction_points(rend_service_descriptor_t *parsed, /* Parse identifier. */ tok = find_by_keyword(tokens, R_IPO_IDENTIFIER); if (base32_decode(info->identity_digest, DIGEST_LEN, - tok->args[0], REND_INTRO_POINT_ID_LEN_BASE32) < 0) { - log_warn(LD_REND, "Identity digest contains illegal characters: %s", + tok->args[0], REND_INTRO_POINT_ID_LEN_BASE32) != + DIGEST_LEN) { + log_warn(LD_REND, + "Identity digest has wrong length or illegal characters: %s", tok->args[0]); rend_intro_point_free(intro); goto err;