Stack-buffer-underflow error due to out of bounds memory access found in src/bin/visit_macos_launcher.c

[Arpan3323]

Describe the bug

Hi, the line below can cause a stack-buffer-underflow error since the condition syscmd[cmdidx] != '/' will be evaluated when cmdidx == -1 and then cmdidx >= 0 (second condition) will be checked.

visit/src/bin/visit_macos_launcher.c

Line 60 in 02347a2

while (syscmd[cmdidx] != '/' && cmdidx >= 0) cmdidx--;

How to reproduce

I used libFuzzer to write a fuzz test for the linked file. This required inline-ing/expanding the macro function COPYCHARS and extracting the body of main into a function that libFuzzer can call.

fuzzer.cpp:

#include <cstddef>
#include <cstdint>
#include <cstring>

extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
    if (Size == 0) return 0;

    /* dummy argv[0] since libFuzzer has it own main() */
    char *argv0 = new char[Size + 1];
    std::memcpy(argv0, Data, Size);
    argv0[Size] = '\0';

    const char *peerPath = "/../Resources/bin/visit";
    int cmdidx;
    char syscmd[8192];

    /* initialize syscmd to all null chars */
    cmdidx = 0;
    while (cmdidx < sizeof(syscmd))
        syscmd[cmdidx++] = '\0';
    cmdidx = 0;

    for (int i = 0; argv0[i]; i++, cmdidx++) {
        if (cmdidx >= sizeof(syscmd)) {
            delete[] argv0;
            return 12; // ENOMEM
        }
        syscmd[cmdidx] = argv0[i];
    }

    /* walk backwards from end of argv[0] to first slash char */
    while (syscmd[cmdidx] != '/' && cmdidx >= 0) cmdidx--;

    if (cmdidx == -1) {
        cmdidx = 0;
        for (int i = 0; "."[i]; i++, cmdidx++) {
            if (cmdidx >= sizeof(syscmd)) {
                delete[] argv0;
                return 12; // ENOMEM
            }
            syscmd[cmdidx] = "."[i];
        }
    }

    for (int i = 0; peerPath[i]; i++, cmdidx++) {
        if (cmdidx >= sizeof(syscmd)) {
            delete[] argv0;
            return 12; // ENOMEM
        }
        syscmd[cmdidx] = peerPath[i];
    }

    delete[] argv0;
    return 0;
}

You can compile this using:

clang++ -g -O1 -fsanitize=fuzzer,address,undefined -fno-omit-frame-pointer fuzzer.cpp -o fuzzer

Crash report

INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2363393054
INFO: Loaded 1 modules   (39 inline 8-bit counters): 39 [0x56079210ffa8, 0x56079210ffcf), 
INFO: Loaded 1 PC tables (39 PCs): 39 [0x56079210ffd0,0x560792110240), 
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
fuzzer.cpp:32:12: runtime error: index -1 out of bounds for type 'char[8192]'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior fuzzer.cpp:32:12 
=================================================================
==55644==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7fd65250401f at pc 0x5607920c9ee0 bp 0x7ffe77875610 sp 0x7ffe77875608
READ of size 1 at 0x7fd65250401f thread T0
    #0 0x5607920c9edf in LLVMFuzzerTestOneInput fuzzer.cpp:32:12
    #1 0x560791fd4c54 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (fuzzer+0x4cc54) (BuildId: 208079321ce9f07db3ccf001f7caadda5e925489)
    #2 0x560791fd4349 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (fuzzer+0x4c349) (BuildId: 208079321ce9f07db3ccf001f7caadda5e925489)
    #3 0x560791fd5fd6 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (fuzzer+0x4dfd6) (BuildId: 208079321ce9f07db3ccf001f7caadda5e925489)
    #4 0x560791fd6477 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (fuzzer+0x4e477) (BuildId: 208079321ce9f07db3ccf001f7caadda5e925489)
    #5 0x560791fc396f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (fuzzer+0x3b96f) (BuildId: 208079321ce9f07db3ccf001f7caadda5e925489)
    #6 0x560791fedff6 in main (fuzzer+0x65ff6) (BuildId: 208079321ce9f07db3ccf001f7caadda5e925489)
    #7 0x7fd653bc21c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
    #8 0x7fd653bc228a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
    #9 0x560791fb8954 in _start (fuzzer+0x30954) (BuildId: 208079321ce9f07db3ccf001f7caadda5e925489)

Address 0x7fd65250401f is located in stack of thread T0 at offset 31 in frame
    #0 0x5607920c9717 in LLVMFuzzerTestOneInput fuzzer.cpp:5

  This frame has 1 object(s):
    [32, 8224) 'syscmd' (line 15) <== Memory access at offset 31 underflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-underflow fuzzer.cpp:32:12 in LLVMFuzzerTestOneInput
Shadow bytes around the buggy address:
  0x7fd652503d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652503e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652503e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652503f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652503f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x7fd652504000: f1 f1 f1[f1]00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652504080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652504100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652504180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652504200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fd652504280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==55644==ABORTING

Potential fix

changing while (syscmd[cmdidx] != '/' && cmdidx >= 0) cmdidx--; to while (cmdidx >= 0 && syscmd[cmdidx] != '/') cmdidx--; fixes the issue and might also make the following if statement redundant.

Please let me know if I am mistaken or if you have any questions :)

[cyrush]

Thank you for this report! We will resolve this in a future release

[Arpan3323]

No problem, happy to help.

[markcmiller86]

I've assigned myself. I think the correct resolution is simply to change the order of the two checks in the && statement.

Instead of...

while (syscmd[cmdidx] != '/' && cmdidx >= 0) cmdidx--; 

use...

while (cmdidx >= 0 && syscmd[cmdidx] != '/') cmdidx--; 

That way, if cmdidx ever does become -1 the && will fail upon the first operand and never evaluate the second.

[markcmiller86]

doh! Thats exactly what @Arpan3323 reported!