oss-security - Potential regression and/or incomplete fix for CVE-2017-12762

Follow @Openwall on Twitter for new release announcements and other news

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJvHH_QwWT5HBMXEH8hVgXxm8xCYzhJfRTWAijAr0LLWQn4gdg@mail.gmail.com>
Date: Tue, 11 Feb 2020 19:27:18 +0000
From: Ibrahim el-sayed <i.elsayed92@...il.com>
To: oss-security@...ts.openwall.com
Subject: Potential regression and/or incomplete fix for CVE-2017-12762

Hello,
I stumbled upon CVE-2017-12762 which has a CVSS score of 10 and I think the
patch is incomplete and it might have regressed in the stable version. I am
probably wrong hence this email to see if anyone familiar with this CVE and
the fix and tell me if I am wrong.

## Incomplete patch
The patch can be found in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f5af546e6acc30f075828cb58c7f09665033967

This is the vulnerable piece of code
```
char *
isdn_net_newslave(char *parm)
{
    char *p = strchr(parm, ',');
    isdn_net_dev *n;
    char newname[10];

    if (p) {
        /* Slave-Name MUST not be empty or overflow 'newname' */
        *if (strscpy(newname, p + 1, sizeof(newname)) <= 0)*
            return NULL;
        *p = 0;
        /* Master must already exist */
        if (!(n = isdn_net_findif(parm)))
            return NULL;
        /* Master must be a real interface, not a slave */
        if (n->local->master)
            return NULL;
        /* Master must not be started yet */
        if (isdn_net_device_started(n))
            return NULL;
        return (isdn_net_new(newname, n->dev));
    }
    return NULL;
}
```

I think it is incomplete and can lead to reading out of bound since it does
*not* check if the src buffer (p) in this case has 10 bytes at least. The
fix assumes p has 10 bytes and copies that into newname. The fix
uses strscpy (
https://github.com/torvalds/linux/blob/cc12071ff39060fc2e47c58b43e249fe0d0061ee/lib/string.c#L180)
which
based on its code it starts copying from count and decrements to zero.
which in this case if param is a string similar to "aa,", then p will point
to the last byte of the string, strscpy will copy from p+1+sizeof(newname)
= p+1+10 -[to]-> p+1 which would allow reading 10 bytes after the buffer p

I do not know if param can end with "," or if there is any validation
checks that it does not end with "," hence this might not be a bug but only
bad practice.

## Regression
I looked quickly into latest version for the kernel v3.16.81 and it seems
that the patch was probably reverted as the code matches exactly to the
vulnerable version to the CVE (
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/isdn/i4l/isdn_net.c?id=v3.16.81#n2646
)
Not sure if the fix was reworked but wanted to surface that issue as well

Ibrahim

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.