**The bug:** Back in commit 687f9b3bd (#17783) we added support for digests SHA3_256 and SHA3_512 as possible signatures for e.g. the consensus. Then in commit 39b597c2 (#17795) we pared down to only expecting SHA1 and SHA256 in practice ("This saves CPU and RAM when handling consensuses and x509 certs.") But that second commit introduced a subtle bug: it pared down the struct that holds our digests: ``` #define N_DIGEST_ALGORITHMS (DIGEST_SHA3_512+1) +#define N_COMMON_DIGEST_ALGORITHMS (DIGEST_SHA256+1) [...] typedef struct { - char d[N_DIGEST_ALGORITHMS][DIGEST512_LEN]; + char d[N_COMMON_DIGEST_ALGORITHMS][DIGEST256_LEN]; } digests_t; ``` but it left in place the possibility that you could write "sha3-256" or "sha3-256" as your algorithm, and suddenly you are past N_COMMON_DIGEST_ALGORITHMS. This bug manifests as an out-of-bounds read in networkstatus_check_document_signature() where we do ``` signed_digest_len = crypto_pk_keysize(cert->signing_key); signed_digest = tor_malloc(signed_digest_len); if (crypto_pk_public_checksig(cert->signing_key, signed_digest, signed_digest_len, sig->signature, sig->signature_len) < dlen || tor_memneq(signed_digest, consensus->digests.d[sig->alg], dlen)) { ``` and d[3] and d[4] are valid values of alg but d isn't big enough for them. And the same pattern happens in two places in networkstatus_parse_detached_signatures(), where it then triggers with an out-of-bounds read and then sometimes an out-of-bounds write: ``` if (!fast_mem_is_zero(digests->d[alg], digest_length)) { [...] if (base16_decode(digests->d[alg], digest_length, ``` **The fix:** The fix is pretty simple: in networkstatus_parse_vote_from_string() where we do ``` const char *algname = tok->args[0]; int a; id_hexdigest = tok->args[1]; sk_hexdigest = tok->args[2]; a = crypto_digest_algorithm_parse_name(algname); if (a<0) { log_warn(LD_DIR, "Unknown digest algorithm %s; skipping", escaped(algname)); continue; } alg = a; ``` we should check not just a<0 but also a>=N_COMMON_DIGEST_ALGORITHMS. And same for the two places in dsigs_parse.c. **Reachability:** If I am reading this right, for the networkstatus.c case, you need to produce a valid signature from a known dir auth before we'll run the memneq. That is, because we check crypto_pk_public_checksig() first, you'd need to forge a signature from a directory authority to get a client to experience the bug. But for the networkstatus_parse_detached_signatures() case, it looks like you could trigger it on dir auths by uploading a detached signature from a known dir auth's key that claims to be signed using sha3, and no signature checking happens in time. **Impact:** For the client (networkstatus.c) case, I think we're mostly fine here? We can read at most 32+64 bytes past the digest array, and ```common_digests_t digests``` is right in the middle of the huge networkstatus_t object so we'll stay inside it, yay. For the dir auth (dsigs_parse.c) case, it is murkier. We malloc a tiny common_digest_t object, all alone, and then we read 32 or 32+64 bytes past it. That could be all sorts of things in that space. And then we might write to it too, but only if we read it first and it's all zeros. That sounds like a pretty esoteric puzzle (the good news is that it has to be all zeroes first, but the bad news is that the attacker can choose what bytes to write) -- not impossible to produce mischief out of it.