From 0977f0876af186975d3861c53b8431a80a27fa83 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Tue, 9 May 2023 10:38:21 +0200 Subject: gluten: check limits Add bounds checking: - if offset is larger then the maximum per offset type - if memcpy is reading/writing inside gluten --- common/gluten.h | 48 +++++++++++++++++-- operations.c | 16 +++---- tests/unit/Makefile | 10 ++++ tests/unit/test_errors.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++ tests/unit/testutil.h | 2 + 5 files changed, 183 insertions(+), 11 deletions(-) create mode 100644 tests/unit/test_errors.c diff --git a/common/gluten.h b/common/gluten.h index b1723ca..d1d3c61 100644 --- a/common/gluten.h +++ b/common/gluten.h @@ -26,7 +26,30 @@ extern struct seccomp_data anonymous_seccomp_data; MAX(MAX(MAX(DATA_SIZE, RO_DATA_SIZE), INST_MAX), \ ARRAY_SIZE(anonymous_seccomp_data.args)) -#define seccomp_offset_args(x) (sizeof(uint64_t) / sizeof(uint16_t)) * (x) +#define check_gluten_limits(g, v, size) \ + do { \ + struct gluten_offset off = { v.type, v.offset + (size) }; \ + if (!is_offset_valid(off)) \ + die(" invalid offset: %d", off.offset); \ + } while (0) + +#define gluten_write(g, dst, src) \ + do { \ + void *p = gluten_write_ptr((g), (dst)); \ + check_gluten_limits((g), (dst), sizeof((src))); \ + if (p == NULL) \ + die(" invalid type of offset"); \ + memcpy(p, &(src), sizeof(src)); \ + } while (0) + +#define gluten_read(s, g, dst, src, size) \ + do { \ + const void *p = gluten_ptr((s), (g), (src)); \ + check_gluten_limits((g), (src), (size)); \ + if (p == NULL) \ + die(" invalid type of offset"); \ + memcpy(&(dst), p, (size)); \ + } while (0) enum gluten_offset_type { OFFSET_RO_DATA = 0, @@ -195,6 +218,22 @@ struct gluten { BUILD_BUG_ON(INST_SIZE < INST_MAX * sizeof(struct op)) +static inline bool is_offset_valid(const struct gluten_offset x) +{ + switch (x.type) { + case OFFSET_DATA: + return x.offset < DATA_SIZE; + case OFFSET_RO_DATA: + return x.offset < RO_DATA_SIZE; + case OFFSET_INSTRUCTION: + return x.offset < INST_SIZE; + case OFFSET_SECCOMP_DATA: + return x.offset < 6; + default: + return false; + } +} + #ifdef COOKER static inline void *gluten_ptr(struct gluten *g, const struct gluten_offset x) #else @@ -202,7 +241,8 @@ static inline void *gluten_write_ptr(struct gluten *g, const struct gluten_offset x) #endif { - /* TODO: Boundary checks */ + if (!is_offset_valid(x)) + die(" invalid offset: %d", x.offset); switch (x.type) { case OFFSET_DATA: @@ -223,6 +263,9 @@ static inline const void *gluten_ptr(const struct seccomp_data *s, struct gluten *g, const struct gluten_offset x) { + if (!is_offset_valid(x)) + die(" invalid offset: %d", x.offset); + switch (x.type) { case OFFSET_DATA: return g->data + x.offset; @@ -237,5 +280,4 @@ static inline const void *gluten_ptr(const struct seccomp_data *s, } } #endif - #endif /* COMMON_GLUTEN_H */ diff --git a/operations.c b/operations.c index 870ecf1..bf03ab8 100644 --- a/operations.c +++ b/operations.c @@ -177,6 +177,7 @@ int op_load(const struct seccomp_notif *req, int notifier, struct gluten *g, ret = -1; goto out; } + check_gluten_limits(&g, load->dst, load->size); if (pread(fd, gluten_write_ptr(g, load->dst), load->size, *src) < 0) { perror("pread"); return -1; @@ -232,7 +233,7 @@ int op_call(const struct seccomp_notif *req, int notifier, struct gluten *g, * reference */ if (op->has_ret) - memcpy(gluten_write_ptr(g, op->ret), &c.ret, sizeof(c.ret)); + gluten_write(g, op->ret, c.ret); return 0; } @@ -263,7 +264,7 @@ int op_return(const struct seccomp_notif *req, int notifier, struct gluten *g, resp.flags = 0; resp.error = 0; - memcpy(&resp.val, gluten_ptr(NULL, g, op->val), sizeof(resp.val)); + gluten_read(NULL, g, resp.val, op->val, sizeof(resp.val)); if (send_target(&resp, notifier) == -1) return -1; @@ -299,10 +300,8 @@ static int do_inject(const struct seccomp_notif *req, int notifier, resp.newfd_flags = 0; resp.id = req->id; - memcpy(&resp.newfd, gluten_ptr(NULL, g, op->new_fd), - sizeof(resp.newfd)); - memcpy(&resp.srcfd, gluten_ptr(NULL, g, op->new_fd), - sizeof(resp.srcfd)); + gluten_read(NULL, g, resp.newfd, op->new_fd, sizeof(resp.newfd)); + gluten_read(NULL, g, resp.srcfd, op->old_fd, sizeof(resp.srcfd)); if (atomic) resp.flags |= SECCOMP_ADDFD_FLAG_SEND; @@ -351,8 +350,9 @@ int op_resolve_fd(const struct seccomp_notif *req, int notifier, (void)notifier; - memcpy(&path, gluten_ptr(NULL, g, op->path), op->path_size); - memcpy(&fd, gluten_ptr(NULL, g, op->fd), sizeof(fd)); + + gluten_read(NULL, g, path, op->path, sizeof(op->path_size)); + gluten_read(NULL, g, fd, op->fd, sizeof(fd)); snprintf(fdpath, PATH_MAX, "/proc/%d/fd/%d", req->pid, fd); if ((nbytes = readlink(fdpath, buf, op->path_size)) < 0) { diff --git a/tests/unit/Makefile b/tests/unit/Makefile index 202e198..966ae7d 100644 --- a/tests/unit/Makefile +++ b/tests/unit/Makefile @@ -23,6 +23,10 @@ HEADERS_OP := $(COMMON_DIR)/gluten.h $(OP_DIR)/operations.h \ $(COMMON_DIR)/common.h testutil.h $(COMMON_DIR)/util.h SRCS_OP := $(COMMON_DIR)/common.c $(OP_DIR)/operations.c util.c $(COMMON_DIR)/util.c +HEADERS_ERROR := $(COMMON_DIR)/gluten.h $(OP_DIR)/operations.h \ + $(COMMON_DIR)/common.h testutil.h $(COMMON_DIR)/util.h +SRCS_ERROR := $(COMMON_DIR)/common.c $(OP_DIR)/operations.c util.c $(COMMON_DIR)/util.c + TARGET := $(shell $(CC) -dumpmachine) TARGET_ARCH := $(shell echo $(TARGET) | cut -f1 -d- | tr [A-Z] [a-z]) TARGET_ARCH := $(shell echo $(TARGET_ARCH) | sed 's/powerpc/ppc/') @@ -67,5 +71,11 @@ build-operations: test_operations.c $(SRCS_OP) $(HEADERS_OP) test-operations: build-operations ./operations +build-error-checks: test_errors.c $(SRCS_ERROR) $(HEADERS_ERROR) + $(CC) $(CFLAGS) -o error_checks $(SRCS_ERROR) \ + test_errors.c + +test-error-checks: build-error-checks + ./error_checks clean: rm -f operations op-call filter filter-build diff --git a/tests/unit/test_errors.c b/tests/unit/test_errors.c new file mode 100644 index 0000000..d00d42e --- /dev/null +++ b/tests/unit/test_errors.c @@ -0,0 +1,118 @@ +#include +#include +#include +#include + +#include + +#include "operations.h" +#include "common/common.h" +#include "common/gluten.h" +#include "testutil.h" + +static void setup_error_check() +{ + at = mmap(NULL, sizeof(struct args_target), PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_ANONYMOUS, -1, 0); + at->check_fd = false; + at->nr = __NR_getpid; + at->install_filter = install_notification_filter; + setup(); +} + +struct gluten_offset test_max_size_data[] = { + { OFFSET_DATA, DATA_SIZE }, + { OFFSET_RO_DATA, RO_DATA_SIZE }, + { OFFSET_SECCOMP_DATA, 6 }, + { OFFSET_INSTRUCTION, INST_SIZE }, +}; + +START_TEST(test_bound_check) +{ + struct op ops[] = { + { OP_RETURN, { { 0 } } }, + }; + ops[0].op.ret.val.offset = test_max_size_data[_i].offset; + ops[0].op.ret.val.type = test_max_size_data[_i].type; + + eval(&gluten, ops, &req, notifyfd); +} + +START_TEST(test_write_op_return) +{ + struct op ops[] = { + { OP_CALL, + { .call = { .nr = __NR_getpid, + .has_ret = true, + .ret = { OFFSET_DATA, DATA_SIZE - 1 } } } }, + }; + + eval(&gluten, ops, &req, notifyfd); +} + +START_TEST(test_write_op_load) +{ + char a[30]; + struct op ops[] = { + { OP_LOAD, + { .load = { { OFFSET_SECCOMP_DATA, 1 }, + { OFFSET_DATA, DATA_SIZE - 1 }, + sizeof(a) } } }, + }; + + eval(&gluten, ops, &req, notifyfd); +} + +START_TEST(test_read_op_return) +{ + struct op ops[] = { + { OP_RETURN, { { 0 } } }, + }; + ops[0].op.ret.val.offset = test_max_size_data[_i].offset - 1; + ops[0].op.ret.val.type = test_max_size_data[_i].type; + + eval(&gluten, ops, &req, notifyfd); +} + +Suite *error_suite(void) +{ + Suite *s; + TCase *bounds, *gwrite, *gread; + + s = suite_create("Error handling"); + + bounds = tcase_create("bound checks"); + tcase_add_loop_exit_test(bounds, test_bound_check, EXIT_FAILURE, 0, + sizeof(test_max_size_data) / + sizeof(test_max_size_data[0])); + suite_add_tcase(s, bounds); + + gwrite = tcase_create("write gluten"); + tcase_add_checked_fixture(gwrite, setup_error_check, teardown); + tcase_add_exit_test(gwrite, test_write_op_return, EXIT_FAILURE); + tcase_add_exit_test(gwrite, test_write_op_load, EXIT_FAILURE); + suite_add_tcase(s, gwrite); + + gread = tcase_create("read gluten"); + tcase_add_loop_exit_test(gread, test_read_op_return, EXIT_FAILURE, 0, + sizeof(test_max_size_data) / + sizeof(test_max_size_data[0])); + suite_add_tcase(s, gread); + + return s; +} + +int main(void) +{ + int no_failed = 0; + Suite *s; + SRunner *runner; + + s = error_suite(); + runner = srunner_create(s); + + srunner_run_all(runner, CK_VERBOSE); + no_failed = srunner_ntests_failed(runner); + srunner_free(runner); + return (no_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE; +} diff --git a/tests/unit/testutil.h b/tests/unit/testutil.h index 45fe08f..ec881c7 100644 --- a/tests/unit/testutil.h +++ b/tests/unit/testutil.h @@ -20,6 +20,8 @@ static inline void *test_gluten_write_ptr(struct gluten *g, const struct gluten_offset x) { + ck_assert_msg(is_offset_valid(x), "offset out of bounds"); + switch (x.type) { case OFFSET_DATA: return (char *)g->data + x.offset; -- cgit v1.2.3