[tor-bugs] #14013 [Core Tor/Tor]: base16_decode() API is inconsistent and error-prone

[Tor Bug Tracker & Wiki]

#14013: base16_decode() API is inconsistent and error-prone
-----------------------------------+------------------------------------
 Reporter:  nickm                  |          Owner:  nikkolasg
     Type:  defect                 |         Status:  merge_ready
 Priority:  High                   |      Milestone:  Tor: 0.2.9.x-final
Component:  Core Tor/Tor           |        Version:
 Severity:  Normal                 |     Resolution:
 Keywords:  lorax, review-group-3  |  Actual Points:
Parent ID:                         |         Points:  1
 Reviewer:  dgoulet                |        Sponsor:  SponsorS-can
-----------------------------------+------------------------------------
Changes (by dgoulet):

 * status:  needs_revision => merge_ready


Comment:

 Replying to [comment:29 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`

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/14013#comment:30>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online