Opened 4 years ago

Closed 3 years ago

#18280 closed defect (fixed)

base32 encoding API doesn't work for a source length that is not a multiple of 5 or 8

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-3 TorCoreTeam201606
Cc: Actual Points: 1 ?
Parent ID: #18278 Points: 1
Reviewer: nickm Sponsor: SponsorR-can

Description

Assuming base32_encode() here (but same issue goes for the decode function), it has a hard requirement of:

Limitation: Requires that srclen*8 is a multiple of 5.

Now, this has some drawbacks because srclen is the length in _bytes_ of what we want to encode. So assuming I want to encode 32 bytes, well I can't because 32 * 8 == 256 which is shy of 4 bits to be a multiple of 8 (260).

But how fun, since I can only pass bytes to that function, how can I tell it to use a srclen of 260 bits (32.5 bytes)? It seems to me we need a function call that takes bits instead of bytes? Or give a positive float as srclen (sounds crazy)?

Or make base32_encode() more smart by using floor((srclen * 8) / 5) as the destination bytes length which will result in the right amount of bit (260 in this case). So that means, the caller must be smart and set the appropriate number of bytes + the extra bits. So to encode 32 bytes + 4 bit, the caller would have to pass a srclen of 33 and expect that 260 bit will be written and not 264.

(Setting parent ID to the proposal 224 key handling since it's a blocker to generate correctly the onion address).

Child Tickets

Change History (17)

comment:1 Changed 4 years ago by dgoulet

Status: newneeds_review

Here is an attempt at fixing the issue. It works for my use case of 32 bytes encoding and make test also passes.

Branch: bug18280_029_01

comment:2 Changed 4 years ago by nickm

Suggestion 1: Let's have unit tests for inputs of various different lengths?

Suggestion 2: Maybe it makes more sense to make it so that we always encrypt all the bits we are given, and zero-pad?

comment:3 in reply to:  2 Changed 4 years ago by dgoulet

Replying to nickm:

Suggestion 1: Let's have unit tests for inputs of various different lengths?

Yes! I actually thought that there would be tests for the base32 API but I see none so I'll do that. :)

Suggestion 2: Maybe it makes more sense to make it so that we always encrypt all the bits we are given, and zero-pad?

Sounds good to me! Taking the example here, encoding 32 bytes will result in 52 bytes with the last 4 bits zeroed. So base32_encode() should take the number of bytes and ceil it to the nearest multiple of 5 bit and test the destlen to at least take that amount (plus NUL byte). Those extra bits will be explicitly zeroed at the end.

comment:4 Changed 4 years ago by dgoulet

Sponsor: SponsorRSponsorR-can

Move those from SponsorR to SponsorR-can.

comment:5 Changed 4 years ago by dgoulet

Reviewer: nickm
Status: needs_reviewneeds_revision

comment:6 Changed 4 years ago by nickm

Owner: set to dgoulet
Status: needs_revisionassigned

comment:7 Changed 4 years ago by nickm

Status: assignedneeds_revision

comment:8 Changed 3 years ago by isabela

Points: small1

comment:9 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

I've added unit tests as a second commit. This also works in chutney.

See branch bug18280_029_02.

comment:10 Changed 3 years ago by nickm

Keywords: review-group-3 added

Move some tickets into review-group-3: they are in 0.2.9, and they are needs_review.

comment:11 Changed 3 years ago by nickm

NM.1:

  • tor_free, not free().

NM.2:

  • on the unit tests, how did you generate these values? I'm a little concerned that we're only testing that our implementation matches itself, not that it matches the 'correct thing'. Are those hand-generated, or just taken from what the code makes today?

comment:12 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

The hard requirement that source buffer be some "right size" is bad. Putting back in needs_revision so we can make it like our base64_encode() API.

comment:13 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

NM1 has been addressed.
NM2 has been detailed in the test how the values are generated.

New branch: bug18280_029_03.

It changes much more things. Among other things, base32_encode() does memory allocation to address the source length that can't be a multiple of 5. I've added two helper functions which gets us closer to what the base64 API looks like.

I don't expect this to be the final version but it's something we can start from.

comment:14 Changed 3 years ago by nickm

So, if the tests really test correct behavior, then we can make it much simpler. How about bug18280_029_03_nm ? (no guarantees.)

comment:15 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

comment:16 in reply to:  14 Changed 3 years ago by dgoulet

Status: needs_revisionmerge_ready

Replying to nickm:

So, if the tests really test correct behavior, then we can make it much simpler. How about bug18280_029_03_nm ? (no guarantees.)

It does work yes! and much simpler.

Small detail, replace buf by src in this comment:

 /* set v to the 16-bit value starting at buf[bits/8], 0-padded. */

comment:17 Changed 3 years ago by nickm

Actual Points: 1 ?
Keywords: TorCoreTeam201606 added
Resolution: fixed
Status: merge_readyclosed

fixed, and squashed as bug18280_029_03_nm_squashed, and merged. Thanks!

Note: See TracTickets for help on using tickets.