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.
Activity
changed milestone to %Tor: 0.2.9.x-final
Trac:
Username: nikkolasg
-
base16_decode()comment needs to mention that we return the size now on success. -
There is still this line looking for an error instead of
!=to the expected size but all other fixes make that not equal change:
+ base16_decode(guard_id, DIGEST_LEN, inputs_tmp, HEX_DIGEST_LEN) < 0) {- About this comment, can you tell us what's the returned value? It seems it should be
DIGEST256_LENindeed.
+ // XXX Should it not be always DIGEST256_LEN ? Running the tests with + // the condition ` != DIGEST256_LEN` fails.Trac:
Status: needs_review to needs_revision-
-
Trac:
Username: nikkolasg -
Oups, I thought I could remove the first patch but no.. Adapted the code for your first two points. About the third:
- Value 17B1C409A7B0E9A9E307D38A18AF7E5F8C15C803
- Size of output 20 (vs 32)
- Failing tests:
- dir/clip_unmeasured_bw_kb
- dir/clip_unmeasured_bw_kb_alt
- dir/v3_networkstatus
If you'd like I can try to debug it further but to be honest, I think a more experienced Tor developer will be far most efficient (=fast) than me as I have to understand the whole codebase evolving around this line. That said, I'm willing to try ;)
Trac:
Username: nikkolasg -
First this is a good find. I've opened #19066 (moved) about it:
+ // XXX Should it not be always DIGEST256_LEN ? Running the tests with + // the condition ` != DIGEST256_LEN` fails.The patch looks good. There are some minor syntax issues that can easily be fixed such as:
if (base16_decode(digest, DIGEST_LEN, hexbuf, HEX_DIGEST_LEN) != DIGEST_LEN) {It should be aligned like this to follow the code base:
if (base16_decode(digest, DIGEST_LEN, hexbuf, HEX_DIGEST_LEN) != DIGEST_LEN) {This one has an extra white space:
base16_decode(voter->identity_digest, sizeof(voter->identity_digest), - tok->args[1], HEX_DIGEST_LEN) < 0) { + tok->args[1], HEX_DIGEST_LEN) != DIGEST_LEN) {Align:
+ tt_int_op(sizeof(blinding_param), OP_EQ, DECODE(blinding_param, + ED25519_BLINDING_PARAMS[i])); + tt_int_op(sizeof(buf),OP_EQ,base16_decode(buf,sizeof(buf),\ + hex,strlen(hex)));\Sorry about this, very wow such nitpicking but it makes the difference in the end for a more clean and maintainable code. Let me know if you can quickly fix those else I can go over them and credit you as the author. Thanks!
Trac:
Status: needs_review to needs_revision -
I get it, no worry, I'm just not used yet to your formatting :) I'll fix those and submit a new patch, maybe not quickly because I've just found some unchanged calls to base16_decode that I need to fix also (still some
base16_decode(..) == 0). Thanks for the review anyway. I'll get back to you soon.Trac:
Username: nikkolasg -
Trac:
Username: nikkolasg -
Hi, finally found the time to finalize it. Here's some comments:
-
In routerset.c:107, a call to base16_decode is un-checked; while it may not be important, I prefer still to announce it so it may be looked over by a Tor dev at some point later ...
-
In control.c:1214, there's inconsistency between the size of the buffer and the return value. The size of the dest buffer is 64 but
base16_decodereturnsS2K_RFC2440_SPECIFIER_LEN + DIGEST_LEN. In fact even the comments of the function say the hashed passwords should be of lengthS2K_RFC2440_SPECIFIER_LEN + DIGEST_LEN. It would be a good practice to unify both values. I did not do it here as it does not make much sense here I think. -
I allowed myself to change some other files to make the
make check-spaceshappy, I hope it's ok ;)
Otherwise the rest is ok :)
Trac:
Username: nikkolasg -
-
I tried different things to apply the patch on current master and I'm unable! Apparently, routerparse.c changed too much...
The patch looks good I would say except few minor syntax things but I can fix them. However, I can't use it as it is, can you maybe provide a git branch or a fresh diff that apply on master? Thanks!
Trac:
Status: needs_review to needs_revision -
Trac:
Username: nikkolasg 
requested changes:
- require that destlen be less than SSIZE_MAX. Otherwise the cast in base16_decode isn't safe.
- document what happens if destlen is greater than or less than srclen/2.
+ ok = base16_decode(id, DIGEST_LEN, cp+strlen("id="), + strlen(cp)-strlen("id=")) != DIGEST_LEN ? 0 : 1;would make more sense as
ok = (base16_decode(...) == DIGEST_LEN)- Why is the comparison in decode_hashed_passwords < rather than != ?
To consider:
- Should we make all of these functions clear the unused portion of the output buffer?
- Is it possible that we missed any instances of base16_decode() ?
Trac:
Status: merge_ready to needs_revision-
Replying to nickm:
requested changes:
- require that destlen be less than SSIZE_MAX. Otherwise the cast in base16_decode isn't safe.
Hrm actually that ssize_t cast doesn't make too much sense. That function returns a int so we should align everything to
INT_MAX. I originally changed it to returnssize_tbut I changed my mind so every encode/decode function have the same interface.Fixup commit:
b397307- document what happens if destlen is greater than or less than srclen/2.
Also in fixup commit:
b397307{{{
-
ok = base16_decode(id, DIGEST_LEN, cp+strlen("id="), -
strlen(cp)-strlen("id=")) != DIGEST_LEN ? 0 : 1;
}}}
would make more sense as
ok = (base16_decode(...) == DIGEST_LEN)- Why is the comparison in decode_hashed_passwords < rather than != ?
This was a leftover.
Both of the above fixed in fixup commit:
6bffbb9To consider:
- Should we make all of these functions clear the unused portion of the output buffer?
Ideally, we should make all the decode function return how many bytes they used so the caller can know the exact amount filled up.
Now this patch makes it for base16 and base64 is already doing that. base32 is the missing one. New ticket time I presume.
- Is it possible that we missed any instances of base16_decode() ?
Always possible but nikkolasg originally made the patch and then I went over each of the base16_decode I could find to fix some syntax and change
<to!=. So if we missed one, bad luck I would say.See branch with the fixup commits:
bug14013_029_01Trac:
Status: needs_revision to merge_ready -
Trac:
Parent: N/A to #19531 (moved) mentioned in issue #17868 (moved)
mentioned in issue #19066 (moved)
moved to tpo/core/tor#14013 (closed)
mentioned in issue tpo/core/tor#19066 (closed)
