From 090a20cfaf3d5439fa39c5d8df473b0cfef181dd Mon Sep 17 00:00:00 2001 From: Evgeny Vereshchagin Date: Fri, 16 Nov 2018 21:23:56 +0100 Subject: [PATCH 1/3] tests: add a fuzzer for process_audit_string --- src/fuzz/fuzz-journald-audit.c | 27 +++++++++++++++++++++++++++ src/fuzz/meson.build | 5 +++++ src/journal/journald-audit.c | 2 +- src/journal/journald-audit.h | 2 ++ test/fuzz/fuzz-journald-audit/basic | 1 + 5 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 src/fuzz/fuzz-journald-audit.c create mode 100644 test/fuzz/fuzz-journald-audit/basic diff --git a/src/fuzz/fuzz-journald-audit.c b/src/fuzz/fuzz-journald-audit.c new file mode 100644 index 00000000000..fe401c0d98f --- /dev/null +++ b/src/fuzz/fuzz-journald-audit.c @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ + +#include "fuzz.h" +#include "journald-audit.h" + +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { + Server s; + _cleanup_free_ char *buffer = NULL; + + s = (Server) { + .syslog_fd = -1, + .native_fd = -1, + .stdout_fd = -1, + .dev_kmsg_fd = -1, + .audit_fd = -1, + .hostname_fd = -1, + .notify_fd = -1, + .storage = STORAGE_NONE, + }; + assert_se(sd_event_default(&s.event) >= 0); + buffer = memdup_suffix0(data, size); + assert_se(buffer); + process_audit_string(&s, 0, buffer, size); + server_done(&s); + + return 0; +} diff --git a/src/fuzz/meson.build b/src/fuzz/meson.build index 5fd3093f077..0cda081f761 100644 --- a/src/fuzz/meson.build +++ b/src/fuzz/meson.build @@ -51,6 +51,11 @@ fuzzers += [ libshared], [libmount]], + [['src/fuzz/fuzz-journald-audit.c'], + [libjournal_core, + libshared], + [libselinux]], + [['src/fuzz/fuzz-journald-kmsg.c'], [libjournal_core, libshared], diff --git a/src/journal/journald-audit.c b/src/journal/journald-audit.c index f591e53d78b..94484f5d01c 100644 --- a/src/journal/journald-audit.c +++ b/src/journal/journald-audit.c @@ -313,7 +313,7 @@ static int map_all_fields( } } -static void process_audit_string(Server *s, int type, const char *data, size_t size) { +void process_audit_string(Server *s, int type, const char *data, size_t size) { size_t n_iov_allocated = 0, n_iov = 0, z; _cleanup_free_ struct iovec *iov = NULL; uint64_t seconds, msec, id; diff --git a/src/journal/journald-audit.h b/src/journal/journald-audit.h index 57bb1711c96..7766618c986 100644 --- a/src/journal/journald-audit.h +++ b/src/journal/journald-audit.h @@ -6,4 +6,6 @@ void server_process_audit_message(Server *s, const void *buffer, size_t buffer_size, const struct ucred *ucred, const union sockaddr_union *sa, socklen_t salen); +void process_audit_string(Server *s, int type, const char *data, size_t size); + int server_open_audit(Server*s); diff --git a/test/fuzz/fuzz-journald-audit/basic b/test/fuzz/fuzz-journald-audit/basic new file mode 100644 index 00000000000..d1ce8cc5f0c --- /dev/null +++ b/test/fuzz/fuzz-journald-audit/basic @@ -0,0 +1 @@ +audit(1542398162.211:744): pid=7376 uid=1000 auid=1000 ses=6 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='op=PAM:accounting grantors=pam_unix,pam_localuser acct="vagrant" exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/1 res=success' \ No newline at end of file From 1dab14aba749b9c5ab8176c5730107b70834240b Mon Sep 17 00:00:00 2001 From: Evgeny Vereshchagin Date: Fri, 16 Nov 2018 23:32:31 +0100 Subject: [PATCH 2/3] journald: check whether sscanf has changed the value corresponding to %n It's possible for sscanf to receive strings containing all three fields and not matching the template at the same time. When this happens the value of k doesn't change, which basically means that process_audit_string tries to access memory randomly. Sometimes it works and sometimes it doesn't :-) See also https://bugzilla.redhat.com/show_bug.cgi?id=1059314. --- src/journal/journald-audit.c | 3 ++- test/fuzz/fuzz-journald-audit/crash | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) create mode 100644 test/fuzz/fuzz-journald-audit/crash diff --git a/src/journal/journald-audit.c b/src/journal/journald-audit.c index 94484f5d01c..345e43ef44f 100644 --- a/src/journal/journald-audit.c +++ b/src/journal/journald-audit.c @@ -341,11 +341,12 @@ void process_audit_string(Server *s, int type, const char *data, size_t size) { if (!p) return; + k = 0; if (sscanf(p, "(%" PRIu64 ".%" PRIu64 ":%" PRIu64 "):%n", &seconds, &msec, &id, - &k) != 3) + &k) != 3 || k == 0) return; p += k; diff --git a/test/fuzz/fuzz-journald-audit/crash b/test/fuzz/fuzz-journald-audit/crash new file mode 100644 index 00000000000..91bd85ca6ed --- /dev/null +++ b/test/fuzz/fuzz-journald-audit/crash @@ -0,0 +1 @@ +audit(1542398162.211:744) pid=7376 uid=1000 auid=1000 ses=6 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='op=PAM:accounting grantors=pam_unix,pam_localuser acct="vagrant" exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/1 res=success' From ed62712dc6fb236845c489a7f386c7aff0ec31d6 Mon Sep 17 00:00:00 2001 From: Evgeny Vereshchagin Date: Sat, 17 Nov 2018 13:01:09 +0100 Subject: [PATCH 3/3] tests: introduce dummy_server_init and use it in all journald fuzzers --- src/fuzz/fuzz-journald-audit.c | 18 +++--------------- src/fuzz/fuzz-journald-kmsg.c | 20 ++++---------------- src/fuzz/fuzz-journald.c | 26 +++++++++++++++++++------- src/fuzz/fuzz-journald.h | 2 ++ src/fuzz/meson.build | 6 ++++-- 5 files changed, 32 insertions(+), 40 deletions(-) diff --git a/src/fuzz/fuzz-journald-audit.c b/src/fuzz/fuzz-journald-audit.c index fe401c0d98f..3f3ce7e8eed 100644 --- a/src/fuzz/fuzz-journald-audit.c +++ b/src/fuzz/fuzz-journald-audit.c @@ -1,26 +1,14 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include "fuzz.h" +#include "fuzz-journald.h" #include "journald-audit.h" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { Server s; - _cleanup_free_ char *buffer = NULL; - s = (Server) { - .syslog_fd = -1, - .native_fd = -1, - .stdout_fd = -1, - .dev_kmsg_fd = -1, - .audit_fd = -1, - .hostname_fd = -1, - .notify_fd = -1, - .storage = STORAGE_NONE, - }; - assert_se(sd_event_default(&s.event) >= 0); - buffer = memdup_suffix0(data, size); - assert_se(buffer); - process_audit_string(&s, 0, buffer, size); + dummy_server_init(&s, data, size); + process_audit_string(&s, 0, s.buffer, size); server_done(&s); return 0; diff --git a/src/fuzz/fuzz-journald-kmsg.c b/src/fuzz/fuzz-journald-kmsg.c index e2611c6d453..f7426c84002 100644 --- a/src/fuzz/fuzz-journald-kmsg.c +++ b/src/fuzz/fuzz-journald-kmsg.c @@ -1,29 +1,17 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include "fuzz.h" +#include "fuzz-journald.h" #include "journald-kmsg.h" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { - Server s = {}; - _cleanup_free_ char *buffer = NULL; + Server s; if (size == 0) return 0; - s = (Server) { - .syslog_fd = -1, - .native_fd = -1, - .stdout_fd = -1, - .dev_kmsg_fd = -1, - .audit_fd = -1, - .hostname_fd = -1, - .notify_fd = -1, - .storage = STORAGE_NONE, - }; - assert_se(sd_event_default(&s.event) >= 0); - buffer = memdup(data, size); - assert_se(buffer); - dev_kmsg_record(&s, buffer, size); + dummy_server_init(&s, data, size); + dev_kmsg_record(&s, s.buffer, size); server_done(&s); return 0; diff --git a/src/fuzz/fuzz-journald.c b/src/fuzz/fuzz-journald.c index f271d7f2fe0..0659b92ba3c 100644 --- a/src/fuzz/fuzz-journald.c +++ b/src/fuzz/fuzz-journald.c @@ -5,12 +5,29 @@ #include "journald-server.h" #include "sd-event.h" +void dummy_server_init(Server *s, const uint8_t *buffer, size_t size) { + *s = (Server) { + .syslog_fd = -1, + .native_fd = -1, + .stdout_fd = -1, + .dev_kmsg_fd = -1, + .audit_fd = -1, + .hostname_fd = -1, + .notify_fd = -1, + .storage = STORAGE_NONE, + }; + assert_se(sd_event_default(&s->event) >= 0); + s->buffer = memdup_suffix0(buffer, size); + assert_se(s->buffer); + s->buffer_size = size + 1; +} + void fuzz_journald_processing_function( const uint8_t *data, size_t size, void (*f)(Server *s, const char *buf, size_t raw_len, const struct ucred *ucred, const struct timeval *tv, const char *label, size_t label_len) ) { - Server s = {}; + Server s; char *label = NULL; size_t label_len = 0; struct ucred *ucred = NULL; @@ -19,12 +36,7 @@ void fuzz_journald_processing_function( if (size == 0) return; - assert_se(sd_event_default(&s.event) >= 0); - s.syslog_fd = s.native_fd = s.stdout_fd = s.dev_kmsg_fd = s.audit_fd = s.hostname_fd = s.notify_fd = -1; - s.buffer = memdup_suffix0(data, size); - assert_se(s.buffer); - s.buffer_size = size + 1; - s.storage = STORAGE_NONE; + dummy_server_init(&s, data, size); (*f)(&s, s.buffer, size, ucred, tv, label, label_len); server_done(&s); } diff --git a/src/fuzz/fuzz-journald.h b/src/fuzz/fuzz-journald.h index e9d32a74aaf..77e3b0c064c 100644 --- a/src/fuzz/fuzz-journald.h +++ b/src/fuzz/fuzz-journald.h @@ -3,6 +3,8 @@ #include "journald-server.h" +void dummy_server_init(Server *s, const uint8_t *buffer, size_t size); + void fuzz_journald_processing_function( const uint8_t *data, size_t size, diff --git a/src/fuzz/meson.build b/src/fuzz/meson.build index 0cda081f761..89f312fee7c 100644 --- a/src/fuzz/meson.build +++ b/src/fuzz/meson.build @@ -51,12 +51,14 @@ fuzzers += [ libshared], [libmount]], - [['src/fuzz/fuzz-journald-audit.c'], + [['src/fuzz/fuzz-journald-audit.c', + 'src/fuzz/fuzz-journald.c'], [libjournal_core, libshared], [libselinux]], - [['src/fuzz/fuzz-journald-kmsg.c'], + [['src/fuzz/fuzz-journald-kmsg.c', + 'src/fuzz/fuzz-journald.c'], [libjournal_core, libshared], [libselinux]],