View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000482fileGeneralpublic2023-10-07 10:212023-12-20 21:15
Reporterpromptfuzz Assigned Tochristos  
PrioritynormalSeveritytweakReproducibilityalways
Status feedbackResolutionopen 
PlatformLinuxOSubuntuOS Version22.04
Product Version5.45 
Summary0000482: Many ASAN warnings in magic_setparam()
DescriptionHi,
    I have wrote a fuzzer to fuzz the magic_setparam() API. The second and third arguments of magic_setparam() were changed to receive fuzzer engine's bytes.
The fuzzer is like below:
```
extern "C" int LLVMFuzzerTestOneInput(const uint8_t* f_data, size_t f_size) {
    ...
    // Call magic_setparam
    const char* parameter = "parameter";
    int result = magic_setparam(magic, fuzz_int32_t_6, fuzz_str_5);
    fprintf(stdout, "Magic setparam result: %d\n", result);

    magic_close(magic);
    return 0;
}
```

However, i found that if i provided the third argument `const void *val` of magic_setparam() with various values, the attached ASAN will produce many warnings.
I have analyzed the root causes of those ASAN warnings.
I think they are caused due to the implicit type annotation of the third argument and the meaningless but error-prone CAST operations on the third arguments.

See the implementation of magic_setparam() below,
If i passed the val as`const char c_val[2] = {0, 100}; const void* val = (const void *) c_val;`, where the val is an array with length of 2 bytes, and the stored value is 100 in the little-endian byte order.
The `*CAST(const size_t *, val)` operations will extend the 2-length array to length of 4, and then access its stored value which caused a buffer-overflow.
Although the over-read value is immediately cast to uint16_t, the over-read operation is unsafe and caused ASAN report warnings.

I have saw the implementations of both magic_setparam(0 and magic_getparam() APIs,
 all the cases of `val` will be eventually cast to size_t.
So, I strongly recommend to declare the `val` with a fixed-length type `const size_t *`, instead of the opaque void type which may cause misunderstanding, as the `const void *` may refer to a value of uint8_t, uint16_t, uint32_t and other types and so no.

```
file_public int
magic_setparam(struct magic_set *ms, int param, const void *val)
{
    if (ms == NULL)
        return -1;
    switch (param) {
    case MAGIC_PARAM_INDIR_MAX:
        ms->indir_max = CAST(uint16_t, *CAST(const size_t *, val));
        return 0;
    ...
    case MAGIC_PARAM_ENCODING_MAX:
        ms->encoding_max = *CAST(const size_t *, val);
        return 0;
    default:
        errno = EINVAL;
        return -1;
    }
}
```
Tagsbug

Activities

christos

2023-12-20 21:15

manager   ~0003986

In the man page it clearly says that the value types are size_t. Why would you expect it to work with other types?

Issue History

Date Modified Username Field Change
2023-10-07 10:21 promptfuzz New Issue
2023-10-07 10:21 promptfuzz Tag Attached: bug
2023-12-20 21:14 christos Assigned To => christos
2023-12-20 21:14 christos Status new => assigned
2023-12-20 21:15 christos Status assigned => feedback
2023-12-20 21:15 christos Note Added: 0003986