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 e95e58d919..f4bb0d38a8 100644 --- a/src/feature/control/control.c +++ b/src/feature/control/control.c @@ -4444,7 +4444,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/hs/hs_client.c b/src/feature/hs/hs_client.c index e1d0e8613a..075f1d5c41 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -1561,7 +1561,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 ebe49f09a5..14655c53a5 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 b94dd9a481..8d286f2bad 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 fadfb43883..abeb150685 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; @@ -1005,4 +1006,3 @@ rend_cache_store_v2_desc_as_client(const char *desc, tor_free(intro_content); return retval; } - diff --git a/src/feature/rend/rendcommon.c b/src/feature/rend/rendcommon.c index de48af795f..5cc054f454 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 abd0feb448..a98cb3ad88 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; diff --git a/src/lib/encoding/binascii.c b/src/lib/encoding/binascii.c index de4d1648bb..187df34243 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/fuzz/fuzz_strops.c b/src/test/fuzz/fuzz_strops.c index 64a6453050..a37cbb5be8 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_); diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index 0b57448bcf..0903c5c49a 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -1703,13 +1703,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') @@ -1717,12 +1717,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 3a0b41faa5..c8945a707c 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)); }