Opened 12 months ago

Closed 5 months ago

#19222 closed defect (fixed)

base64_decode() unreachable heap corruption on 32-bit systems

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-bug-bounty, 028-backport, isaremoved
Cc: Actual Points:
Parent ID: #19531 Points: 1
Reviewer: Sponsor:

Description

Hello,

this is a bug by Guido Vranken from our bug bounty program. After analysis, we found that there are no codepaths that allow the attacker to specify such a big input size to base64_decode() hence this bug should not be exploitable. More checking should be done, and there might be more instances of this rounding pattern around our codebase.

Here follows the bug report as received:


int
base64_decode(char *dest, size_t destlen, const char *src, size_t srclen)
{
...
...
  if (destlen < (srclen*3)/4)
    return -1;
  if (destlen > SIZE_T_CEILING)
    return -1;

The problem here is that the multiplication (by 3) occurs before the division (by 4).

For source strings larger than 0xFFFFFFFF / 3 == 0x55555555, an overflow will occur within this calculation. If the result of the overflow-affected calculation is smaller than what destlen is, then
this check will be passed and memory will be corrupted.

Child Tickets

Change History (8)

comment:1 Changed 12 months ago by nickm

  • Keywords 028-backport added; 029-proposed removed
  • Milestone changed from Tor: 0.2.??? to Tor: 0.2.9.x-final

Adding this (and #19223) to 0.2.9 on the theory that we have very seldom regretted hardening a function against future mistakes

comment:2 Changed 10 months ago by isabela

  • Keywords isaremoved added
  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.2.???

comment:3 Changed 9 months ago by dgoulet

  • Parent ID set to #19531

comment:4 Changed 6 months ago by teor

  • Milestone changed from Tor: 0.2.??? to Tor: 0.3.???

Milestone renamed

comment:5 Changed 5 months ago by hji

  • Status changed from new to needs_review

comment:6 Changed 5 months ago by teor

  • Milestone changed from Tor: 0.3.??? to Tor: 0.3.0.x-final
  • Status changed from needs_review to merge_ready

Looks good to me, but I haven't run it.
It should be tested on at least i386 and x86_64 before we merge.

(Or we could just merge, and let the buildbots do that.)

comment:7 Changed 5 months ago by nickm

tested and merged. lgtm too.

comment:8 Changed 5 months ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed
Note: See TracTickets for help on using tickets.