From acc6ba1cb7293a0581b1d82fba27b7990b5a96aa Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 16 Feb 2022 12:59:24 -0800 Subject: [PATCH] libapparmor: fix handling of failed symlink traversal Ideally we would have a flag or something so the caller could choose to handle symlinks, or traverse them. But since all callers currently don't handle symlinks just handle them in the iterator. Beyond fixing the early termination due to a failed symlink this also fixes another case of failure in one job cause dir based loads to terminate early. Which can result in partial loads. Fixes: https://gitlab.com/apparmor/apparmor/-/issues/215 MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/850 Signed-off-by: John Johansen Approved-by: Georgia Garcia --- libraries/libapparmor/src/features.c | 2 ++ libraries/libapparmor/src/policy_cache.c | 2 ++ libraries/libapparmor/src/private.c | 31 ++++++++++++++++++++---- parser/parser_lex.l | 2 ++ parser/parser_main.c | 6 ++++- parser/tst/Makefile | 7 ++++-- 6 files changed, 42 insertions(+), 8 deletions(-) diff --git a/libraries/libapparmor/src/features.c b/libraries/libapparmor/src/features.c index 3f875beec..2b16dcf58 100644 --- a/libraries/libapparmor/src/features.c +++ b/libraries/libapparmor/src/features.c @@ -194,6 +194,8 @@ static int features_dir_cb(int dirfd, const char *name, struct stat *st, if (features_snprintf(fst, "%s {", name) == -1) return -1; + /* Handle symlink here. See _aa_dirat_for_each in private.c */ + if (S_ISREG(st->st_mode)) { ssize_t len; size_t remaining; diff --git a/libraries/libapparmor/src/policy_cache.c b/libraries/libapparmor/src/policy_cache.c index 1f3478746..7e840dc66 100644 --- a/libraries/libapparmor/src/policy_cache.c +++ b/libraries/libapparmor/src/policy_cache.c @@ -45,6 +45,8 @@ struct aa_policy_cache { static int clear_cache_cb(int dirfd, const char *path, struct stat *st, void *data unused) { + /* Handle symlink here. See _aa_dirat_for_each in private.c */ + if (S_ISREG(st->st_mode)) { /* remove regular files */ return unlinkat(dirfd, path, 0); diff --git a/libraries/libapparmor/src/private.c b/libraries/libapparmor/src/private.c index 3d23b1648..26b34c8d1 100644 --- a/libraries/libapparmor/src/private.c +++ b/libraries/libapparmor/src/private.c @@ -452,7 +452,8 @@ int _aa_overlaydirat_for_each(int dirfd[], int n, void *data, * * The cb function is called with the DIR in use and the name of the * file in that directory. If the file is to be opened it should - * use the openat, fstatat, and related fns. + * use the openat, fstatat, and related fns. If the file is a symlink + * _aa_dirat_for_each currently tries to traverse it for the caller * * Returns: 0 on success, else -1 and errno is set to the error code */ @@ -485,14 +486,34 @@ int _aa_dirat_for_each(int dirfd, const char *name, void *data, autofree struct dirent *dir = namelist[i]; struct stat my_stat; - if (rc) - continue; - - if (fstatat(cb_dirfd, dir->d_name, &my_stat, 0)) { + if (fstatat(cb_dirfd, dir->d_name, &my_stat, AT_SYMLINK_NOFOLLOW)) { PDEBUG("stat failed for '%s': %m\n", dir->d_name); rc = -1; continue; } + /* currently none of the callers handle symlinks, and this + * same basic code was applied to each. So for this patch + * just drop it here. + * + * Going forward we need to start handling symlinks as + * they have meaning. + * In the case of + * cache: they act as a place holder for files that have been + * combined into a single binary. This enables the + * file based cache lookup time find that relation + * and dedup, so multiple loads aren't done. + * profiles: just a profile in an alternate location, but + * should do dedup detection when doing dir reads + * so we don't double process. + */ + if (S_ISLNK(my_stat.st_mode)) { + /* just traverse the symlink */ + if (fstatat(cb_dirfd, dir->d_name, &my_stat, 0)) { + PDEBUG("symlink target stat failed for '%s': %m\n", dir->d_name); + rc = -1; + continue; + } + } if (cb(cb_dirfd, dir->d_name, &my_stat, data)) { PDEBUG("dir_for_each callback failed for '%s'\n", diff --git a/parser/parser_lex.l b/parser/parser_lex.l index 8aded956a..7d99cc0f2 100644 --- a/parser/parser_lex.l +++ b/parser/parser_lex.l @@ -141,6 +141,8 @@ static int include_dir_cb(int dirfd unused, const char *name, struct stat *st, return 0; } + /* Handle symlink here. See _aa_dirat_for_each in private.c */ + if (S_ISREG(st->st_mode)) { if (!(yyin = fopen(path,"r"))) yyerror(_("Could not open '%s' in '%s'"), path, d->filename); diff --git a/parser/parser_main.c b/parser/parser_main.c index 126df2485..5a6baf1cb 100644 --- a/parser/parser_main.c +++ b/parser/parser_main.c @@ -1450,6 +1450,8 @@ static int profile_dir_cb(int dirfd unused, const char *name, struct stat *st, { int rc = 0; + /* Handle symlink here. See _aa_dirat_for_each in private.c */ + if (!S_ISDIR(st->st_mode) && !is_blacklisted(name, NULL)) { struct dir_cb_data *cb_data = (struct dir_cb_data *)data; autofree char *path = NULL; @@ -1472,6 +1474,8 @@ static int binary_dir_cb(int dirfd unused, const char *name, struct stat *st, { int rc = 0; + /* Handle symlink here. See _aa_dirat_for_each in private.c */ + if (!S_ISDIR(st->st_mode) && !is_blacklisted(name, NULL)) { struct dir_cb_data *cb_data = (struct dir_cb_data *)data; autofree char *path = NULL; @@ -1664,7 +1668,7 @@ int main(int argc, char *argv[]) if ((retval = dirat_for_each(AT_FDCWD, profilename, &cb_data, cb))) { last_error = errno; - PDEBUG("Failed loading profiles from %s\n", + PERROR("There was an error while loading profiles from %s\n", profilename); if (abort_on_error) break; diff --git a/parser/tst/Makefile b/parser/tst/Makefile index a05838c0c..08d42f459 100644 --- a/parser/tst/Makefile +++ b/parser/tst/Makefile @@ -17,8 +17,8 @@ endif all: tests -.PHONY: tests error_output gen_dbus gen_xtrans parser_sanity caching minimize equality valgrind -tests: error_output caching minimize equality parser_sanity +.PHONY: tests error_output gen_dbus gen_xtrans parser_sanity caching minimize equality dirtest valgrind +tests: error_output caching minimize equality dirtest parser_sanity GEN_TRANS_DIRS=simple_tests/generated_x/ simple_tests/generated_perms_leading/ simple_tests/generated_perms_safe/ simple_tests/generated_dbus @@ -46,6 +46,9 @@ minimize: $(PARSER) equality: $(PARSER) LANG=C APPARMOR_PARSER="$(PARSER) $(PARSER_ARGS)" ./equality.sh +dirtest: $(PARSER) + LANG=C APPARMOR_PARSER="$(PARSER) $(PARSER_ARGS)" ./dirtest.sh + valgrind: $(PARSER) gen_xtrans gen_dbus LANG=C ./valgrind_simple.py -p "$(PARSER) $(PARSER_ARGS)" -v simple_tests