Opened 8 months ago

Closed 6 months ago

#21894 closed defect (fixed)

Base32_encode: *actually* allow inputs of odd sizes

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 030-backport
Cc: dgoulet, catalyst Actual Points: .1
Parent ID: Points: .1
Reviewer: Sponsor: SponsorR-can

Description

When we "fixed" #18280 in 4e4a7d2b0c199227252a742541461ec4cc35d358 in 0291 it appears that we introduced a bug: The base32_encode function can read off the end of the input buffer, if the input buffer size modulo 5 is not equal to 0 or 3.

This is not completely horrible, for two reasons:

  • The extra bits that are read are never actually used: so this is only a crash when asan is enabled, in the worst case.
  • The input sizes passed to base32_encode are only ever multiples of 5. They are all either DIGEST_LEN (20), REND_SERVICE_ID_LEN (10), sizeof(rand_bytes) in addressmap.c (10), or an input in crypto.c that is forced to a multiple of 5. So this bug can't actually trigger.

Nonetheless, let's fix it!

Child Tickets

Change History (10)

comment:1 Changed 8 months ago by nickm

Owner: set to nickm
Status: newaccepted

Please review branch bug21894_029.

Also to consider: How far to backport this? The bug is not triggerable today.

comment:2 Changed 8 months ago by nickm

Actual Points: .1
Points: .1
Sponsor: SponsorR-can
Status: acceptedneeds_review

#18280 was SponsorR-can, so this one must be too. :)

comment:3 Changed 8 months ago by catalyst

How about changing src[bit/8+1] in the original to src[(bit+5)/8]? That is more concise and more symmetric with the original if-expression. I think that any time the same byte gets loaded twice into v, the low 8 bits get discarded by the following right-shift. Then again that might be less clear than this patch.

comment:4 Changed 8 months ago by nickm

Is that correct, though? I think in that case, we'd be adding the same byte twice sometimes, and then just not looking at the low bits.

comment:5 Changed 8 months ago by catalyst

When bits = {0, 10, 25} mod 40, the same byte gets loaded into high and low halves of the 16-bit word we're assembling in v, but the right-shift of 11-(bits%8) is greater than 8 bits, so we discard the low byte. (which might be the same thing you're saying in different words?)

comment:6 Changed 8 months ago by nickm

That's right, exactly. It won't make the results wrong, but it will load the "wrong" byte. (Which we won't look at.)

comment:7 Changed 8 months ago by dgoulet

lgtm! Overflow check makes sense. Tests passes.

comment:8 Changed 8 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.2.9.x-final

Ok; after discussion on irc, merging to 0.3.0 and master. Putting in for consideration for 0.2.9 backport or lack thereof

comment:9 Changed 7 months ago by catalyst

Cc: catalyst added

comment:10 Changed 6 months ago by catalyst

Keywords: 029-backport removed
Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final
Resolution: fixed
Status: needs_reviewclosed

If nothing in-tree actually calls base32_encode with an odd size, it probably doesn't need backporting to 0.2.9. Feel free to reopen if you disagree.

Note: See TracTickets for help on using tickets.