TOR-024 — Tor client – Missing sanity checks in pem_decode

The pem_decode function passes incorrect boundaries to the underlying standard C library function memmem when parsing a PEM file.

• Vulnerability type: CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer
• Threat level: Low

Technical description:

Attackers can craft a payload invoking the tor_memstr (memem) function with invalid borders by crafting a haystack smaller than the needle. However, there is a sanity check in the underlying lib, which prevents any form of out-of-bounds. The memmem() function finds the start of the first occurrence of the substring needle of length needlelen in the memory area haystack of length haystacklen. The memmem() function returns a pointer to the beginning of the substring, or NULL if the substring is not found.

Proof of Concept

int poc(){
  static const char payload[] = "-----BEGIN WOMBAT QUOTE-----\nA";
  unsigned char buf[4096];
  int n = pem_decode(buf, sizeof(buf),payload, strlen(payload),"WOMBAT QUOTE");
  return n;
}

In src/lib/encoding/pem.c:

int
pem_decode(uint8_t *dest, size_t destlen, const char *src, size_t srclen, const char *objtype)
{
  const char *eos = src + srclen;

  src = eat_whitespace_eos(src, eos);

  char *tag = NULL;
  tor_asprintf(&tag, "-----BEGIN %s-----", objtype);
  if ((size_t)(eos-src) < strlen(tag) || fast_memneq(src, tag, strlen(tag))) {
    tor_free(tag);
    return -1;
  }
  src += strlen(tag);
  tor_free(tag);
  /* At this point we insist on spaces (including CR), then an LF. */
  src = eat_whitespace_eos_no_nl(src, eos);
  if (src == eos || *src != '\n') {
    /* Extra junk at end of line: this isn't valid. */
    return -1;
  }

  // NOTE lack of trailing \n. We do not enforce its presence.
  tor_asprintf(&tag, "\n-----END %s-----", objtype);
  const char *end_of_base64 = tor_memstr(src, eos-src, tag);
  tor_free(tag);
  if (end_of_base64 == NULL)
    return -1;
  [...]
}

Impact:

This vulnerability could lead to out-of-bounds read/write and other vulnerabilities, including remote code execution. However, demonstrating a critical impact was impossible as the underlying library checked the bounds of the parameters. Nevertheless, this could change, e.g., by using a vulnerable library.

Recommendations:

• Do not rely on sanity checks within the underlying libraries.
• A valid patch could ensure the haystack length is >= needle length before invoking tor_memstr. Or even better, implement this check within tor_memmem.