systemd-run can crash pid 1

[jsynacek]

Since this requires privileges to be set up correctly in order to be run, I don't really consider it a DoS.

Using the following command crashes pid 1:

# systemd-run /usr/bin/echo привет

That's because a failed assertion gets triggered in strv_join_quoted in src/basic/strv.c:419. That function doesn't really make much sense to me, including the assertion, so I leave this to someone more knowledgeable to fix correctly. The following patch might help you with testing this:

diff --git a/src/test/test-strv.c b/src/test/test-strv.c
index aec00eb81..cf409fd3b 100644
--- a/src/test/test-strv.c
+++ b/src/test/test-strv.c
@@ -726,7 +726,18 @@ static void test_strv_fnmatch(void) {
         assert_se(strv_fnmatch(v, "\\", FNM_NOESCAPE));
 }
 
+static void test_utf8(void) {
+        _cleanup_strv_free_ char **v = NULL;
+        _cleanup_free_ char *s = NULL;
+
+        v = strv_new("/usr/bin/echo ", "привет", NULL);
+        assert_se(v);
+        s = strv_join_quoted(v);
+        assert_se(s);
+}
+
 int main(int argc, char *argv[]) {
+        test_utf8();
         test_specifier_printf();
         test_str_in_set();
         test_strptr_in_set();

I've tried fixing this by removing the assertion and making the algorithm allocate more space, but even then, when trying to streq the result with what I expected, it didn't work.

As a side note, there are probably more assumptions throughout the code base that simply assume that a character is one byte long, which don't hold.

[evverx]

Interestingly, I can crash v234, but v237 seems to be fine. Could you elaborate on what version of systemd you are using?

[jsynacek]

I forgot to mention that, sorry. I'm using the current master (ce691f31aafe4492011aa94600453ce69662b910), but the commit has been there for years. I can reproduce this the same way on Fedora 27, Rawhide and RHEL 7.

Note, though, that the strings have to be of certain length. If, for example, instead of saying "привет", I say "привет сыстемд", the problem doesn't reproduce, since the assertion holds again. At least that was my case.

[evverx]

That's true that the commit where strv_join_quoted was introduced has been there for years, but it seems that the function has not been widely used since 2e59b24 was merged. Maybe it should be removed altogether.

[yuwata]

I've created #8057. Please take a look.

@evverx strv_join_quoted() is now used (only) on tests of strv_split_extract(). And strv_split_extract() is used several places. What do you think about moving strv_join_quoted() to test-strv.c, not dropping it completely?

[jsynacek]

I must have confused the versions... The systemd-run ... reproducer is not reproducible on current master. Sorry about the confusion again.