[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:  review-group-4  |  Actual Points:
Parent ID:                  |         Points:  0.1
 Reviewer:  dgoulet         |        Sponsor:  SponsorR-can
----------------------------+------------------------------------

Comment (by nikkolasg):

 I finally found time to dig more into this problem.
 First of all, looking at the my current version of the code, here's where
 `base64_{encode,decode}_nopad` is used:
 * `base64_encode_nopad`:
     * crypto_format.c: `int n = base64_encode_nopad(buf, sizeof(buf),
 sig->sig, ED25519_SIG_LEN);`
     * test_crypto.c
     * test_dir_handle_get.c:
         *
 `base64_encode_nopad(digest_base64,sizeof(digest_base64),(uint8_t *)
 digest,DIGEST256_LEN);`
         * `base64_encode_nopad(digest_base64, sizeof(digest_base64),
 (uint8_t *) digest,DIGEST256_LEN);`

 * `base64_decode_nopad`:
     * test_crypto.c

 Your example just above uses the function `base64_encode_size` to compute
 the length of the encoded buffer, but it seems to me that all the use case
 for `base64_decode_nopad` uses a already-known length for the base64
 encoded buffer (`DIGEST256_LEN`,`ED25519_SIG_LEN`...).

 Second of all, the issue description states:
 `Passing 40 bytes for dstlen and 54 for srclen (which is the expected
 value _without_ padding), the nopad() call changes srclen to 56 bytes but
 then dstlen should be 42 bytes else the call fails.`
 First thing I've done when tackling this issue is I wrote a failing test,
 and tried to make it work (test_util_format.c:189). It works now: passing
 a dst buffer of length 40 with a src buffer of length 54 decodes
 correctly. What you just described above is the use of
 `base64_encode_size` which will necessarily gives a src buffer length of
 56. So we're not really back into the beginning :) Giving a correct size
 will *not* fail.

 With these two remarks in mind, it seems to me that using
 `base64_encode_size` for encoding *without* padding is wrong API wise and
 logic wise. My suggestion would be to introduce a new function
 `base64_encode_size_nopad` which returns the encoded buffer size without
 padding. That way, both base64 APIs are being consistent:
 * Padding:
     * Get the base64 buffer length with `m = base64_encode_size(n)`
     * Encode with `base64_encode(encoded,n,plain,m)`
     * Decode with `base64_decode(decoded,m,encoded,n)`
 * No padding:
     * Get the base64 buffer length with `m = base64_encode_size_nopad(n)`
     * Encode with `base64_encode_nopad(encoded,n,plain,m)`
     * Decode with `base64_decode_nopad(decoded,m,encoded,n)`

 Does it make sense or am I over my head ?
 If it does make sense, I'm happy to provide that method once you approve
 it.
 If it does not make sense, then I'm eager to hear your reaction ;)

 Thanks
 Ps: sorry for the huge latency, but holidays first !

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