lkml.org 
[lkml]   [2015]   [Dec]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[GIT PULL] keys bugfix
    This is a resend of just the first (critical) fix.

    Please pull.


    The following changes since commit 8db7b3c54401d83a4dc370a59b8692854000ea03:

    Merge branch 'parisc-4.4-4' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux (2015-12-25 13:19:50 -0800)

    are available in the git repository at:

    git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus2

    David Howells (1):
    KEYS: Fix race between read and revoke

    security/keys/keyctl.c | 18 +++++++++---------
    1 files changed, 9 insertions(+), 9 deletions(-)

    ---

    commit b4a1b4f5047e4f54e194681125c74c0aa64d637d
    Author: David Howells <dhowells@redhat.com>
    Date: Fri Dec 18 01:34:26 2015 +0000

    KEYS: Fix race between read and revoke

    This fixes CVE-2015-7550.

    There's a race between keyctl_read() and keyctl_revoke(). If the revoke
    happens between keyctl_read() checking the validity of a key and the key's
    semaphore being taken, then the key type read method will see a revoked key.

    This causes a problem for the user-defined key type because it assumes in
    its read method that there will always be a payload in a non-revoked key
    and doesn't check for a NULL pointer.

    Fix this by making keyctl_read() check the validity of a key after taking
    semaphore instead of before.

    I think the bug was introduced with the original keyrings code.

    This was discovered by a multithreaded test program generated by syzkaller
    (http://github.com/google/syzkaller). Here's a cleaned up version:

    #include <sys/types.h>
    #include <keyutils.h>
    #include <pthread.h>
    void *thr0(void *arg)
    {
    key_serial_t key = (unsigned long)arg;
    keyctl_revoke(key);
    return 0;
    }
    void *thr1(void *arg)
    {
    key_serial_t key = (unsigned long)arg;
    char buffer[16];
    keyctl_read(key, buffer, 16);
    return 0;
    }
    int main()
    {
    key_serial_t key = add_key("user", "%", "foo", 3, KEY_SPEC_USER_KEYRING);
    pthread_t th[5];
    pthread_create(&th[0], 0, thr0, (void *)(unsigned long)key);
    pthread_create(&th[1], 0, thr1, (void *)(unsigned long)key);
    pthread_create(&th[2], 0, thr0, (void *)(unsigned long)key);
    pthread_create(&th[3], 0, thr1, (void *)(unsigned long)key);
    pthread_join(th[0], 0);
    pthread_join(th[1], 0);
    pthread_join(th[2], 0);
    pthread_join(th[3], 0);
    return 0;
    }

    Build as:

    cc -o keyctl-race keyctl-race.c -lkeyutils -lpthread

    Run as:

    while keyctl-race; do :; done

    as it may need several iterations to crash the kernel. The crash can be
    summarised as:

    BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
    IP: [<ffffffff81279b08>] user_read+0x56/0xa3
    ...
    Call Trace:
    [<ffffffff81276aa9>] keyctl_read_key+0xb6/0xd7
    [<ffffffff81277815>] SyS_keyctl+0x83/0xe0
    [<ffffffff815dbb97>] entry_SYSCALL_64_fastpath+0x12/0x6f

    Reported-by: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: David Howells <dhowells@redhat.com>
    Tested-by: Dmitry Vyukov <dvyukov@google.com>
    Cc: stable@vger.kernel.org
    Signed-off-by: James Morris <james.l.morris@oracle.com>

    diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
    index fb111ea..1c3872a 100644
    --- a/security/keys/keyctl.c
    +++ b/security/keys/keyctl.c
    @@ -751,16 +751,16 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)

    /* the key is probably readable - now try to read it */
    can_read_key:
    - ret = key_validate(key);
    - if (ret == 0) {
    - ret = -EOPNOTSUPP;
    - if (key->type->read) {
    - /* read the data with the semaphore held (since we
    - * might sleep) */
    - down_read(&key->sem);
    + ret = -EOPNOTSUPP;
    + if (key->type->read) {
    + /* Read the data with the semaphore held (since we might sleep)
    + * to protect against the key being updated or revoked.
    + */
    + down_read(&key->sem);
    + ret = key_validate(key);
    + if (ret == 0)
    ret = key->type->read(key, buffer, buflen);
    - up_read(&key->sem);
    - }
    + up_read(&key->sem);
    }

    error2:
    --
    James Morris
    <jmorris@namei.org>



    \
     
     \ /
      Last update: 2015-12-28 05:21    [W:3.078 / U:1.800 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site