base16_decode() API is inconsistent and error-prone
Our other baseXYZ_decode() functions return the number of bytes written to the output. But base16_decode() returns 0 on success and -1 on failure, so unless you checked the input length, you have a risk of leaving the output buffer partially full.
Calls where we messed up:
- connction_dir_retry_bridges()
- decoding the K_FINGERPRINT entry in router_parse_entry_from_string()
- add_fingerprint_to_dir()
- entry_Guards_parse_state()
- getinfo_helper_networkstatus()
- authority_cert_parse_from_string()
As far as I can tell, none of these is disastrous, or leaks stack information to an attacker. But wow, it sure would be good to fix them.
Proposed fix for backporting:
- Add a memset(0) to base*_en/decode, so they never leave stuff unassigned.
Proposed fix for master:
- Make the return value for base16_decode consistent with the others.
- Fix everything that checks for whether base16_decode() to check an actual return value.