• Mauricio Faria de Oliveira's avatar
    apparmor: check/put label on apparmor_sk_clone_security() · 3b646abc
    Mauricio Faria de Oliveira authored
    Currently apparmor_sk_clone_security() does not check for existing
    label/peer in the 'new' struct sock; it just overwrites it, if any
    (with another reference to the label of the source sock.)
    
        static void apparmor_sk_clone_security(const struct sock *sk,
                                               struct sock *newsk)
        {
                struct aa_sk_ctx *ctx = SK_CTX(sk);
                struct aa_sk_ctx *new = SK_CTX(newsk);
    
                new->label = aa_get_label(ctx->label);
                new->peer = aa_get_label(ctx->peer);
        }
    
    This might leak label references, which might overflow under load.
    Thus, check for and put labels, to prevent such errors.
    
    Note this is similarly done on:
    
        static int apparmor_socket_post_create(struct socket *sock, ...)
        ...
                if (sock->sk) {
                        struct aa_sk_ctx *ctx = SK_CTX(sock->sk);
    
                        aa_put_label(ctx->label);
                        ctx->label = aa_get_label(label);
                }
        ...
    
    Context:
    -------
    
    The label reference count leak is observed if apparmor_sock_graft()
    is called previously: this sets the 'ctx->label' field by getting
    a reference to the current label (later overwritten, without put.)
    
        static void apparmor_sock_graft(struct sock *sk, ...)
        {
                struct aa_sk_ctx *ctx = SK_CTX(sk);
    
                if (!ctx->label)
                        ctx->label = aa_get_current_label();
        }
    
    And that is the case on crypto/af_alg.c:af_alg_accept():
    
        int af_alg_accept(struct sock *sk, struct socket *newsock, ...)
        ...
                struct sock *sk2;
                ...
                sk2 = sk_alloc(...);
                ...
                security_sock_graft(sk2, newsock);
                security_sk_clone(sk, sk2);
        ...
    
    Apparently both calls are done on their own right, especially for
    other LSMs, being introduced in 2010/2014, before apparmor socket
    mediation in 2017 (see commits [1,2,3,4]).
    
    So, it looks OK there! Let's fix the reference leak in apparmor.
    
    Test-case:
    ---------
    
    Exercise that code path enough to overflow label reference count.
    
        $ cat aa-refcnt-af_alg.c
        #include <stdio.h>
        #include <string.h>
        #include <unistd.h>
        #include <sys/socket.h>
        #include <linux/if_alg.h>
    
        int main() {
                int sockfd;
                struct sockaddr_alg sa;
    
                /* Setup the crypto API socket */
                sockfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
                if (sockfd < 0) {
                        perror("socket");
                        return 1;
                }
    
                memset(&sa, 0, sizeof(sa));
                sa.salg_family = AF_ALG;
                strcpy((char *) sa.salg_type, "rng");
                strcpy((char *) sa.salg_name, "stdrng");
    
                if (bind(sockfd, (struct sockaddr *) &sa, sizeof(sa)) < 0) {
                        perror("bind");
                        return 1;
                }
    
                /* Accept a "connection" and close it; repeat. */
                while (!close(accept(sockfd, NULL, 0)));
    
                return 0;
        }
    
        $ gcc -o aa-refcnt-af_alg aa-refcnt-af_alg.c
    
        $ ./aa-refcnt-af_alg
        <a few hours later>
    
        [ 9928.475953] refcount_t overflow at apparmor_sk_clone_security+0x37/0x70 in aa-refcnt-af_alg[1322], uid/euid: 1000/1000
        ...
        [ 9928.507443] RIP: 0010:apparmor_sk_clone_security+0x37/0x70
        ...
        [ 9928.514286]  security_sk_clone+0x33/0x50
        [ 9928.514807]  af_alg_accept+0x81/0x1c0 [af_alg]
        [ 9928.516091]  alg_accept+0x15/0x20 [af_alg]
        [ 9928.516682]  SYSC_accept4+0xff/0x210
        [ 9928.519609]  SyS_accept+0x10/0x20
        [ 9928.520190]  do_syscall_64+0x73/0x130
        [ 9928.520808]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
    
    Note that other messages may be seen, not just overflow, depending on
    the value being incremented by kref_get(); on another run:
    
        [ 7273.182666] refcount_t: saturated; leaking memory.
        ...
        [ 7273.185789] refcount_t: underflow; use-after-free.
    
    Kprobes:
    -------
    
    Using kprobe events to monitor sk -> sk_security -> label -> count (kref):
    
    Original v5.7 (one reference leak every iteration)
    
     ... (af_alg_accept+0x0/0x1c0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd2
     ... (af_alg_release_parent+0x0/0xd0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd4
     ... (af_alg_accept+0x0/0x1c0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd3
     ... (af_alg_release_parent+0x0/0xd0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd5
     ... (af_alg_accept+0x0/0x1c0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd4
     ... (af_alg_release_parent+0x0/0xd0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd6
    
    Patched v5.7 (zero reference leak per iteration)
    
     ... (af_alg_accept+0x0/0x1c0) label=0xffff9ff376c25eb0 label_refcnt=0x593
     ... (af_alg_release_parent+0x0/0xd0) label=0xffff9ff376c25eb0 label_refcnt=0x594
     ... (af_alg_accept+0x0/0x1c0) label=0xffff9ff376c25eb0 label_refcnt=0x593
     ... (af_alg_release_parent+0x0/0xd0) label=0xffff9ff376c25eb0 label_refcnt=0x594
     ... (af_alg_accept+0x0/0x1c0) label=0xffff9ff376c25eb0 label_refcnt=0x593
     ... (af_alg_release_parent+0x0/0xd0) label=0xffff9ff376c25eb0 label_refcnt=0x594
    
    Commits:
    -------
    
    [1] commit 507cad35 ("crypto: af_alg - Make sure sk_security is initialized on accept()ed sockets")
    [2] commit 4c63f83c ("crypto: af_alg - properly label AF_ALG socket")
    [3] commit 2acce6aa ("Networking") a.k.a ("crypto: af_alg - Avoid sock_graft call warning)
    [4] commit 56974a6f ("apparmor: add base infastructure for socket mediation")
    
    Fixes: 56974a6f ("apparmor: add base infastructure for socket mediation")
    Reported-by: default avatarBrian Moyles <bmoyles@netflix.com>
    Signed-off-by: default avatarMauricio Faria de Oliveira <mfo@canonical.com>
    Signed-off-by: default avatarJohn Johansen <john.johansen@canonical.com>
    3b646abc
lsm.c 49 KB