search

dbus-fuzzer / dfuzzer

D-Bus fuzzer
GNU General Public License v3.0
38 stars 10 forks source link

rand: bump the signature limits a bit #114

Closed mrc0mmand closed 2 years ago

mrc0mmand commented 2 years ago

to see if the remote side can deal with non-standard signature lengths and signature nest levels.

Posting this as a PR to have a place for discussion.

Even though glib doesn't have a problem with generating >255 bytes long signatures, somebody along the way is not happy, since with a 256 bytes long signature I start getting The connection is closed exceptions:

# build/dfuzzer --log-dir logs -vd -n org.freedesktop.systemd1 -o /org/freedesktop/systemd1 -i org.freedesktop.systemd1.Manager -t StartTransientUnit --iteration 3
[SESSION BUS]
[PROCESS: /usr/lib/systemd/systemd]
[CONNECTED TO PID: 792]
Object: /org/freedesktop/systemd1
 Interface: org.freedesktop.systemd1.Manager
  Method: StartTransientUnit (ssa(sv)a(sa(sv))) => 3 iterations
  EXCE StartTransientUnit - D-Bus exception thrown: Job mode AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA is invalid.
   -- Signature: (ssa(sv)a(sa(sv)))
   -- Value: ('AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', @a(sv) [], @a(sa(sv)) [])
  EXCE StartTransientUnit - D-Bus exception thrown: Job mode %s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s is invalid.
   -- Signature: (ssa(sv)a(sa(sv)))
   -- Value: ('%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', '%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', [('%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', <(int64 9223372036854775807,)>), ('%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', <(signature 'dxxua(i(yudqbsosghsuugbyxthonhdisuqd)dnta{nh}gi(h)gosh)bhqhxbsisgxhsgq(xnqdaa{on})quhyyoboxdb((ahh)oigy(sqxiuho)y)da{tb}xtbu(x)u',)>)], [('%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', [('%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', <(uint64 18446744073709551615,)>), ('%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', <(int64 9223372036854775807,)>)]), ('%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', [('%s%s%s%s%s%s%s%s%s%n%s%n%n%n%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s', <(int64 9223372036854775807,)>)])])
  EXCE StartTransientUnit - D-Bus exception thrown: The connection is closed
   -- Signature: (ssa(sv)a(sa(sv)))
   -- Value: ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', '%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', [('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(0,)>)], [('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', [('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(signature 'igsoghabgqhsangagnhdiyohougo(gnoqnxa(ssiq(yba{iq}nsyoh)ahhdstnhsotsa{nab}(u))xqubbdusniyyhxhs)oydotgosggn(ushbtyt(xittaudhgutd)stxya{yx}iqxusg(qb)sudsy)otsngiyqoy(yugubsda{oat}na{t(tohognb(bt)ix(gsd)t)}ouusqn)a{xi}a{ss}ixttdhnag(tsusga{bh}nshb)xyhqa{hb}dyt',)>)]), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', [('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(objectpath '/0',)>), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(0.0,)>), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(handle 2,)>)]), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', [('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(0.0,)>), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(uint32 2147483647,)>), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(uint16 32767,)>), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(0,)>)]), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', [('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(handle 2,)>)]), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', [('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n',)>), ('%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n%n', <(int64 0,)>)])])
  PASS [M] StartTransientUnit

I tried this with both dbus-broker and dbus-daemon, both yield the same result. dbus-broker also logs a helpful message to the journal, which I guess is the culprit:

# journalctl -e -o short-monotonic --no-hostname -u dbus-broker --no-pager
[    2.767104] systemd[1]: Starting dbus-broker.service - D-Bus System Message Bus...
[    2.791217] systemd[1]: Started dbus-broker.service - D-Bus System Message Bus.
[    2.798363] dbus-broker-lau[572]: Ready
[ 1223.177915] dbus-broker[579]: Peer :1.26 is being disconnected as it sent a message with an invalid body.
[ 1238.868812] dbus-broker[579]: Peer :1.27 is being disconnected as it sent a message with an invalid body.
[ 1254.080639] dbus-broker[579]: Peer :1.28 is being disconnected as it sent a message with an invalid body.
[ 1323.704647] dbus-broker[579]: Peer :1.29 is being disconnected as it sent a message with an invalid body.
[ 1334.622910] dbus-broker[579]: Peer :1.30 is being disconnected as it sent a message with an invalid body.
[ 1351.759202] dbus-broker[579]: Peer :1.31 is being disconnected as it sent a message with an invalid body.
[ 1355.965540] dbus-broker[579]: Peer :1.32 is being disconnected as it sent a message with an invalid body.
[ 1513.914771] dbus-broker[579]: Peer :1.34 is being disconnected as it sent a message with an invalid body.

Which makes sense, given this assert https://github.com/bus1/dbus-broker/blob/701759a08f5982f515308c269a8e224fc433f4af/src/dbus/message.c#L378

After monkey-patching the patch to test the nest-levels as well I get the same result:

# Generate a signature with nest level > 64
$ perl -e 'print "(" x 65; print "ua{ui}"; print ")" x 65'
(((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((ua{ui})))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))
diff --git a/src/fuzz.h b/src/fuzz.h
index 80b89ab..921929d 100644
--- a/src/fuzz.h
+++ b/src/fuzz.h
@@ -27,10 +27,12 @@
 #define MAX_BUFFER_LENGTH 50000
 /** Maximum length of strings containing D-Bus object path */
 #define MAX_OBJECT_PATH_LENGTH 256
-/** Maximum length of D-Bus signature string */
-#define MAX_SIGNATURE_LENGTH 255
-#define MAX_SIGNATURE_NEST_LEVEL 64
 #define MAX_SUPPRESSIONS 256
+/* Set the limits fro signature length & nest level a bit higher than the limits
+ * given by the D-Bus spec (255 bytes for signature length and 64 levels for nest
+ * levels), to check if the remote side can deal with it */
+#define MAX_SIGNATURE_LENGTH (10 * 1024)
+#define MAX_SIGNATURE_NEST_LEVEL 256

 /* Basic (non-container) types which can appear in a signature
  *
diff --git a/src/rand.c b/src/rand.c
index b01261a..930772e 100644
--- a/src/rand.c
+++ b/src/rand.c
@@ -768,10 +768,15 @@ int df_rand_dbus_signature_string(gchar **buf, guint64 iteration)
         g_autoptr(GString) signature = NULL;
         guint16 size;

-        size = (iteration % MAX_SIGNATURE_LENGTH) + 1;
-        signature = g_string_sized_new(size + 1);
+        if (iteration == 0)
+                signature = g_string_new("(((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((ua{ui})))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))");
+        else {
+                size = (iteration % MAX_SIGNATURE_LENGTH) + 1;
+                signature = g_string_sized_new(size + 1);
+
+                df_generate_random_signature(signature, size, 0, /* complete= */ FALSE);
+        }

-        df_generate_random_signature(signature, size, 0, /* complete= */ FALSE);
         g_assert(g_variant_is_signature(signature->str));

         *buf = g_steal_pointer(&signature->str);
@@ -784,13 +789,17 @@ int df_rand_GVariant(GVariant **var, guint64 iteration)
         g_autoptr(GString) signature = NULL;
         guint16 size;

-        size = (iteration % MAX_SIGNATURE_LENGTH) + 1;
-        signature = g_string_sized_new(size + 3);
-
-        /* Variant must be a single complete type */
-        g_string_append_c(signature, '(');
-        df_generate_random_signature(signature, size, 0, /* complete= */ TRUE);
-        g_string_append_c(signature, ')');
+        if (iteration == 0)
+                signature = g_string_new("(((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((ua{ui})))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))");
+        else {
+                size = (iteration % MAX_SIGNATURE_LENGTH) + 1;
+                signature = g_string_sized_new(size + 3);
+
+                /* Variant must be a single complete type */
+                g_string_append_c(signature, '(');
+                df_generate_random_signature(signature, size, 0, /* complete= */ TRUE);
+                g_string_append_c(signature, ')');
+        }

         g_assert(g_variant_is_signature(signature->str) && g_variant_type_string_is_valid(signature->str));

# build/dfuzzer --log-dir logs -vd -n org.freedesktop.systemd1 -o / -i org.freedesktop.DBus.Properties -t Set --iteration 1
...
[SESSION BUS]
[PROCESS: /usr/lib/systemd/systemd]
[CONNECTED TO PID: 792]
Object: /
 Interface: org.freedesktop.DBus.Properties
  Method: Set (ssv) => 1 iterations
  EXCE Set - D-Bus exception thrown: The connection is closed
   -- Signature: (ssv)
   -- Value: ('AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', <(((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((uint32 0, @a{ui} {}),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),)>)
  PASS [M] Set
[SYSTEM BUS]
[PROCESS: /usr/lib/systemd/systemd]
[CONNECTED TO PID: 1]
Object: /
 Interface: org.freedesktop.DBus.Properties
  Method: Set (ssv) => 1 iterations
  EXCE Set - D-Bus exception thrown: The connection is closed
   -- Signature: (ssv)
   -- Value: ('AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', <(((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((uint32 0, @a{ui} {}),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),)>)
  PASS [M] Set
Exit status: 0

with familiar dbus-broker messages in journal:

[ 2579.801396] dbus-broker[2148]: Peer :1.34 is being disconnected as it sent a message with an invalid body.
[ 2579.804653] dbus-broker[579]: Peer :1.56 is being disconnected as it sent a message with an invalid body.

I have a feeling the dbus daemons don't want us going all medieval on other dbus clients :-)

evverx commented 2 years ago

I have a feeling the dbus daemons don't want us going all medieval on other dbus clients :-)

I somehow keep forgetting that dfuzzer doesn't test PID1 with its own dbus socket and all the messages go through dbus-(broker|daemon) :-) dbus-broker should be covered by https://github.com/google/oss-fuzz/pull/7870 eventually.

I think normal signatures should be good enough then. The current limits are in line with https://github.com/c-util/c-dvar/blob/main/src/c-dvar.h#L33 so they should reach dbus services in most cases on distributions shipping dbus-broker. In theory dbus-daemon should probably impose the same limits but I'm not sure.

evverx commented 2 years ago

Generally I think there should be a test or something like that that would look for those "Peer :1.34 is being disconnected" messages. If it could find them it would mean that dfuzzer can't get past dbus-broker and some functions should be adjusted to unleash it on services it's supposed to test instead of dbus-broker.

mrc0mmand commented 2 years ago

I think normal signatures should be good enough then. The current limits are in line with https://github.com/c-util/c-dvar/blob/main/src/c-dvar.h#L33 so they should reach dbus services in most cases on distributions shipping dbus-broker. In theory dbus-daemon should probably impose the same limits but I'm not sure.

Looks like it does:

https://github.com/freedesktop/dbus/blob/ef55a3db0d8f17848f8a579092fb05900cc076f5/dbus/dbus-protocol.h#L181

As for the nest levels/recursion, our code might right now generate some invalid signatures, due to this restriction:

The maximum depth of container type nesting is 32 array type codes and 32 open parentheses. This implies that the maximum total depth of recursion is 64, for an "array of array of array of ... struct of struct of struct of ..." where there are 32 array and 32 struct.

https://dbus.freedesktop.org/doc/dbus-specification.html#container-types (section Valid signatures)

Even though, theoretically, the maximum depth is 64, it's only one specific corner case. dbus-daemon handles this according to the spec:

https://github.com/freedesktop/dbus/blob/1aed6933c70eaed81ecfb079b455c6197cdfe296/dbus/dbus-protocol.h#L222-L227 https://github.com/freedesktop/dbus/blob/ef55a3db0d8f17848f8a579092fb05900cc076f5/dbus/dbus-marshal-validate.c#L51

but dbus-broker appears to be a bit more lenient (or at least the c-dvar library is, maybe dbus-broker checks are stricter):

https://github.com/c-util/c-dvar/blob/fdfe98534012309c082b94014a2074a6f62dbe9b/src/c-dvar.h#L46-L61

evverx commented 2 years ago

As for the nest levels/recursion, our code might right now generate some invalid signatures

I think it would be easier to catch that if dfuzzer didn't ignore the "The connection is closed" errors. Looking at

[CONNECTED TO PID: 1]
Object: /
 Interface: org.freedesktop.DBus.Properties
  Method: Set (ssv) => 1 iterations
  EXCE Set - D-Bus exception thrown: The connection is closed
   -- Signature: (ssv)
   -- Value: ('AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', <(((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((uint32 0, @a{ui} {}),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),)>)
  PASS [M] Set
Exit status: 0

it appears dfuzzer returns 0, which prevents issues like that from being ever discovered automatically.