TOR-025 — Tor client - Off-by-one in read_file_to_str_until_eof

This issue is for tracking one of the vulnerabilities coming from the security audit that Radically Open Security did on changes from project Sponsor 61.

The read_file_to_str_until_eof function returns the read size without counting the 0-byte, resulting in an off-by-one vulnerability.

• Vulnerability type: CWE-193: Off-by-one Error
• Threat level: Moderate

Impact:

The vulnerability can be exploited for an information leak, denial of service, or as a primitive for remote code execution, depending on how it is used. However, we did not succeed in creating an effective exploit during the code review, so the vulnerability is rated moderate severity.

Technical description:

During the ongoing fuzzing campaign, we discovered a vulnerability in the read_file_to_str_until_eof function. The second parameter, MAX_FUZZ_SIZE specifies the maximum number of bytes to be read. In the third parameter, size the read bytes are stored. If the read bytes are equal to the maximum number of bytes, a 0-byte is appended to the read string input. In this case, the length of the string is not size but size+1. Further use of the size parameter like tor_memdup(input, size) leads to an off-by-one within the newly allocated buffer raw.

In src/test/fuzz/fuzzing_common.c:

int
main(int argc, char **argv)
{
  size_t size;
#define MAX_FUZZ_SIZE (128*1024)
  char *input = read_file_to_str_until_eof(0, MAX_FUZZ_SIZE, &size);
  tor_assert(input);
  char *raw = tor_memdup(input, size); /* Because input is nul-terminated */
  tor_free(input);
  fuzz_main((const uint8_t*)raw, size);
  tor_free(raw);
 [...]
}

Proof of Concept

# compile Tor with ASAN
CFLAGS="-fsanitize=address -g -O0" CXXFLAGS="-fsanitize=address -g -O0" LDFLAGS="-
fsanitize=address" ./configure
make -j`nproc`
# Trigger the Off-by-one:
echo -en "AAAAAAAAAA" | ./src/app/tor

Edit the Tor main file in src/app/main/main.c:

int
tor_run_main(const tor_main_configuration_t *tor_cfg)
{
#ifdef EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED
  event_set_mem_functions(tor_malloc_, tor_realloc_, tor_free_);
#endif
  size_t bytes_read = 0;
  printf("The function read_file_to_str_until_eof reads maximum %d bytes from stdin:\n", 10);
  char *input = read_file_to_str_until_eof(0, 10, &bytes_read);

  //At this point 0-9 contains "A", and at i=10, we have the 0 byte that ends the string. However,the size is 11 and not 10
  printf("Bytes read from STDIN into *input:\n");
  for(size_t i=0; i < bytes_read+1; i++){
    printf("i=%zu => %02hhX\n", i, input[i]);
    if(i == bytes_read){
      printf("\n=> Off-by-one => i=%zu => %02hhX\n", i, input[i]);
    }
  }
  tor_assert(input);

  // now we malloc 10 bytes (0-9)
  char *raw = tor_memdup(input, bytes_read); /* Because input is nul-terminated */

  printf("\n=> Print the values of *raw within the malloc boundaries:\n");
  for(size_t i=0; i < bytes_read; i++){
    printf("i=%zu => %02hhX\n", i, input[i]);
  }

  free(input);

  printf("Trigger the Off-by-one with ASAN: %lu\n", strlen(raw));
  
  free(raw);
  return 1;
}

Recommendation:

Alter the read_file_to_str_until_eof function to allow for 1 byte less than the maximum number of possible bytes to leave space for appending the required 0 byte.