Redis [BUG] Potential integer overflow in moduleCreateArgvFromUserFormat

In the function moduleCreateArgvFromUserFormat, we have the following code:

robj **moduleCreateArgvFromUserFormat(const char *cmdname, const char *fmt, int *argcp, int *flags, va_list ap) {
    int argv_size;
    argv_size = strlen(fmt)+1;
    const char *p = fmt;
    while(*p) {
        ...
        else if (*p == 'v') {
             robj **v = va_arg(ap, void*);
             size_t vlen = va_arg(ap, size_t);

             argv_size += vlen-1;
             // The multiplication can overflow with a particularly large vlen!
             argv = zrealloc(argv,sizeof(robj*)*argv_size);
             ...
        }
    }
}

The variable vlen is taken from the argument list. Suppose it is a large 64 bits integer (e.g., UINT64_MAX), assigning its value to argv_size (which is of int type) can result in a negative value for argv_size due to truncation. Later, when argv_size is used to compute the allocation size for zrealloc, it will be sign-extended to 64 bits, causing an integer overflow in the multiplication sizeof(robj)argv_size. This can cause memory allocation of a size smaller than expected, which is dangerous.

This is not directly exploitable since the only possible attack vector is a loaded module. Still, it is a bug that worth fixing.

Comment From: sundb

Hi @yiyuaner, I wonder if the stack will overflow if vlen reaches such a large size?

Comment From: yiyuaner

Hi @yiyuaner, I wonder if the stack will overflow if vlen reaches such a large size?

I am not certain whether the variable list ap is always sanitized. Since vlen = va_arg(ap, size_t) is just one argument from the argument list ap, there is no need to provide a very long argument list to trigger it (as long as ap contains a large value of type size_t that will assign to vlen).

Am I right? I am assuming that the caller for the function moduleCreateArgvFromUserFormat may pass in arbitrary arguments. Under that assumption, we do not need a large number of objects on the stack to trigger the bug.

Comment From: sundb

@yiyuaner That's true, but why bother when the attacker can already load the module? The client does not seem to be able to use this overflow to attack(Unless the module developer allows). I'm not sure it needs to be fixed.

Comment From: yiyuaner

@yiyuaner That's true, but why bother when the attacker can already load the module? The client does not seem to be able to use this overflow to attack(Unless the module developer allows). I'm not sure it needs to be fixed.

Ok, seems like it is the responsibility of the module developer to avoid this case. The fix is just an one line assertion though. Please close this issue and pr if it is not needed.

Comment From: sundb

@yiyuaner Wait to see if others have other opinions.

Comment From: yossigo

@sundb I agree this is not a big problem but also the fix is not big, and it does prevent a careless module from being exploited if it doesn't do its own input validation properly.