Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#14013 closed defect (implemented)

base16_decode() API is inconsistent and error-prone

Reported by: nickm Owned by: nikkolasg
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: lorax, review-group-3
Cc: Actual Points:
Parent ID: #19531 Points: 1
Reviewer: dgoulet Sponsor: SponsorS-can

Description

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.

Child Tickets

Attachments (4)

0001-base16_decodes-returns-bytes.patch (15.7 KB) - added by nikkolasg 3 years ago.
patch with correct spaces
base16_decode_reviewed1.patch (15.3 KB) - added by nikkolasg 3 years ago.
Path after 1st review of dgoulet
base16_decode_reviewed2.patch (22.3 KB) - added by nikkolasg 3 years ago.
Patch after review2
base16_decode_reviewed3.patch (41.5 KB) - added by nikkolasg 3 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 5 years ago by nickm

Initial branch is "bug14013_024" and is awaiting review.

comment:2 Changed 5 years ago by nickm

Merged that to 0.2.5 and later. SHould consider for 0.2.4 backport IMO. Still needs a better fix for 0.2.6. base32_decode has the same API stupidity.

comment:3 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

Defer some items from 0.2.6. They are mostly worth doing, but not going to happen super-fast.

comment:4 Changed 4 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final

comment:5 Changed 4 years ago by nickm

Points: small

comment:6 Changed 4 years ago by nickm

Keywords: lorax added; 024-backport 025-backport tor-client removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final
Severity: Normal

comment:7 Changed 3 years ago by isabela

Sponsor: SponsorS-can

comment:8 Changed 3 years ago by nickm

Priority: HighMedium

comment:9 Changed 3 years ago by nickm

Priority: MediumHigh

comment:10 Changed 3 years ago by nikkolasg

Here's the (my first) patch I've wrote from the master branch. Any comments most welcomed ;)

Last edited 3 years ago by nikkolasg (previous) (diff)

Changed 3 years ago by nikkolasg

patch with correct spaces

comment:11 Changed 3 years ago by nickm

Status: newneeds_review

comment:12 Changed 3 years ago by nickm

Owner: set to nikkolasg
Status: needs_reviewassigned

reassigning in order to set the owner.

comment:13 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:14 Changed 3 years ago by nickm

Keywords: review-group-1 added

comment:15 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision
  • 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_LEN indeed.
+    // XXX Should it not be always DIGEST256_LEN ? Running the tests with
+    // the condition ` != DIGEST256_LEN` fails.

Changed 3 years ago by nikkolasg

Path after 1st review of dgoulet

comment:16 Changed 3 years ago by 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 ;)

comment:17 Changed 3 years ago by nikkolasg

Status: needs_revisionneeds_review

comment:18 Changed 3 years ago by nickm

Reviewer: dgoulet

comment:19 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

First this is a good find. I've opened #19066 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!

comment:20 Changed 3 years ago by nikkolasg

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.

comment:21 Changed 3 years ago by isabela

Points: small1

comment:22 Changed 3 years ago by nickm

Keywords: review-group-2 added; review-group-1 removed

Everything in review-group-1 has had at least 1 review.

Changed 3 years ago by nikkolasg

Patch after review2

comment:23 Changed 3 years ago by 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_decode returns S2K_RFC2440_SPECIFIER_LEN + DIGEST_LEN. In fact even the comments of the function say the hashed passwords should be of length S2K_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-spaces happy, I hope it's ok ;)

Otherwise the rest is ok :)

comment:24 Changed 3 years ago by nikkolasg

Status: needs_revisionneeds_review

comment:25 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

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!

comment:26 Changed 3 years ago by nickm

Keywords: review-group-3 added; review-group-2 removed

These have all had at least one review during review-group-2.

Changed 3 years ago by nikkolasg

comment:27 Changed 3 years ago by nikkolasg

Status: needs_revisionneeds_review

comment:28 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

Here is the patch with minor syntax edit and fix: bug14013_029_01

Both chutney works and make test.

comment:29 Changed 3 years ago by nickm

Status: merge_readyneeds_revision

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() ?

comment:30 in reply to:  29 Changed 3 years ago by dgoulet

Status: needs_revisionmerge_ready

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 return ssize_t but 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: 6bffbb9

To 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_01

comment:31 Changed 3 years ago by nickm

Status: merge_readyneeds_revision

Are you sure you pushed the fixup commits? I'm only seeing one commit on that branch.

comment:32 Changed 3 years ago by dgoulet

Sorry about that, I just pushed it.

comment:33 Changed 3 years ago by dgoulet

Status: needs_revisionmerge_ready

comment:34 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

looks good. squashed and merged. I will open another ticket for the "clear target buffer" thing.

comment:35 Changed 3 years ago by dgoulet

Parent ID: #19531
Note: See TracTickets for help on using tickets.