nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.25k stars 320 forks source link

http: fix use-of-uninitialized-value bug #1292

Closed pkillarjun closed 4 weeks ago

pkillarjun commented 1 month ago

oss-fuzz Issue 68458.

ac000 commented 1 month ago

Thanks for the patch.

The link above is giving me "Permission denied" but I take it it's the following call chain?

nxt_http_fields_hash()

        ret = nxt_lvlhsh_insert(hash, &lhq);                                    

nxt_lvlhsh_insert()

            return nxt_lvlhsh_bucket_insert(lhq, &lh->slot, key, -1);           

nxt_lvlhsh_bucket_insert()

        ret = nxt_lvlhsh_convert_bucket_to_level(lhq, slot, nlvl, bucket);      

nxt_lvlhsh_convert_bucket_to_level()

    lvl = proto->alloc(lhq->pool, size * (sizeof(void *)));                     
pkillarjun commented 1 month ago

The link above is giving me "Permission denied"

You are not in the contact. I will add you in my next PR https://github.com/google/oss-fuzz/pull/12002

but I take it it's the following call chain

Yes.

    bucket = lhq->proto->alloc(lhq->pool, nxt_lvlhsh_bucket_size(lhq->proto));

lhq->pool is not initialized.

More Info:

Stack trace

BAD BUILD: /tmp/not-out/tmpmofmr8u2/fuzz_http_parse seems to have either startup crash or exit:
vm.mmap_rnd_bits = 28
/tmp/not-out/tmpmofmr8u2/fuzz_http_parse -rss_limit_mb=2560 -timeout=25 -seed=1337 -runs=4 -dict=fuzz_http_parse.dict < /dev/null
==156==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x563d1a8e97aa in nxt_lvlhsh_new_bucket /src/unit/src/nxt_lvlhsh.c:292:14
    #1 0x563d1a91560e in nxt_http_fields_hash /src/unit/src/nxt_http_parse.c:1199:15
    #2 0x563d1a89e5d4 in LLVMFuzzerInitialize /src/unit/src/fuzz/nxt_http_parse_fuzz.c:32:11
    #3 0x563d1a782843 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:650:5
    #4 0x563d1a7b0c52 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #5 0x7fd90cb52082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 87b331c034a6458c64ce09c03939e947212e18ce)
    #6 0x563d1a77[589](https://github.com/google/oss-fuzz/actions/runs/9253017792/job/25451853369#step:7:590)d in _start (/tmp/not-out/tmpmofmr8u2/fuzz_http_parse+0x8989d)

DEDUP_TOKEN: nxt_lvlhsh_new_bucket--nxt_http_fields_hash--LLVMFuzzerInitialize
  Uninitialized value was created by an allocation of 'lhq' in the stack frame
    #0 0x563d1a9152d1 in nxt_http_fields_hash /src/unit/src/nxt_http_parse.c:1181:5

DEDUP_TOKEN: nxt_http_fields_hash
SUMMARY: MemorySanitizer: use-of-uninitialized-value /src/unit/src/nxt_lvlhsh.c:292:14 in nxt_lvlhsh_new_bucket
ac000 commented 4 weeks ago

Wondering why we don't hit this issue in practice...

In src/nxt_http_parse.c::nxt_http_fields_hash()

lhc.proto,alloc

Is set via

const nxt_lvlhsh_proto_t  nxt_http_fields_hash_proto  nxt_aligned(64) = {       
    NXT_LVLHSH_BUCKET_SIZE(64),                                                 
    { NXT_HTTP_FIELD_LVLHSH_SHIFT, 0, 0, 0, 0, 0, 0, 0 },                       
    nxt_http_field_hash_test,                                                   
    nxt_lvlhsh_alloc,                                                           
    nxt_lvlhsh_free,                                                            
}; 
...
lhq.proto = &nxt_http_fields_hash_proto;

Which results in something like

(gdb) p *lhq.proto
$11 = {
  bucket_end = 12,
  bucket_size = 64,
  bucket_mask = 63,
  shift = "\005\000\000\000\000\000\000",
  test = 0x464350 <nxt_http_field_hash_test>,
  alloc = 0x40841a <nxt_lvlhsh_alloc>,
  free = 0x40843f <nxt_lvlhsh_free>
}

It looks like this step isn't happening in the fuzzing case, which I assume is src/fuzz/nxt_http_parse_fuzz.c ?

However I can't immediately see how this is being built....

pkillarjun commented 4 weeks ago

Wondering why we don't hit this issue in practice...

Because I didn't say lhq->proto isn't initialized; I said lhq->pool isn't initialized.

The lhq->proto->alloc call shows that there is a bug in lhq->pool nxt_lvlhsh.c#L292.

Also, my patch isn't new to this codebase; it's already been used.

pkillarjun commented 4 weeks ago

scorched earth;