Opened 17 months ago

Closed 2 months ago

#19281 closed defect (fixed)

Potential heap corruption via `write_escaped_data` in control.c

Reported by: asn Owned by: nickm
Priority: High Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-bug-bounty, heap-correctness, disaster-waiting-to-happen, review-group-22
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: dgoulet Sponsor: SponsorV-can

Description

Here follows another bug report by guido from hackerone.

We think this is safe to list on trac, because the write_escaped_data function is only called from the control API, and we believe it's not possible to force the control API to write enough amounts of data to trigger this bug (since descriptors, dir documents, and cells are all capped in size, and this bug requires message lengths close to INT_MAX).

Bug report follows:


/** Given a <b>len</b>-character string in <b>data</b>, made of lines
 * terminated by CRLF, allocate a new string in *<b>out</b>, and copy the
 * contents of <b>data</b> into *<b>out</b>, adding a period before any period
 * that appears at the start of a line, and adding a period-CRLF line at
 * the end. Replace all LF characters sequences with CRLF.  Return the number
 * of bytes in *<b>out</b>.
 */
STATIC size_t
write_escaped_data(const char *data, size_t len, char **out)
{
  size_t sz_out = len+8;
  char *outp;
  const char *start = data, *end;
  int i;
  int start_of_line;
  for (i=0; i<(int)len; ++i) {
    if (data[i]== '\n')
      sz_out += 2; /* Maybe add a CR; maybe add a dot. */
  }
  *out = outp = tor_malloc(sz_out+1);
  end = data+len;
  start_of_line = 1;
  while (data < end) {
    if (*data == '\n') {
      if (data > start && data[-1] != '\r')
        *outp++ = '\r';
      start_of_line = 1;
    } else if (*data == '.') {
      if (start_of_line) {
        start_of_line = 0;
        *outp++ = '.';
      }
    } else {
      start_of_line = 0;
    }
    *outp++ = *data++;
  }
  if (outp < *out+2 || fast_memcmp(outp-2, "\r\n", 2)) {
    *outp++ = '\r';
    *outp++ = '\n';
  }
  *outp++ = '.';
  *outp++ = '\r';
  *outp++ = '\n';
  *outp = '\0'; /* NUL-terminate just in case. */
  tor_assert((outp - *out) <= (int)sz_out);
  return outp - *out;
}

There are two potential vulnerabilities lurking here:

  1. If the input size (len) >= 0x80000000, then this loop will not execute at all:
  for (i=0; i<(int)len; ++i) {
    if (data[i]== '\n')
      sz_out += 2; /* Maybe add a CR; maybe add a dot. */
  }

Because the condition i<(int)len is effectively i<(negative number) and i is intialized to 0, this can never be true. As a result of this, the output buffer (whose size is based on sz_out)
is too small to hold the result for an input buffer containing '\n' characters.
Triggering this is typically only feasible on a 64-bit system, because if the input buffer is >= 0x80000000 bytes, then sz_out is set to 0x80000008 bytes, and allocating such an amount twice (one for the
input buffer, and one for the output buffer) is not possible on a 32-bit system.

  1. If the equation (number of '\n' characters in input buffer * 2 + size of input buffer) exceeds 0xFFFFFFFF, then this will cause heap corruption on a 32-bit system, because sz_out overflows.

Child Tickets

Change History (15)

comment:1 Changed 17 months ago by nickm

Keywords: 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

Calling this in-for-0.2.9 since it is about correctness and heap-respect.

comment:2 Changed 15 months ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:3 Changed 11 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:4 Changed 10 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:5 Changed 5 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:6 Changed 5 months ago by nickm

Keywords: isaremoved removed

comment:7 Changed 4 months ago by nickm

Keywords: heap-correctness disaster-waiting-to-happen added
Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Priority: MediumHigh

comment:8 Changed 3 months ago by nickm

Sponsor: SponsorV-can

comment:9 Changed 2 months ago by nickm

Fix in bug192821_025. I recommend no backport.

comment:10 Changed 2 months ago by nickm

Status: newneeds_review

comment:11 Changed 2 months ago by nickm

Owner: set to nickm
Status: needs_reviewassigned

Setting owner

comment:12 Changed 2 months ago by nickm

Status: assignedneeds_review

comment:13 Changed 2 months ago by nickm

Keywords: review-group-22 added

comment:14 Changed 2 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

If by some miracle someone is able to pass a string that is of SIZE_MAX, then I can assert my tor through the control port... Hmmm, I would say unlikely that is possible because it would mean a string that has the length of basically my entire RAM (?) ;).

+  tor_assert(len < SIZE_MAX - 9);

lgtm;

comment:15 Changed 2 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Yeah, I don't think this can happen either, but guido has a pretty good track record, and we might as well fix all the stuff he found. Code that's harmless today can become harmful tomorrow if somebody changes it or copies under the assumption that it was correct to start with.

Merging to 0.3.2, no backport.

Note: See TracTickets for help on using tickets.