Is the sscanf function in the Linux kernel susceptible to buffer overflow attacks?

Question

From what I understand, a typical buffer overflow attack occurs when an attack overflows a buffer of memory on the stack, thus allowing the attacker to inject malicious code and rewrite the return address on the stack to point to that code.

This is a common concern when using functions (such as sscanf) that blindly copy data from one area to another, checking one for a termination byte:

char str[8];                               /* holds up to 8 bytes of data */
char *buf = "lots and lots of foobars";    /* way more than 8 bytes of data */
sscanf(buf, "%s", str);                    /* buffer overflow occurs here! */

I noticed some sysfs_ops store functions in the Linux kernel are implemented with the Linux kernel's version of the sscanf function:

static char str[8];    /* global string */
static ssize_t my_store(struct device *dev,
                        struct device_attribute *attr,
                        const char *buf, size_t size)
{
        sscanf(buf, "%s", str);    /* buf holds more than 8 bytes! */
        return size;
}

Suppose this store callback function is set to a writable sysfs attribute. Would a malicious user be able to intentionally overflow the buffer via a write call?

Normally, I would expect guards against buffer overflow attacks -- such as limiting the number of bytes read -- but I see none in a good number of functions (for example in drivers/scsi/scsi_sysfs.c).

Does the implementation of the Linux kernel version of sscanf protect against buffer overflow attacks; or is there another reason -- perhaps buffer overflow attacks are impossible given how the Linux kernel works under the hood?

Answer 1

The Linux sscanf() is vulnerable to buffer overflows; inspection of the source shows this. You can use width specifiers to limit the amount a %s is allowed to write. At some point your str must have had copy_from_user() run on it as well. It is possible the user space to pass some garbage pointer to the kernel.

In the version of Linux you cited, the scsi_sysfs.c does have a buffer overflow. The latest version does not. The committed fix should fix the issue you see.

Answer 2

Short answer:

sscanf, when well called, will not cause buffer overflow, especially in sysfs xxx_store() function. (There are a lot sscanf in sysfs XXX_store() examples), because Linux kernel add a '\0' (zero-terminated) byte after the string (buf[len] = 0;) for your XXX_store() function.

Long answer:

Normally, sysfs are defined to have a strict formatted data. Since you expect 8 bytes at most, it's reasonable to limit the size you get like this:

static char str[8];    /* global string */
static ssize_t my_store(struct device *dev,
                        struct device_attribute *attr,
                        const char *buf, size_t size)
{
        if (size > 8) {
                printk("Error: Input size > 8: too large\n");
                return -EINVAL;
        }
        sscanf(buf, "%s", str);    /* buf holds more than 8 bytes! */
        return size;
}

(Note: use 9 rather than 8, if you expect a 8-bytes string plus '\n')

(Note that you do reject some inputs such as those with many leading white spaces. However, who would send a string with many leading white spaces? Those who want to break your code, right? If they don't follow your spec, just reject them.)

Note that Linux kernel purposely inserts a '\0' at offset len (i.e. buf[len] = 0;) when the user write len bytes to sysfs purposely for safe sscanf, as said in a comment in kernel 2.6: fs/sysfs/file.c:

static int 
fill_write_buffer(struct sysfs_buffer * buffer, const char __user * buf, size_t count)
{
    int error;

    if (!buffer->page)
        buffer->page = (char *)get_zeroed_page(GFP_KERNEL);
    if (!buffer->page)
        return -ENOMEM;

    if (count >= PAGE_SIZE)
        count = PAGE_SIZE - 1;
    error = copy_from_user(buffer->page,buf,count);
    buffer->needs_read_fill = 1;
    /* if buf is assumed to contain a string, terminate it by \0,
       so e.g. sscanf() can scan the string easily */
    buffer->page[count] = 0;
    return error ? -EFAULT : count;
}
...
static ssize_t
sysfs_write_file(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
{
    struct sysfs_buffer * buffer = file->private_data;
    ssize_t len;

    mutex_lock(&buffer->mutex);
    len = fill_write_buffer(buffer, buf, count);
    if (len > 0)
        len = flush_write_buffer(file->f_path.dentry, buffer, len);
    if (len > 0)
        *ppos += len;
    mutex_unlock(&buffer->mutex);
    return len;
}

Higher kernel version keeps the same logic (though already completely rewritten).