Write-based buffer overflow in uvwasi__normalize_path

[AdamKorcz]

This is a disclosure for an issue in node.js's use of UVWASI that we have found during a security audit of node.js.
The issue is a heap write-based buffer overflow in src/path_resolver.c:uvwasi__normalize_path. The issue was found by the following fuzzer:

#include <stdint.h>
#include <stdlib.h>
#include <string.h>

#include "../src/path_resolver.h"

#define BUFFER_SIZE 128

char normalized_buffer[BUFFER_SIZE];

int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
  char *new_str = (char *)malloc(size + 1);
  if (new_str == NULL) {
    return 0;
  }
  memcpy(new_str, data, size);
  new_str[size] = '\0';

  memset(normalized_buffer, 0, BUFFER_SIZE);

  uvwasi__normalize_path(new_str, size, normalized_buffer, BUFFER_SIZE);

  free(new_str);
  return 0;
}

The fuzzer finds an issue with the following ASAN report:

=================================================================
==97003==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000101f8a0 at pc 0x00000056c043 bp 0x7ffc71cbc100 sp 0x7ffc71cbc0f8
WRITE of size 1 at 0x00000101f8a0 thread T0
    #0 0x56c042 in uvwasi__normalize_path /src/uvwasi/src/path_resolver.c
    #1 0x56b533 in LLVMFuzzerTestOneInput /src/uvwasi/.clusterfuzzlite/path_resolve_fuzzer.c:48:9
    #2 0x43f323 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #3 0x42aa82 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:324:6
    #4 0x43032c in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:860:9
    #5 0x459862 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #6 0x7f7d5a029d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #7 0x7f7d5a029e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #8 0x420c4d in _start (/tmp/oss-fuzz/build/out/uvwasi/path_resolve_fuzzer+0x420c4d)

The issue occurs on line 141 in path_resolver.c:

uvwasi/src/path_resolver.c

Lines 139 to 141 in 2d0c0d0

	memcpy(ptr, cur, cur_len);
	ptr += cur_len;
	*ptr = '\0';

      memcpy(ptr, cur, cur_len);
      ptr += cur_len;
      *ptr = '\0';

The problem is that ptr += cur_len may set ptr to point at normalized_path + normalized_len which causes an off-by-one issue when the normalized_len corresponds to the size of the normalized_path buffer.
The proposed fix is to check that ptr has not increased beyond the bounds of normalized_path:

      memcpy(ptr, cur, cur_len);
      ptr += cur_len;
      if (ptr >= (normalized_path + normalized_len))
        return UVWASI_ENOTCAPABLE;
      *ptr = '\0';

It's important to highlight in this case that the function used within uvwasi always allocates an extra byte such as here and here. However, we consider it counter-intuitive that uvwasi__normalize_path reads beyond the specified length, this is made more counter-intuitive considering the tests of this function provides the size of the buffer and not 1 less than the size of the normalized buffer here.

Considering that the API is used correctly in the places it's called, we consider the best option to either change the tests to ensure proper sizing is enabled, or, better yet to ensure that the funtion does not read beyond the speified length and adjust the callers of the function accordingly.

The fuzzer we used for this is:

#include <stdint.h>
#include <stdlib.h>
#include <string.h>

#include "uvwasi.h"
#include "../src/fd_table.h"
#include "../src/path_resolver.h"
#include "../src/wasi_rights.h"

#define BUFFER_SIZE 128

char normalized_buffer[BUFFER_SIZE];
static uvwasi_t uvwasi;

int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
  uvwasi_errno_t err;
  struct uvwasi_fd_wrap_t fd;

  if (size < 10) {
    return 0;
  }

  char *new_str = (char *)malloc(size + 1);
  if (new_str == NULL) {
    return 0;
  }
  memcpy(new_str, data, size);
  new_str[size] = '\0';

  memset(normalized_buffer, 0, BUFFER_SIZE);

  static uvwasi_options_t init_options;
  uvwasi_options_init(&init_options);
  uvwasi_init(&uvwasi, &init_options);


  fd.id = 3;
  fd.fd = 3;
  fd.path = new_str;
  fd.real_path = new_str;
  fd.normalized_path = normalized_buffer;
  fd.type = UVWASI_FILETYPE_DIRECTORY;
  fd.rights_base = UVWASI__RIGHTS_ALL;
  fd.rights_inheriting = UVWASI__RIGHTS_ALL;
  fd.preopen = 0;


  err = uvwasi__normalize_path(new_str, size, fd.normalized_path, BUFFER_SIZE);
  if (err == UVWASI_ESUCCESS) {
    char* resolved = NULL;
    uvwasi__resolve_path(&uvwasi, &fd, new_str, size, &resolved, 0);
    if (resolved != NULL) {
        free(resolved);
    }
  }
  
  uvwasi_destroy(&uvwasi);

  free(new_str);
  return 0;
}

[guybedford]

Thank you for sharing this case and for fuzzing new cases in the codebase, it's great to verify these API surfaces.

As you note, the specific function call contract for uvwasi_normalize_path is that normalized_len is strlen of normalized_path which contains an additional C-string zero byte at the length index (and therefore the underlying string allocation is len + 1). You also confirm this invariant is guaranteed in all call sites.

Were you able to tell if there is any potential for this case given the current call site usage?

Then further, can you clarify the alternative refactoring you were suggesting here? Is the suggestion to instead have the len be a definition of the length that includes the zero byte?

[mhdawson]

@AdamKorcz any chance you can answer:

Then further, can you clarify the alternative refactoring you were suggesting here? Is the suggestion to instead have the len be a definition of the length that includes the zero byte?

[DavidKorczynski]

Apologies for the delay.

Is the suggestion to instead have the len be a definition of the length that includes the zero byte?

Yes.

As I see it there are probably these options:

1. Ensure that the logic does not read beyond the provided length from a pure byte perspective, i.e. the length should include any zero bytes and be a raw indicator of the size of the buffer.
2. Adjust the test so it won't fail if e.g. a very long path is used as data (this is more of an indirect way) to show potential developers against the code that this is the expected input.
3. Make it clear through some documentation that the length excludes a zero-byte and thus the length argument should be one less than the size of the provided buffer. In essence just copying what @guybedford highlighted above "the specific function call contract for uvwasi_normalize_path is that normalized_len is strlen of normalized_path which contains an additional C-string zero byte at the length index (and therefore the underlying string allocation is len + 1)" -- I don't think this is mentioned anywhere as such?

Were you able to tell if there is any potential for this case given the current call site usage?

Am not sure if you mean if we found any potential uses of the code that could misuse it? If so, then no. The only place could perhaps be the test where the size is not indicated explicitly to be +1, which is probably because the paths are not large enough. Perhaps for this reason option (3) mentioned above is the most appropriate if you think it's reasonable to let devs know about the function's contract.

[mhdawson]

@DavidKorczynski, I'm good with option 3