Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

pkillarjun
Copy link
Contributor

@pkillarjun pkillarjun commented May 27, 2024

oss-fuzz Issue 68458.

@ac000
Copy link
Member

ac000 commented May 27, 2024

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
Copy link
Contributor Author

The link above is giving me "Permission denied"

You are not in the contact.
I will add you in my next PR google/oss-fuzz#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
Copy link
Member

ac000 commented May 28, 2024

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
Copy link
Contributor Author

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 pkillarjun changed the title Fix use-of-uninitialized-value http: fix use-of-uninitialized-value bug May 29, 2024
Signed-off-by: Arjun <pkillarjun@protonmail.com>
@pkillarjun
Copy link
Contributor Author

scorched earth;

@pkillarjun pkillarjun deleted the fix-uninitialized branch May 29, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants