From 6b42227edb78193294d8096d39751b5fa45985e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 31 May 2021 11:12:16 +0200 Subject: [PATCH 1/4] systemctl: put static destructor in the order of variables --- src/systemctl/systemctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 16c1f39440..a4100bd554 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -112,11 +112,11 @@ bool arg_read_only = false; bool arg_mkdir = false; bool arg_marked = false; -STATIC_DESTRUCTOR_REGISTER(arg_wall, strv_freep); -STATIC_DESTRUCTOR_REGISTER(arg_root, freep); STATIC_DESTRUCTOR_REGISTER(arg_types, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_states, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_properties, strv_freep); +STATIC_DESTRUCTOR_REGISTER(arg_wall, strv_freep); +STATIC_DESTRUCTOR_REGISTER(arg_root, freep); STATIC_DESTRUCTOR_REGISTER(arg_clean_what, strv_freep); static int systemctl_help(void) { From a88f9dbae2cbd2af6016e7f4dab5b7a0771491c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 31 May 2021 11:23:20 +0200 Subject: [PATCH 2/4] systemctl: unset const char* arguments in static destructors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When fuzzing, the following happens: - we parse 'data' and produce an argv array, - one of the items in argv is assigned to arg_host, - the argv array is subsequently freed by strv_freep(), and arg_host has a dangling symlink. In normal use, argv is static, so arg_host can never become a dangling pointer. In fuzz-systemctl-parse-argv, if we repeatedly parse the same array, we have some dangling pointers while we're in the middle of parsing. If we parse the same array a second time, at the end all the dangling pointers will have been replaced again. But for a short time, if parsing one of the arguments uses another argument, we would use a dangling pointer. Such a case occurs when we have --host=… --boot-loader-entry=help. The latter calls acquire_bus() which uses arg_host. I'm not particularly happy with making the code more complicated just for fuzzing, but I think it's better to resolve this, even if the issue cannot occur in normal invocations, than to deal with fuzzer reports. Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=31714. --- src/basic/alloc-util.h | 7 +++++++ src/systemctl/systemctl-kill.c | 2 +- src/systemctl/systemctl-start-unit.c | 2 +- src/systemctl/systemctl.c | 15 ++++++++++----- src/systemctl/systemctl.h | 6 +++++- .../fuzz-systemctl-parse-argv/oss-fuzz-31714 | Bin 0 -> 132 bytes 6 files changed, 24 insertions(+), 8 deletions(-) create mode 100644 test/fuzz/fuzz-systemctl-parse-argv/oss-fuzz-31714 diff --git a/src/basic/alloc-util.h b/src/basic/alloc-util.h index 99fbd3a889..3c33308f48 100644 --- a/src/basic/alloc-util.h +++ b/src/basic/alloc-util.h @@ -79,6 +79,13 @@ void* memdup_suffix0(const void *p, size_t l); /* We can't use _alloc_() here, s memcpy_safe(_q_, p, _l_); \ }) +static inline void unsetp(void *p) { + /* A trivial "destructor" that can be used in cases where we want to + * unset a pointer from a _cleanup_ function. */ + + *(void**)p = NULL; +} + static inline void freep(void *p) { *(void**)p = mfree(*(void**) p); } diff --git a/src/systemctl/systemctl-kill.c b/src/systemctl/systemctl-kill.c index 810aad108a..489e754752 100644 --- a/src/systemctl/systemctl-kill.c +++ b/src/systemctl/systemctl-kill.c @@ -22,7 +22,7 @@ int kill_unit(int argc, char *argv[], void *userdata) { arg_kill_who = "all"; /* --fail was specified */ - if (streq(arg_job_mode, "fail")) + if (streq(arg_job_mode(), "fail")) kill_who = strjoina(arg_kill_who, "-fail"); r = expand_unit_names(bus, strv_skip(argv, 1), NULL, &names, NULL); diff --git a/src/systemctl/systemctl-start-unit.c b/src/systemctl/systemctl-start-unit.c index 096b8ada20..274b278d2d 100644 --- a/src/systemctl/systemctl-start-unit.c +++ b/src/systemctl/systemctl-start-unit.c @@ -306,7 +306,7 @@ int start_unit(int argc, char *argv[], void *userdata) { /* A command in style of "systemctl start …", "sysemctl stop …" and so on */ method = verb_to_method(argv[0]); job_type = verb_to_job_type(argv[0]); - mode = arg_job_mode; + mode = arg_job_mode(); } else method = job_type = mode = NULL; diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index a4100bd554..e37569ab7f 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -65,7 +65,7 @@ char **arg_states = NULL; char **arg_properties = NULL; bool arg_all = false; enum dependency arg_dependency = DEPENDENCY_FORWARD; -const char *arg_job_mode = "replace"; +const char *_arg_job_mode = NULL; UnitFileScope arg_scope = UNIT_FILE_SYSTEM; bool arg_wait = false; bool arg_no_block = false; @@ -115,8 +115,13 @@ bool arg_marked = false; STATIC_DESTRUCTOR_REGISTER(arg_types, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_states, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_properties, strv_freep); +STATIC_DESTRUCTOR_REGISTER(_arg_job_mode, unsetp); STATIC_DESTRUCTOR_REGISTER(arg_wall, strv_freep); +STATIC_DESTRUCTOR_REGISTER(arg_kill_who, unsetp); STATIC_DESTRUCTOR_REGISTER(arg_root, freep); +STATIC_DESTRUCTOR_REGISTER(arg_reboot_argument, unsetp); +STATIC_DESTRUCTOR_REGISTER(arg_host, unsetp); +STATIC_DESTRUCTOR_REGISTER(arg_boot_loader_entry, unsetp); STATIC_DESTRUCTOR_REGISTER(arg_clean_what, strv_freep); static int systemctl_help(void) { @@ -598,19 +603,19 @@ static int systemctl_parse_argv(int argc, char *argv[]) { break; case ARG_JOB_MODE: - arg_job_mode = optarg; + _arg_job_mode = optarg; break; case ARG_FAIL: - arg_job_mode = "fail"; + _arg_job_mode = "fail"; break; case ARG_IRREVERSIBLE: - arg_job_mode = "replace-irreversibly"; + _arg_job_mode = "replace-irreversibly"; break; case ARG_IGNORE_DEPENDENCIES: - arg_job_mode = "ignore-dependencies"; + _arg_job_mode = "ignore-dependencies"; break; case ARG_USER: diff --git a/src/systemctl/systemctl.h b/src/systemctl/systemctl.h index ed8152e3dd..8199ae9e0d 100644 --- a/src/systemctl/systemctl.h +++ b/src/systemctl/systemctl.h @@ -49,7 +49,7 @@ extern char **arg_states; extern char **arg_properties; extern bool arg_all; extern enum dependency arg_dependency; -extern const char *arg_job_mode; +extern const char *_arg_job_mode; extern UnitFileScope arg_scope; extern bool arg_wait; extern bool arg_no_block; @@ -96,4 +96,8 @@ extern bool arg_read_only; extern bool arg_mkdir; extern bool arg_marked; +static inline const char* arg_job_mode(void) { + return _arg_job_mode ?: "replace"; +} + int systemctl_dispatch_parse_argv(int argc, char *argv[]); diff --git a/test/fuzz/fuzz-systemctl-parse-argv/oss-fuzz-31714 b/test/fuzz/fuzz-systemctl-parse-argv/oss-fuzz-31714 new file mode 100644 index 0000000000000000000000000000000000000000..dfb14cd762d68516712b70256c33867cab4dbdd1 GIT binary patch literal 132 zcmZR~DA$F9VAmiAIEN6UznF=EL6?D1S2rWSxWqQ}^Mb|&AOk%7gF_fn^GX?Xb(8Y* jOLTMc6H`))bW?3JQgaFzbam4bGjm{OGcqtRl;i^d+d3ew literal 0 HcmV?d00001 From 23b8aa648d1d8cb1209ddd521989aa47f9344f68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 31 May 2021 12:11:48 +0200 Subject: [PATCH 3/4] journal-remote: downgrade messages about input data to warnings Those are unexpected, so a user-visible message seems appropriate. But they are not our errors, and to some extent we can recover from them, so "warning" seems more appropriate than "error". --- src/journal-remote/journal-remote-parse.c | 2 +- src/shared/journal-importer.c | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/journal-remote/journal-remote-parse.c b/src/journal-remote/journal-remote-parse.c index 7bc349c304..2ece329251 100644 --- a/src/journal-remote/journal-remote-parse.c +++ b/src/journal-remote/journal-remote-parse.c @@ -74,7 +74,7 @@ int process_source(RemoteSource *source, bool compress, bool seal) { &source->importer.boot_id, compress, seal); if (r == -EBADMSG) { - log_error_errno(r, "Entry is invalid, ignoring."); + log_warning_errno(r, "Entry is invalid, ignoring."); r = 0; } else if (r < 0) log_error_errno(r, "Failed to write entry of %zu bytes: %m", diff --git a/src/shared/journal-importer.c b/src/shared/journal-importer.c index 0d82acc25c..9e11dc09c1 100644 --- a/src/shared/journal-importer.c +++ b/src/shared/journal-importer.c @@ -69,9 +69,9 @@ static int get_line(JournalImporter *imp, char **line, size_t *size) { imp->scanned = imp->filled; if (imp->scanned >= DATA_SIZE_MAX) - return log_error_errno(SYNTHETIC_ERRNO(ENOBUFS), - "Entry is bigger than %u bytes.", - DATA_SIZE_MAX); + return log_warning_errno(SYNTHETIC_ERRNO(ENOBUFS), + "Entry is bigger than %u bytes.", + DATA_SIZE_MAX); if (imp->passive_fd) /* we have to wait for some data to come to us */ @@ -163,9 +163,9 @@ static int get_data_size(JournalImporter *imp) { imp->data_size = unaligned_read_le64(data); if (imp->data_size > DATA_SIZE_MAX) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Stream declares field with size %zu > DATA_SIZE_MAX = %u", - imp->data_size, DATA_SIZE_MAX); + return log_warning_errno(SYNTHETIC_ERRNO(EINVAL), + "Stream declares field with size %zu > DATA_SIZE_MAX = %u", + imp->data_size, DATA_SIZE_MAX); if (imp->data_size == 0) log_warning("Binary field with zero length"); @@ -203,8 +203,8 @@ static int get_data_newline(JournalImporter *imp) { int l; l = cescape_char(*data, buf); - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Expected newline, got '%.*s'", l, buf); + return log_warning_errno(SYNTHETIC_ERRNO(EINVAL), + "Expected newline, got '%.*s'", l, buf); } return 1; From 2dd7a72d5a58e9a99b1f79b0a2c966e5b3ee1b1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 31 May 2021 12:05:29 +0200 Subject: [PATCH 4/4] fuzz-journal-remote: print some kinds of errors In https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34803, we fail with: Assertion 'IN_SET(r, -ENOMEM, -EMFILE, -ENFILE)' failed at src/journal-remote/fuzz-journal-remote.c:69, function int LLVMFuzzerTestOneInput(const uint8_t *, size_t)(). Aborting. AddressSanitizer:DEADLYSIGNAL Let's try to print the error, so maybe we can see what is going on. With the previous commit we shouldn't print out anything. --- src/journal-remote/fuzz-journal-remote.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/journal-remote/fuzz-journal-remote.c b/src/journal-remote/fuzz-journal-remote.c index 37eff2f748..32c4e1819c 100644 --- a/src/journal-remote/fuzz-journal-remote.c +++ b/src/journal-remote/fuzz-journal-remote.c @@ -28,7 +28,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { return 0; if (!getenv("SYSTEMD_LOG_LEVEL")) - log_set_max_level(LOG_CRIT); + log_set_max_level(LOG_ERR); fdin = memfd_new_and_map("fuzz-journal-remote", size, &mem); if (fdin < 0) @@ -66,6 +66,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { r = sd_journal_open_files(&j, (const char**) STRV_MAKE(name), 0); if (r < 0) { + log_error_errno(r, "sd_journal_open_files([\"%s\"]) failed: %m", name); assert_se(IN_SET(r, -ENOMEM, -EMFILE, -ENFILE)); return r; }