[tor-bugs] #17868 [Core Tor/Tor]: base64_decode_nopad() destination buffer length problem

[Tor Bug Tracker & Wiki]

#17868: base64_decode_nopad() destination buffer length problem
--------------------------+------------------------------------
 Reporter:  dgoulet       |          Owner:  nikkolasg
     Type:  defect        |         Status:  needs_revision
 Priority:  Medium        |      Milestone:  Tor: 0.2.9.x-final
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:                |  Actual Points:
Parent ID:  #16943        |         Points:  0.1
 Reviewer:  dgoulet       |        Sponsor:  SponsorR-can
--------------------------+------------------------------------
Changes (by dgoulet):

 * keywords:  review-group-2 =>
 * status:  needs_review => needs_revision


Comment:

 First of all, thanks for this!

 Could you explain how this patch is fixing the issue? Basically, for the
 commit message.

 Some review comments:

 * Rename `base64_decode_internal` to something like `base64_decode_impl(`
 and make it static so it's not accessible by anyone outside of
 util_format.c

 * In the test, `uint8_t *dst2 = tor_malloc_zero(40);` could probably be
 changed to `uint8_t dst2[40];` and then pass `sizeof(dst2)` instead of 40.

 * In the test, I would put this string in a const char above
 `"Hi,thisisatestforbase64decodenopadfuncti"` like `char *decoded_src2` and
 use it in the mem test with strlen(). It just makes things so much easier
 if we ever need to modify this part.

 * Please align the arguments of `base64_decode_internal()` like
 `base64_decode_nopad` does it.

 * I think the following checks could be done at the beginning of the
 function just before the malloc because now they leak `buf` memory on
 error:

 {{{
   if (destlen < (srclen*3)/4)
     return -1;

   if (destlen > SIZE_T_CEILING)
     return -1;
 }}}

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