systemctl: unset const char* arguments in static destructors

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.

diff --git a/src/basic/alloc-util.h b/src/basic/alloc-util.h
index 99fbd3a889ec80a33e85edf442ff1edc0297e20c..3c33308f481c9748687db42dcfb7e0eeea1bad54 100644 (file)
--- 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 810aad108a3d0ac00cce324f1364276df6ddc0e1..489e75475269985a64ff7927263f9937ac852272 100644 (file)
--- 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 096b8ada200e9cb9b47dd7114ee05d35f148076e..274b278d2d63c3ead6d395b78473091696ce092c 100644 (file)
--- 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 <unit1> <unit2> …", "sysemctl stop <unit1> <unit2> …" 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 a4100bd554bc909aed1ac0511bb0d64c13ab4714..e37569ab7f5629bcdbdf58c713412c498dc80641 100644 (file)
--- 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 ed8152e3dd8e48d5c65a1bd8be44aa98ebd63f00..8199ae9e0d3032d11c0198c98f8fd05f6c06c1a5 100644 (file)
--- 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 (file)
index 0000000..dfb14cd
Binary files /dev/null and b/test/fuzz/fuzz-systemctl-parse-argv/oss-fuzz-31714 differ