From 26de3af817b0c5746cb61b798ae8e138e01ea17c Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>

Date: Mon, 9 Jul 2018 07:03:01 +0200

Subject: [PATCH] Introduce free_and_strndup and use it in bus-message.c

v2: fix error in free_and_strndup()

When the orignal and copied message were the same, but shorter than specified

length l, memory read past the end of the buffer would be performed. A test

case is included: a string that had an embedded NUL ("q\0") is used to replace

"q".

v3: Fix one more bug in free_and_strndup and add tests.

v4: Some style fixed based on review, one more use of free_and_replace, and

make the tests more comprehensive.

(cherry picked from commit 7f546026abbdc56c453a577e52d57159458c3e9c)

Resolves: #1635428

---

 src/basic/string-util.c                       |  28 +++++++-

 src/basic/string-util.h                       |   1 +

 src/libsystemd/sd-bus/bus-message.c           |  34 ++++------

 src/test/test-string-util.c                   |  62 ++++++++++++++++++

 ...h-b88ad9ecf4aacf4a0caca5b5543953265367f084 | Bin 0 -> 32 bytes

 5 files changed, 103 insertions(+), 22 deletions(-)

 create mode 100644 test/fuzz/fuzz-bus-message/crash-b88ad9ecf4aacf4a0caca5b5543953265367f084

diff --git a/src/basic/string-util.c b/src/basic/string-util.c

index 0a40683493..dfa739996f 100644

--- a/src/basic/string-util.c

+++ b/src/basic/string-util.c

@@ -1004,7 +1004,7 @@ int free_and_strdup(char **p, const char *s) {

         assert(p);

-        /* Replaces a string pointer with an strdup()ed new string,

+        /* Replaces a string pointer with a strdup()ed new string,

          * possibly freeing the old one. */

         if (streq_ptr(*p, s))

@@ -1023,6 +1023,32 @@ int free_and_strdup(char **p, const char *s) {

         return 1;

 }

+int free_and_strndup(char **p, const char *s, size_t l) {

+        char *t;

+

+        assert(p);

+        assert(s || l == 0);

+

+        /* Replaces a string pointer with a strndup()ed new string,

+         * freeing the old one. */

+

+        if (!*p && !s)

+                return 0;

+

+        if (*p && s && strneq(*p, s, l) && (l > strlen(*p) || (*p)[l] == '\0'))

+                return 0;

+

+        if (s) {

+                t = strndup(s, l);

+                if (!t)

+                        return -ENOMEM;

+        } else

+                t = NULL;

+

+        free_and_replace(*p, t);

+        return 1;

+}

+

 #if !HAVE_EXPLICIT_BZERO

 /*

  * Pointer to memset is volatile so that compiler must de-reference

diff --git a/src/basic/string-util.h b/src/basic/string-util.h

index c0cc4e78d7..96a9260f93 100644

--- a/src/basic/string-util.h

+++ b/src/basic/string-util.h

@@ -176,6 +176,7 @@ char *strrep(const char *s, unsigned n);

 int split_pair(const char *s, const char *sep, char **l, char **r);

 int free_and_strdup(char **p, const char *s);

+int free_and_strndup(char **p, const char *s, size_t l);

 /* Normal memmem() requires haystack to be nonnull, which is annoying for zero-length buffers */

 static inline void *memmem_safe(const void *haystack, size_t haystacklen, const void *needle, size_t needlelen) {

diff --git a/src/libsystemd/sd-bus/bus-message.c b/src/libsystemd/sd-bus/bus-message.c

index 381034f5f8..7c8bad2bdd 100644

--- a/src/libsystemd/sd-bus/bus-message.c

+++ b/src/libsystemd/sd-bus/bus-message.c

@@ -4175,20 +4175,19 @@ _public_ int sd_bus_message_peek_type(sd_bus_message *m, char *type, const char

                 if (contents) {

                         size_t l;

-                        char *sig;

                         r = signature_element_length(c->signature+c->index+1, &l);

                         if (r < 0)

                                 return r;

-                        assert(l >= 1);

+                        /* signature_element_length does verification internally */

-                        sig = strndup(c->signature + c->index + 1, l);

-                        if (!sig)

+                        assert(l >= 1);

+                        if (free_and_strndup(&c->peeked_signature,

+                                             c->signature + c->index + 1, l) < 0)

                                 return -ENOMEM;

-                        free(c->peeked_signature);

-                        *contents = c->peeked_signature = sig;

+                        *contents = c->peeked_signature;

                 }

                 if (type)

@@ -4201,19 +4200,17 @@ _public_ int sd_bus_message_peek_type(sd_bus_message *m, char *type, const char

                 if (contents) {

                         size_t l;

-                        char *sig;

                         r = signature_element_length(c->signature+c->index, &l);

                         if (r < 0)

                                 return r;

                         assert(l >= 2);

-                        sig = strndup(c->signature + c->index + 1, l - 2);

-                        if (!sig)

+                        if (free_and_strndup(&c->peeked_signature,

+                                             c->signature + c->index + 1, l - 2) < 0)

                                 return -ENOMEM;

-                        free(c->peeked_signature);

-                        *contents = c->peeked_signature = sig;

+                        *contents = c->peeked_signature;

                 }

                 if (type)

@@ -4253,9 +4250,8 @@ _public_ int sd_bus_message_peek_type(sd_bus_message *m, char *type, const char

                                 if (k > c->item_size)

                                         return -EBADMSG;

-                                free(c->peeked_signature);

-                                c->peeked_signature = strndup((char*) q + 1, k - 1);

-                                if (!c->peeked_signature)

+                                if (free_and_strndup(&c->peeked_signature,

+                                                     (char*) q + 1, k - 1) < 0)

                                         return -ENOMEM;

                                 if (!signature_is_valid(c->peeked_signature, true))

@@ -5085,25 +5081,21 @@ int bus_message_parse_fields(sd_bus_message *m) {

                         if (*p == 0) {

                                 size_t l;

-                                char *c;

                                 /* We found the beginning of the signature

                                  * string, yay! We require the body to be a

                                  * structure, so verify it and then strip the

                                  * opening/closing brackets. */

-                                l = ((char*) m->footer + m->footer_accessible) - p - (1 + sz);

+                                l = (char*) m->footer + m->footer_accessible - p - (1 + sz);

                                 if (l < 2 ||

                                     p[1] != SD_BUS_TYPE_STRUCT_BEGIN ||

                                     p[1 + l - 1] != SD_BUS_TYPE_STRUCT_END)

                                         return -EBADMSG;

-                                c = strndup(p + 1 + 1, l - 2);

-                                if (!c)

+                                if (free_and_strndup(&m->root_container.signature,

+                                                     p + 1 + 1, l - 2) < 0)

                                         return -ENOMEM;

-

-                                free(m->root_container.signature);

-                                m->root_container.signature = c;

                                 break;

                         }

diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c

index 3e72ce2c0a..43a6b14c34 100644

--- a/src/test/test-string-util.c

+++ b/src/test/test-string-util.c

@@ -5,6 +5,7 @@

 #include "macro.h"

 #include "string-util.h"

 #include "strv.h"

+#include "tests.h"

 #include "utf8.h"

 static void test_string_erase(void) {

@@ -30,6 +31,64 @@ static void test_string_erase(void) {

         assert_se(x[9] == '\0');

 }

+static void test_free_and_strndup_one(char **t, const char *src, size_t l, const char *expected, bool change) {

+        int r;

+

+        log_debug("%s: \"%s\", \"%s\", %zd (expect \"%s\", %s)",

+                  __func__, strnull(*t), strnull(src), l, strnull(expected), yes_no(change));

+

+        r = free_and_strndup(t, src, l);

+        assert_se(streq_ptr(*t, expected));

+        assert_se(r == change); /* check that change occurs only when necessary */

+}

+

+static void test_free_and_strndup(void) {

+        static const struct test_case {

+                const char *src;

+                size_t len;

+                const char *expected;

+        } cases[] = {

+                     {"abc", 0, ""},

+                     {"abc", 0, ""},

+                     {"abc", 1, "a"},

+                     {"abc", 2, "ab"},

+                     {"abc", 3, "abc"},

+                     {"abc", 4, "abc"},

+                     {"abc", 5, "abc"},

+                     {"abc", 5, "abc"},

+                     {"abc", 4, "abc"},

+                     {"abc", 3, "abc"},

+                     {"abc", 2, "ab"},

+                     {"abc", 1, "a"},

+                     {"abc", 0, ""},

+

+                     {"", 0, ""},

+                     {"", 1, ""},

+                     {"", 2, ""},

+                     {"", 0, ""},

+                     {"", 1, ""},

+                     {"", 2, ""},

+                     {"", 2, ""},

+                     {"", 1, ""},

+                     {"", 0, ""},

+

+                     {NULL, 0, NULL},

+

+                     {"foo", 3, "foo"},

+                     {"foobar", 6, "foobar"},

+        };

+

+        _cleanup_free_ char *t = NULL;

+        const char *prev_expected = t;

+

+        for (unsigned i = 0; i < ELEMENTSOF(cases); i++) {

+                test_free_and_strndup_one(&t,

+                                          cases[i].src, cases[i].len, cases[i].expected,

+                                          !streq_ptr(cases[i].expected, prev_expected));

+                prev_expected = t;

+        }

+}

+

 static void test_ascii_strcasecmp_n(void) {

         assert_se(ascii_strcasecmp_n("", "", 0) == 0);

@@ -497,7 +556,10 @@ static void test_memory_startswith(void) {

 }

 int main(int argc, char *argv[]) {

+        test_setup_logging(LOG_DEBUG);

+

         test_string_erase();

+        test_free_and_strndup();

         test_ascii_strcasecmp_n();

         test_ascii_strcasecmp_nn();

         test_cellescape();

diff --git a/test/fuzz/fuzz-bus-message/crash-b88ad9ecf4aacf4a0caca5b5543953265367f084 b/test/fuzz/fuzz-bus-message/crash-b88ad9ecf4aacf4a0caca5b5543953265367f084

new file mode 100644

index 0000000000000000000000000000000000000000..52469650b5498a45d5d95bd9d933c989cfb47ca7

GIT binary patch

literal 32

ccmd1#|DTBg0(2Mzp)7_%AVVXuuuM|`09r!?!~g&Q

literal 0

HcmV?d00001