minijail: Check for repeat syscall definitions
Add an option that allows for checking for duplicate syscall
definitions.
Add as a compile-time option and filter_option. If this option is on:
Maintain a data structure throughout seccomp policy syscall filter
parsing that keeps track of syscalls that have already been encountered
and where they were defined. Use this structure to tell when there are
duplicate syscall policy definitions and warn the user.
Write a unit test that checks that compile_file will return -1 if there
is a repeat syscall policy definition. Also change existing tests to
reflect this behavior.
Bug: None
TEST=built and ran unit tests
Change-Id: I3f5da9f926006dc7498d4a6510dda5aa5aedd1a3
diff --git a/Android.bp b/Android.bp
index 482f6c7..68797a9 100644
--- a/Android.bp
+++ b/Android.bp
@@ -35,6 +35,7 @@
cflags: [
"-D_FILE_OFFSET_BITS=64",
"-DALLOW_DEBUG_LOGGING",
+ "-DALLOW_DUPLICATE_SYSCALLS",
"-DDEFAULT_PIVOT_ROOT=\"/var/empty\"",
"-Wall",
"-Werror",
diff --git a/Makefile b/Makefile
index a7c4357..ce240a7 100644
--- a/Makefile
+++ b/Makefile
@@ -37,6 +37,11 @@
CPPFLAGS += -DUSE_EXIT_ON_DIE
endif
+# Setting this flag allows duplicate syscalls definitions for seccomp filters.
+ifeq ($(ALLOW_DUPLICATE_SYSCALLS),yes)
+CPPFLAGS += -DALLOW_DUPLICATE_SYSCALLS
+endif
+
MJ_COMMON_FLAGS = -Wunused-parameter -Wextra -Wno-missing-field-initializers
CFLAGS += $(MJ_COMMON_FLAGS)
CXXFLAGS += $(MJ_COMMON_FLAGS)
diff --git a/libminijail.c b/libminijail.c
index 74f6464..92f16d9 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -1063,6 +1063,9 @@
filteropts.allow_syscalls_for_logging =
filteropts.allow_logging && !seccomp_ret_log_available();
+ /* Whether to fail on duplicate syscalls. */
+ filteropts.allow_duplicate_syscalls = allow_duplicate_syscalls();
+
if (compile_filter(filename, policy_file, fprog, &filteropts)) {
free(fprog);
return -1;
diff --git a/parse_seccomp_policy.cc b/parse_seccomp_policy.cc
index 38fcbee..000b80d 100644
--- a/parse_seccomp_policy.cc
+++ b/parse_seccomp_policy.cc
@@ -82,6 +82,7 @@
.action = ACTION_RET_KILL,
.allow_logging = 0,
.allow_syscalls_for_logging = 0,
+ .allow_duplicate_syscalls = allow_duplicate_syscalls(),
};
struct sock_fprog fp;
diff --git a/syscall_filter.c b/syscall_filter.c
index c573854..fcdbaa8 100644
--- a/syscall_filter.c
+++ b/syscall_filter.c
@@ -161,12 +161,47 @@
append_filter_block(head, filter, len);
}
+void copy_parser_state(struct parser_state *src, struct parser_state *dest)
+{
+ const char *filename = strdup(src->filename);
+ if (!filename)
+ pdie("strdup(src->filename) failed");
+
+ dest->line_number = src->line_number;
+ dest->filename = filename;
+}
+
+/*
+ * Inserts the current state into the array of previous syscall states at the
+ * index |ind| if it is a newly encountered syscall. Returns true if it is a
+ * newly encountered syscall and false if it is a duplicate.
+ */
+bool insert_and_check_duplicate_syscall(struct parser_state **previous_syscalls,
+ struct parser_state *state, size_t ind)
+{
+ if (ind >= get_num_syscalls()) {
+ die("syscall index %zu out of range: %zu total syscalls", ind,
+ get_num_syscalls());
+ }
+ struct parser_state *prev_state_ptr = previous_syscalls[ind];
+ if (prev_state_ptr == NULL) {
+ previous_syscalls[ind] = calloc(1, sizeof(struct parser_state));
+ if (!previous_syscalls[ind])
+ die("could not allocate parser_state buffer");
+ copy_parser_state(state, previous_syscalls[ind]);
+ return true;
+ }
+ return false;
+}
+
void allow_logging_syscalls(struct filter_block *head)
{
unsigned int i;
+
for (i = 0; i < log_syscalls_len; i++) {
warn("allowing syscall: %s", log_syscalls[i]);
- append_allow_syscall(head, lookup_syscall(log_syscalls[i]));
+ append_allow_syscall(head,
+ lookup_syscall(log_syscalls[i], NULL));
}
}
@@ -580,6 +615,7 @@
struct filter_block *head, struct filter_block **arg_blocks,
struct bpf_labels *labels,
const struct filter_options *filteropts,
+ struct parser_state **previous_syscalls,
unsigned int include_level)
{
/* clang-format off */
@@ -643,6 +679,7 @@
}
if (compile_file(filename, included_file, head,
arg_blocks, labels, filteropts,
+ previous_syscalls,
include_level + 1) == -1) {
compiler_warn(&state, "'@include %s' failed",
filename);
@@ -674,7 +711,8 @@
}
syscall_name = strip(syscall_name);
- int nr = lookup_syscall(syscall_name);
+ size_t ind = 0;
+ int nr = lookup_syscall(syscall_name, &ind);
if (nr < 0) {
compiler_warn(&state, "nonexistent syscall '%s'",
syscall_name);
@@ -697,6 +735,16 @@
goto free_line;
}
+ if (!insert_and_check_duplicate_syscall(previous_syscalls,
+ &state, ind)) {
+ if (!filteropts->allow_duplicate_syscalls)
+ ret = -1;
+ compiler_warn(&state, "syscall %s redefined here",
+ lookup_syscall_name(nr));
+ compiler_warn(previous_syscalls[ind],
+ "previous definition here");
+ }
+
/*
* For each syscall, add either a simple ALLOW,
* or an arg filter block.
@@ -769,6 +817,14 @@
struct filter_block *head = new_filter_block();
struct filter_block *arg_blocks = NULL;
+ /*
+ * Create the data structure that will keep track of what system
+ * calls we have already defined if the option is true.
+ */
+ size_t num_syscalls = get_num_syscalls();
+ struct parser_state **previous_syscalls =
+ calloc(num_syscalls, sizeof(*previous_syscalls));
+
/* Start filter by validating arch. */
struct sock_filter *valid_arch = new_instr_buf(ARCH_VALIDATION_LEN);
size_t len = bpf_validate_arch(valid_arch);
@@ -788,7 +844,8 @@
allow_logging_syscalls(head);
if (compile_file(filename, initial_file, head, &arg_blocks, &labels,
- filteropts, 0 /* include_level */) != 0) {
+ filteropts, previous_syscalls,
+ 0 /* include_level */) != 0) {
warn("compile_filter: compile_file() failed");
ret = -1;
goto free_filter;
@@ -864,6 +921,7 @@
free_block_list(head);
free_block_list(arg_blocks);
free_label_strings(&labels);
+ free_previous_syscalls(previous_syscalls);
return ret;
}
@@ -897,3 +955,16 @@
free(prev);
}
}
+
+void free_previous_syscalls(struct parser_state **previous_syscalls)
+{
+ size_t num_syscalls = get_num_syscalls();
+ for (size_t i = 0; i < num_syscalls; i++) {
+ struct parser_state *state = previous_syscalls[i];
+ if (state) {
+ free((char *)state->filename);
+ free(state);
+ }
+ }
+ free(previous_syscalls);
+}
diff --git a/syscall_filter.h b/syscall_filter.h
index e3435b5..304f8c0 100644
--- a/syscall_filter.h
+++ b/syscall_filter.h
@@ -9,6 +9,8 @@
#ifndef SYSCALL_FILTER_H
#define SYSCALL_FILTER_H
+#include <stdbool.h>
+
#include "bpf.h"
#ifdef __cplusplus
@@ -40,6 +42,7 @@
enum block_action action;
int allow_logging;
int allow_syscalls_for_logging;
+ bool allow_duplicate_syscalls;
};
struct bpf_labels;
@@ -54,6 +57,7 @@
struct filter_block *head, struct filter_block **arg_blocks,
struct bpf_labels *labels,
const struct filter_options *filteropts,
+ struct parser_state **previous_syscalls,
unsigned int include_level);
int compile_filter(const char *filename, FILE *policy_file,
@@ -64,8 +68,16 @@
int flatten_block_list(struct filter_block *head, struct sock_filter *filter,
size_t index, size_t cap);
void free_block_list(struct filter_block *head);
+void free_previous_syscalls(struct parser_state **previous_syscalls);
int seccomp_can_softfail(void);
+static inline bool allow_duplicate_syscalls(void)
+{
+#if defined(ALLOW_DUPLICATE_SYSCALLS)
+ return true;
+#endif
+ return false;
+}
#ifdef __cplusplus
}; /* extern "C" */
diff --git a/syscall_filter_unittest.cc b/syscall_filter_unittest.cc
index 077b298..0dbad9d 100644
--- a/syscall_filter_unittest.cc
+++ b/syscall_filter_unittest.cc
@@ -43,6 +43,10 @@
USE_RET_LOG_LOGGING = 2,
};
+/*
+ * TODO(crbug.com/1146502) Add more tests for greater granularity for checking
+ * for duplicate syscalls.
+ */
int test_compile_filter(
std::string filename,
FILE* policy_file,
@@ -53,6 +57,7 @@
.action = action,
.allow_logging = allow_logging != NO_LOGGING,
.allow_syscalls_for_logging = allow_logging == USE_SIGSYS_LOGGING,
+ .allow_duplicate_syscalls = true,
};
return compile_filter(filename.c_str(), policy_file, prog, &filteropts);
}
@@ -70,9 +75,17 @@
.action = action,
.allow_logging = allow_logging != NO_LOGGING,
.allow_syscalls_for_logging = allow_logging == USE_SIGSYS_LOGGING,
+ .allow_duplicate_syscalls = true,
};
- return compile_file(filename.c_str(), policy_file, head, arg_blocks, labels,
- &filteropts, include_level);
+ size_t num_syscalls = get_num_syscalls();
+ struct parser_state **previous_syscalls =
+ (struct parser_state **)calloc(num_syscalls,
+ sizeof(struct parser_state *));
+ int res = compile_file(filename.c_str(), policy_file, head, arg_blocks,
+ labels, &filteropts, previous_syscalls,
+ include_level);
+ free_previous_syscalls(previous_syscalls);
+ return res;
}
struct filter_block* test_compile_policy_line(
@@ -1610,7 +1623,7 @@
index = ARCH_VALIDATION_LEN + 1;
for (i = 0; i < log_syscalls_len; i++)
EXPECT_ALLOW_SYSCALL(actual.filter + (index + 2 * i),
- lookup_syscall(log_syscalls[i]));
+ lookup_syscall(log_syscalls[i], NULL));
index += 2 * log_syscalls_len;
@@ -1656,7 +1669,7 @@
index = ARCH_VALIDATION_LEN + 1;
for (i = 0; i < log_syscalls_len; i++)
EXPECT_ALLOW_SYSCALL(actual.filter + (index + 2 * i),
- lookup_syscall(log_syscalls[i]));
+ lookup_syscall(log_syscalls[i], NULL));
index += 2 * log_syscalls_len;
diff --git a/util.c b/util.c
index 7cee515..1770d5d 100644
--- a/util.c
+++ b/util.c
@@ -149,12 +149,40 @@
dprintf(logging_config.fd, "\n");
}
-int lookup_syscall(const char *name)
+/*
+ * TODO(crbug.com/1145660): We would like for this get the length at
+ * compile-time from gen_syscalls.sh.
+ */
+size_t get_num_syscalls(void)
{
+ static size_t num_syscalls = 0;
+ if (num_syscalls) {
+ return num_syscalls;
+ }
const struct syscall_entry *entry = syscall_table;
for (; entry->name && entry->nr >= 0; ++entry)
- if (!strcmp(entry->name, name))
+ num_syscalls++;
+ return num_syscalls;
+}
+
+/*
+ * Returns the syscall nr and optionally populates the index in the pointer
+ * |ind| if it is non-NULL.
+ */
+int lookup_syscall(const char *name, size_t *ind)
+{
+ size_t ind_tmp = 0;
+ const struct syscall_entry *entry = syscall_table;
+ for (; entry->name && entry->nr >= 0; ++entry) {
+ if (!strcmp(entry->name, name)) {
+ if (ind != NULL)
+ *ind = ind_tmp;
return entry->nr;
+ }
+ ind_tmp++;
+ }
+ if (ind != NULL)
+ *ind = -1;
return -1;
}
diff --git a/util.h b/util.h
index 370ee18..bc935ff 100644
--- a/util.h
+++ b/util.h
@@ -142,7 +142,8 @@
#endif
}
-int lookup_syscall(const char *name);
+size_t get_num_syscalls(void);
+int lookup_syscall(const char *name, size_t *ind);
const char *lookup_syscall_name(int nr);
long int parse_single_constant(char *constant_str, char **endptr);
