add seccomp argument filtering

[jdstrand]

Code changes are from lp:~jdstrand/ubuntu-core-launcher/seccom-arg-filtering
but update for README -> README.md and use of shellcheck

[jdstrand]

@tyhicks - please review (this obsoletes https://code.launchpad.net/~jdstrand/ubuntu-core-launcher/seccom-arg-filtering/+merge/291069)

[zyga]

This should be /var/lib/snapd/seccomp/profiles

[jdstrand]

Fixed committed.

[zyga]

Can we please add AF_CAN here

[jdstrand]

Done.

[zyga]

Any reason why we're not doing strncmp(buf_token, "=", 1) == 0 here?

[tyhicks]

I agree that we should compare against "=". However, I'd prefer strcmp(buf_token, "=") == 0. The first reason is because we already know that buf_token is NUL terminated because we've already called strcmp() and strlen() on it. The second reason is because strncmp(buf_token, "=", 1) would match "=abcd123" because only the first character is compared.

[jdstrand]

I'm not checking for '=' here because '=NNN' is not part of the syntax (you just specify 'NNN' instead). This is checking if the token has a length of 1, parse it as a number and let read_number() set errno. The comment tried to mention that, but it used 'syscall 1'-- I'll update that to use 'syscall N' and adjust the comment to make it more clear.

[zyga]

Any reason we are not doing strncmp(buf_token, "!", 1) == 0 here?

[jdstrand]

No reason. I agree it would make the code easier to read. Fix committed here and other places you mentioned.

[zyga]

And here

[zyga]

And here

[zyga]

I suspect shellcheck will complain about a few of those now.

[jdstrand]

I made it all work with shellcheck before performing the PR.

[zyga]

Any reason why we're changing this?

[jdstrand]

No. It was a cleanup after a sed to fix some shellcheck stuff and I left this change for consistency in other tests.

[zyga]

A few small comments inline. Two comments here:

• we should use static functions more often, function names are becoming very generic quickly (map_...)
• we should do a pass over the whole tree to unify error handling messages ("cannot ....") vs what we do now.

[tyhicks]

Hard-coding 82 will cause a maintenance issue down the road. I'd like to see a struct defined that holds key value pairs (maybe even use an array of ENTRY) and then have an array of structs that are iterated through. sizeof() could be used to determine the size of the array.

[jdstrand]

@tyhicks - FYI, I went with a linked list of structs. I then populate the htab with the linked list and therefore do not need to hardcode a map length.

[tyhicks]

We're abusing e.data here. It is a void pointer and is intended to point to a char array or a struct or something along those lines. We're using a pointer to store a number. It'll work for most cases but we'll run into problems if/when we want to store a data type that takes more space than a pointer requires. One such example is the off_t offset parameter of lseek(). The e.data void pointer wouldn't be able to represent all possible values of an off_t.

Most system call parameters that we'll want to filter can be represented by an off_t so I won't block on this limitation but it at least needs to be clearly documented in the code.

[jdstrand]

This was a good catch and as it turns out, we fail the compile on i386 because of our hardening options. I'll rework this so this is no longer an issue.

[jdstrand]

I've fixed this by allocating a void pointer to hold a pointer to scmp_datum_t. This means that we can reliably use scmp_datum_t everywhere and makes it work on 32bit.

[tyhicks]

If we make a typo in the list of calls to map_add() and call it twice with the same key value, hsearch_r() will silently ignore the ENTER operation. Lets be a little proactive here and make sure that doesn't happen. We'll need to die() if ep->data does not equal e.data.

[jdstrand]

I made this change in the new code with:

    while (p != NULL) {
        errno = 0;
        if (hsearch_r(*p->e, ENTER, &p->ep, &sc_map_htab) == 0)
            die("hsearch_r failed");

        if (&p->ep == NULL)
            die("could not initialize map");
        else {
            scmp_datum_t *d = p->ep->data;
            if (*d != *(scmp_datum_t *) p->e->data)
                die("e->data != ep->data after ENTER");
            printf("ep->data=%lu, e->data=%lu\n", *d, *(scmp_datum_t *) p->e->data);
        }

        p = p->next;
    }

and when I sc_map_add() something twice, hsearch_r doesn't operate the way you describe-- the data is the same.

[tyhicks]

This can be done in a future commit but "stringification" could clean up these map_add()s and help prevent typos in the future:

https://gcc.gnu.org/onlinedocs/cpp/Stringification.html

#include <stdio.h>
#include <sys/socket.h>

#define kvpair_print(X) kvp(#X, X)

void kvp(const char *key, int value)
{
    printf("%s = %d\n", key, value);
}

int main(void)
{
    kvpair_print(AF_UNIX);
    kvpair_print(SOCK_RAW);
}

That program prints:

$ ./stringification
AF_UNIX = 1
SOCK_RAW = 3

[jdstrand]

I went ahead and used stringification-- it made the code a lot cleaner and easier to read:

#define sc_map_add(X) sc_map_add_kvp(#X, X)
...
void sc_map_add_kvp(const char *key, scmp_datum_t value)
...
sc_map_add(AF_UNIX);

Note that I am passing scmp_datum_t as the second arg to sc_map_add_kvp(). I think this is ok because it is a widening conversion (eg, AF_UNIX to scmp_datum_t is still '1'). Let me known if I'm missing something.

[tyhicks]

Lets switch to strcmp() since we know buf_token is NUL terminated and the current test only compares the first two chars and would match ">=ZZZZ".

[tyhicks]

Same here. strncmp() -> strcmp()

[jdstrand]

@tyhicks - regarding strncmp vs strcmp, this is because the syntax does not have a space between the operator and the operand. Ie, these are valid: >5 and <=10 but this is not >= 1. As such, the parsing is checking what the string starts with and if it matches, sends the rest of the string to read_number() and lets it handle setting errno.

[zyga]

Please merge with master to use autoconf/automake for this.

[jdstrand]

Ok, I think I addressed everyone's comments. Thanks for the reviews so far!

[tyhicks]

malloc(sizeof(struct foo)) is somewhat dangerous of someone ever changes the type of *node in the future.

malloc(sizeof(*node)) is the safer option.

Also, I find it surprising that you need to cast the return value. Please drop that, if possible.

[jdstrand]

Done.

[tyhicks]

Optional suggestion: remove the declaration of *p and just do val = *(ep->data)

[jdstrand]

This yields a compiler error but I can use val = *((scmp_datum_t *) ep->data);

[tyhicks]

and here

[jdstrand]

Done (with node->e->data = malloc(sizeof(scmp_datum_t *));)

[tyhicks]

This is incorrect and shows the potential problem. This is allocating size of a pointer instead of the size of the type being used to store the data. But ignore this for now because I think I have a better fix to suggest in a higher-level comment.

[tyhicks]

Same comments here as the other malloc() above.

[jdstrand]

Done.

[tyhicks]

This casting is really odd since it is on the left side of the assignment. It is covering up the fact that this PR has the same problem as the first PR.

node->e->data is of type (void *). value is of type scmp_datum_t which, according to libseccomp's seccomp.h, is a uint64_t:

typedef uint64_t scmp_datum_t;

We're still assigning a 64 bit type to a small type.

[tyhicks]

You'll have to declare a new struct, which contains a scmp_datum_t field. You'll assign a pointer to that struct to ENTRY.data when adding to the map.

[jdstrand]

I think there is some confusion on what I'm doing here. Consider:

node->e->data = malloc(sizeof(scmp_datum_t *));
if (node->e->data == NULL)
        die("Out of memory creating e->data");
*(scmp_datum_t *) node->e->data = value;

What I'm doing is malloc()ing something the size of a scmp_datum_t and assigning the address of that in node->e->data, which is a void pointer. Then I dereference node->e->data and assign it the variable value. Since node->e->data is a void pointer, the (scmp_datum_t *) is used on the left hand sign to say that node->e->data is a pointer to scmp_datum_t, and the * in front says to dereference that pointer. In this manner, we are not assigning a 64 bit type to a small type. node->e->data = malloc(sizeof(scmp_datum_t *)) is pointer to pointer, and *(scmp_datum_t *) node->e->data = value is 64 bit to 64 bit, therefore the sizes are all correct, and we can demonstrate how sc_map_search() later will get the correct value and that the sizes are correct in the attached file. Here are the relevant bits:

scmp_datum_t get_val(ENTRY *e) {
    return *(scmp_datum_t *) e->data;
}

int main(int argc, char *argv[]) {
    ...
    scmp_datum_t value = 4294967297; // bigger than 2^32
    ...
    node->e->data = malloc(sizeof(scmp_datum_t *));
    ...
    *(scmp_datum_t *) node->e->data = value;

    printf("sizeof(&value)=\t\t\t\t\t%u\n", sizeof(&value));
    printf("sizeof(value)=\t\t\t\t\t%u\n", sizeof(value));
    printf("value=\t\t\t\t\t\t%llu\n", value);

    printf("sizeof(node->e->data)=\t\t\t\t%u\n", sizeof(node->e->data));
    printf("sizeof(*(scmp_datum_t *) node->e->data)=\t%u\n", sizeof(*(scmp_datum_t *) node->e->data));
    printf("*(scmp_datum_t *)node->e->data=\t\t\t%llu\n", *(scmp_datum_t *)node->e->data);

    printf("get_val(node->e)=\t\t\t\t%llu\n", get_val(node->e));

Now run the program on a 32 bit system (eg, an i386 chroot) since it shows the different sizes:

$ gcc ./prove-sizes.c && ./a.out
sizeof(&value)=                             4
sizeof(value)=                              8
value=                                      4294967297
sizeof(node->e->data)=                      4
sizeof(*(scmp_datum_t *) node->e->data)=    8
*(scmp_datum_t *)node->e->data=             4294967297
get_val(node->e)=                           4294967297

[jdstrand]

Meh, I couldn't get github to attach the file. Here it is:

$ cat ./prove-sizes.c
#include <stdio.h>
#include <stdlib.h>
#include <seccomp.h>
#include <search.h>

struct sc_map_entry {
    ENTRY *e;
    ENTRY *ep;
    struct sc_map_entry *next;
};

scmp_datum_t get_val(ENTRY *e) {
    return *(scmp_datum_t *) e->data;
}

int main(int argc, char *argv[])
{
    scmp_datum_t value = 4294967297; // bigger than 2^32

    struct sc_map_entry *node;
    node = malloc(sizeof(*node));
    if (node == NULL)
        perror("no memory");

    node->e = malloc(sizeof(node->e));
    if (node->e == NULL)
        perror("no memory");

    node->e->data = malloc(sizeof(scmp_datum_t *));
    if (node->e->data == NULL)
        perror("no memory");
    *(scmp_datum_t *) node->e->data = value;

    printf("sizeof(&value)=\t\t\t\t\t%u\n", sizeof(&value));
    printf("sizeof(value)=\t\t\t\t\t%u\n", sizeof(value));
    printf("value=\t\t\t\t\t\t%llu\n", value);

    printf("sizeof(node->e->data)=\t\t\t\t%u\n", sizeof(node->e->data));
    printf("sizeof(*(scmp_datum_t *) node->e->data)=\t%u\n", sizeof(*(scmp_datum_t *) node->e->data));
    printf("*(scmp_datum_t *)node->e->data=\t\t\t%llu\n", *(scmp_datum_t *)node->e->data);

    printf("get_val(node->e)=\t\t\t\t%llu\n", get_val(node->e));
    return 0;
}

[tyhicks]

Running this program through valgrind shows quite a few issues (remove the printf()s first because incorrect modifiers are used that generate additional compiler and valgrind warnings).

[tyhicks]

I'm going to hold off on reviewing the rest since changes to the core of this PR will need to change.

[zyga]

Thank you for catching that @tyhicks

[jdstrand]

At this point I made the few small requested cleanups and shown how the assignments are correct. This should be ready to review by @tyhicks (but I'm going to start merging in all of Zygmunt's changes so it is commitable again.

[jdstrand]

Actually, I'm going to hold off on those merges so everything is clear when Tyler is ready to review again.

[tyhicks]

There are a few small changes that I'd like to see. Rather than drag this PR out any longer, I proposed them in jdstrand#1.

With those changes, this looks ok to me.

[tyhicks]

Looks good!

[jdstrand]

Note that for some reason the testsuite doesn't run if I go into an i386 schroot, install all the build-deps and then do:

$ debian/rules build

This is not a regression in this PR. If I go into an amd64 schroot and do:

$ debian/rules build

then go into an i386 chroot and do:

$ make clean
$ make check

the tests pass on i386. Note that in the past that the testsuite failed on i386, but I fixed that in this PR since having 32 bit and 64 bit testsuite runs is useful. Perhaps 32 bit was disabled somewhere else with the change to autoconf?

[tyhicks]

@jdstrand looks like tests were disable on all architectures except for amd64: 02205bc

[jdstrand]

Thanks @tyhicks that was helpful. Re-enabled tests on i686.

[jdstrand]

@zyga - this is ready to land now. If you have instructions on how to do more testing with a snapd that uses snap-confine, I could do some more testing.

[zyga]

@jdstrand just install the package on your system.

[jdstrand]

Verified that seccomp correctly:

• blocks setpriority when it isn't in the policy
• allows setpriority when it is
• blocks setpriority when args are specified in the policy that don't match the caller
• allows setpriority when args are specified in the policy that match the caller

[zyga]

And we crash because val is NULL and gets dereferenced below.

[jdstrand]

Meh, this was introduced in the last code commit (7902948). Nice catch!

[zyga]

This function should be static

[zyga]

This function should be static

[jdstrand]

Thanks for the review @zyga. I made the requested changes and also made the sc_map_* functions static. They are almost generalized, but sc_map_init() would need work to be used outside of this file.

[jdstrand]

retest this please

[zyga]

Hmm, I need to adjust the secret handling in snap-confine to let tests pass for forks

[zyga]

I'm merging this. I don't want to delay on travis config