A fuzzer for journal-remote #9014

keszybz · 2018-05-17T10:02:36Z

No description provided.

evverx

fuzz-journal-remote seems to be failing under msan as soon as it starts:

$ sudo infra/helper.py run_fuzzer systemd fuzz-journal-remote
Running: docker run --rm -i --privileged -e FUZZING_ENGINE=libfuzzer -v /home/vagrant/oss-fuzz/build/out/systemd:/out -t gcr.io/oss-fuzz-base/base-runner run_fuzzer fuzz-journal-remote
Using seed corpus: fuzz-journal-remote_seed_corpus.zip
/out/fuzz-journal-remote -rss_limit_mb=2048 -timeout=25 /tmp/fuzz-journal-remote_corpus -max_len=65536 < /dev/null
INFO: Seed: 3380449479
INFO: Loaded 2 modules   (36336 inline 8-bit counters): 36139 [0x7ff36ea31d39, 0x7ff36ea3aa64), 197 [0x9998c8, 0x99998d),
INFO: Loaded 2 PC tables (36336 PCs): 36139 [0x7ff36ea3aa68,0x7ff36eac7d18), 197 [0x999990,0x99a5e0),
INFO:        2 files found in /tmp/fuzz-journal-remote_corpus
INFO: seed corpus: files: 2 min: 4657b max: 7790b total: 12447b rss: 97Mb
Uninitialized bytes in __interceptor_pwrite64 at offset 24 inside [0x7fffdd4d7230, 240)
==15==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7ff36e685e8a in journal_file_init_header /work/build/../../src/systemd/src/journal/journal-file.c:436:13
    #1 0x7ff36e683a9d in journal_file_open /work/build/../../src/systemd/src/journal/journal-file.c:3333:21
    #2 0x7ff36e68b8f6 in journal_file_open_reliably /work/build/../../src/systemd/src/journal/journal-file.c:3520:13
    #3 0x4a3f35 in open_output /work/build/../../src/systemd/src/journal-remote/journal-remote.c:70:13
    #4 0x4a34d0 in journal_remote_get_writer /work/build/../../src/systemd/src/journal-remote/journal-remote.c:136:21
    #5 0x4a550f in get_source_for_fd /work/build/../../src/systemd/src/journal-remote/journal-remote.c:183:13
    #6 0x4a46bd in journal_remote_add_source /work/build/../../src/systemd/src/journal-remote/journal-remote.c:235:13
    #7 0x4a271c in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:36:9
    #8 0x4f27cc in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:524:13
    #9 0x4efa0b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:448:3
    #10 0x4f8e96 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:732:7
    #11 0x4f9f73 in fuzzer::Fuzzer::Loop(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:752:3
    #12 0x4bf329 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:756:6
    #13 0x4ac391 in main /src/libfuzzer/FuzzerMain.cpp:20:10
    #14 0x7ff36d14982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #15 0x41f9d8 in _start (/out/fuzz-journal-remote+0x41f9d8)

  Uninitialized value was stored to memory at
    #0 0x7ff36e61cd41 in sd_id128_randomize /work/build/../../src/systemd/src/libsystemd/sd-id128/sd-id128.c:288:16
    #1 0x7ff36e685cec in journal_file_init_header /work/build/../../src/systemd/src/journal/journal-file.c:426:13
    #2 0x7ff36e683a9d in journal_file_open /work/build/../../src/systemd/src/journal/journal-file.c:3333:21
    #3 0x7ff36e68b8f6 in journal_file_open_reliably /work/build/../../src/systemd/src/journal/journal-file.c:3520:13
    #4 0x4a3f35 in open_output /work/build/../../src/systemd/src/journal-remote/journal-remote.c:70:13
    #5 0x4a34d0 in journal_remote_get_writer /work/build/../../src/systemd/src/journal-remote/journal-remote.c:136:21
    #6 0x4a550f in get_source_for_fd /work/build/../../src/systemd/src/journal-remote/journal-remote.c:183:13
    #7 0x4a46bd in journal_remote_add_source /work/build/../../src/systemd/src/journal-remote/journal-remote.c:235:13
    #8 0x4a271c in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:36:9
    #9 0x4f27cc in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:524:13
    #10 0x4efa0b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:448:3
    #11 0x4f8e96 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:732:7
    #12 0x4f9f73 in fuzzer::Fuzzer::Loop(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:752:3
    #13 0x4bf329 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:756:6
    #14 0x4ac391 in main /src/libfuzzer/FuzzerMain.cpp:20:10
    #15 0x7ff36d14982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

  Uninitialized value was created by an allocation of 't' in the stack frame of function 'sd_id128_randomize'
    #0 0x7ff36e61cb00 in sd_id128_randomize /work/build/../../src/systemd/src/libsystemd/sd-id128/sd-id128.c:274

SUMMARY: MemorySanitizer: use-of-uninitialized-value /work/build/../../src/systemd/src/journal/journal-file.c:436:13 in journal_file_init_header
Exiting
MS: 0 ; base unit: 0000000000000000000000000000000000000000
artifact_prefix='./'; Test unit written to ./crash-847911777b3096783f4ee70a69ab6d28380c810b
[vagrant@localhost oss-fuzz]$ sudo infra/helper.py check_build --sanitizer=memory systemd
Running: docker run --rm -i --privileged -e FUZZING_ENGINE=libfuzzer -e SANITIZER=memory -v /home/vagrant/oss-fuzz/build/out/systemd:/out -t gcr.io/oss-fuzz-base/base-runner test_all
INFO: performing bad build checks for /out/fuzz-dhcp-server.
INFO: performing bad build checks for /out/fuzz-journal-remote.
INFO: performing bad build checks for /out/fuzz-unit-file.
INFO: performing bad build checks for /out/fuzz-dns-packet.
4 fuzzers total, 0 seem to be broken (0%).
Check build passed.

It's a false positive which is most likely caused by google/sanitizers#852. I think it could be got around by avoiding getrandom when the code is compiled with msan:

diff --git a/src/basic/random-util.c b/src/basic/random-util.c
index 0750083b8..03117c225 100644
--- a/src/basic/random-util.c
+++ b/src/basic/random-util.c
@@ -46,7 +46,7 @@ int acquire_random_bytes(void *p, size_t n, bool high_quality_required) {
          * for us. */

         /* Use the getrandom() syscall unless we know we don't have it. */
-        if (have_syscall != 0) {
+        if (have_syscall != 0 && !HAS_FEATURE_MEMORY_SANITIZER) {
                 r = getrandom(p, n, GRND_NONBLOCK);
                 if (r > 0) {
                         have_syscall = true;

keszybz · 2018-05-20T21:09:43Z

@evverx I pushed your patch and some fixes for an issue in the json output code found by the fuzzer.

The issue I pointed out has been fixed.

evverx · 2018-05-20T22:37:40Z

src/fuzz/fuzz-journal-remote.c

+                assert_se(r >= 0);
+
+                r = sd_journal_seek_head(j);
+                assert_se(r >= 0);


In http://llvm.org/docs/LibFuzzer.html#fuzz-target It is recommended that logging should be avoided. I'm wondering if it's possible to make show_journal skip calling fprintf if it's run in the "fuzzer" mode.

Half of this fuzzer is the journal output code, and writing the stuff out is an integral part of it. Disabling output would be possible, but it would reduce the coverage a lot.

I was thinking about the coverage too, but currently can neither confirm nor deny anything because of google/oss-fuzz#1426. Should something go wrong, it probably won't be hard to fix, so leaving everything as is makes sense to me. Thank you.

In the meantime, I reconsidered, and pushed another patch on top to write the logs to /dev/null. I think all our output code ignored write failures, so this should have the same effect and writing to stdout. I tested that the one failure that was found is still reproducible with /dev/null.

Unfortunately there seems to be some leak of fds. I need to work on this a bit more.

evverx · 2018-05-21T13:48:28Z

src/journal/journal-file.c

@@ -451,7 +451,10 @@ static int journal_file_refresh_header(JournalFile *f) {
        assert(f->header);

        r = sd_id128_get_machine(&f->header->machine_id);
-        if (r < 0)
+        if (IN_SET(r, -ENOENT, -ENOMEDIUM))


The docker image that oss-fuzz uses has an empty /etc/machine-id.

I'm not sure it's a docker issue. An empty machine-id seems to come from the base ubuntu image and it's totally fine according to https://www.freedesktop.org/software/systemd/man/machine-id.html#Initialization:

For operating system images which are created once and used on multiple machines, for example for containers or in the cloud, /etc/machine-id should be an empty file in the generic file system image.

Given that all oss-fuzz images are based on Ubuntu, where systemd is used by default, I would expect machine-id to be initialized. systemd itself is never run there and therefore it has no chance to do what it's supposed to do, so I think the right place would be https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-image/Dockerfile, though, I may be wrong.

@inferno-chromium, @Dor1s sorry to bother you, but would it be possible to run systemd-machine-id-setup or something like echo 760cd9963f3a45d8858f9daae65426cf >/etc/machine-id (to make the environment reproducible) while building base-image?

You're right, it not the responsibility of docker itself to initialize the id.

But no matter who should initialize it, I still think that not requiring it to be present in journal code is the right thing.

@evverx, that sounds fine to me. CC @oliverchang should we add it to all base images?

should it be a random value every time?

@Dor1s although using the same id would make the environment more predictable, it seems to be artificial. I think generating a new id with systemd-machine-id-setup would reflect reality better.

But no matter who should initialize it, I still think that not requiring it to be present in journal code is the right thing.

I vaguely remember there were discussions about journald and machine-id where @poettering said that it's expected that journald fails if something is wrong with machine-id, but I haven't been able to find them yet.

I think I'm more concerned about the commit where the return value of sd_id128_get_machine has been changed. It's a public interface and there might be software out there depending on it. Although EINVAL doesn't seem to be the best choice, it's the only way to distinguish an empty machine-id, which is normal, from an id consisting of only zeros, which should never happen.

I think it comes down to what the program trying to use /etc/machine-id is supposed to do. It doesn't really matter if the file is missing, empty, or all zeros, the handling should be the same. -EINVAL means a programming error, so we should provide codes for those other conditions that can be distinguished from it.

so, i figure we should sooner or later support a machine-id-less mode altogether btw, in the interest of "fully anonymous" systems... ChromeOS has a concept like that, and I figure it makes some sense for ultra-privacy-focussed systems.

(I used to be a staunch supporter of requiring machine ID strictly, but I changed my mind on this).

Hence yes, I think @keszybz makes a ton of sense

poettering · 2018-05-23T19:29:45Z

src/fuzz/fuzz-journal-remote.c

+        assert_se(r >= 0);
+
+        sd_journal_close(j);
+        unlink(name);


why not use that new cleanup function you added for this?

Indeed, I added it when working on this commit. But then this commit evolved to use mkostemps (because the journal code requires the .journal suffix), which is not supported by my cleanup function.

poettering · 2018-05-23T19:34:00Z

src/journal/journal-file.c

@@ -451,7 +451,10 @@ static int journal_file_refresh_header(JournalFile *f) {
        assert(f->header);

        r = sd_id128_get_machine(&f->header->machine_id);
-        if (r < 0)
+        if (IN_SET(r, -ENOENT, -ENOMEDIUM))


so, i figure we should sooner or later support a machine-id-less mode altogether btw, in the interest of "fully anonymous" systems... ChromeOS has a concept like that, and I figure it makes some sense for ultra-privacy-focussed systems.

(I used to be a staunch supporter of requiring machine ID strictly, but I changed my mind on this).

Hence yes, I think @keszybz makes a ton of sense

poettering · 2018-05-23T19:44:03Z

src/basic/string-util.c

+        size_t i;
+        const char *t = s;
+
+        assert(len > 4 + 4 + 1); /* two chars and the terminator */


>= is good enough, no?

poettering · 2018-05-23T19:47:11Z

src/basic/journal-importer.c

@@ -347,6 +348,16 @@ int journal_importer_process_data(JournalImporter *imp) {
                        /* chomp newline */
                        n--;

+                        if (!journal_field_valid(line, sep - line, true)) {
+                                char buf[64], *t;


so far we never did such arbitrarily sized arrays, but instead used some macro or so. Can we do that here too? i.e. add FIELD_ELLIPSIZE_LIMIT or so?

It is completely arbitrary unfortunately. But yeah, I should add some define for it.

not that this matters too much, but this line still has no comment btw... The one at the top of process_special_field() does however... Given that this already appears twice here I wonder if adding a (local) macro for this might not be worth it after all...

poettering · 2018-05-23T19:48:02Z

src/fuzz/fuzz-journal-remote.c

@@ -47,8 +48,10 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
        r = sd_journal_open_files(&j, (const char**) STRV_MAKE(name), 0);
        assert_se(r >= 0);

+        assert_se(dev_null = fopen("/dev/null", "w"));


"we"? (out of principle, not for any specific reason beyond that)

poettering · 2018-05-23T20:00:59Z

src/basic/time-util.c

        if (t > USEC_TIMESTAMP_FORMATTABLE_MAX)
-                return NULL;
+                return "--- ✗✗✗✗-✗✗-✗✗ ✗✗:✗✗:✗✗";


hmm, so unconditional unicode (i.e. not bound to is_unicode_locale()) is probably something we should avoid...

Maybe just use regular ASCII "9"s here after all, the unicode stuff doesn't get us much I figure... i.e. --- XXXX-XX-XX XX:XX:XX

I am not too fond of returning the static string here directly, some code might expect the buffer passed in to be filled in, and quite frankly for a good reason. I am pretty sure we should fill in the buffer unconditionally, and return that and leave the return value as non-const ptr...

Also, maybe it would be better to make this bvehaviour opt-out, i.e. let's replace the two existing bool params by a new flags param, and then add a new flag: "FORMAT_TIMESTAMP_FAIL_NON_DISPLAYABLE" or so, which would get the old behaviour back of returning NULL on failure. (And yeah, format_timestamp_internal() should probably be made public then under a new name, maybe format_timestamp_full() or so. And maybe we can then drop some of the less used flavours of the wrappers and just make the callers use format_timestamp_full() directly

keszybz · 2018-05-24T11:40:42Z

Updated. All issues were addressed, modulo the comments below.

so far we never did such arbitrarily sized arrays, but instead used some macro or so. Can we do that here too? i.e. add FIELD_ELLIPSIZE_LIMIT or so?

In the end I just added a comment. This is only used in one place, so a define would move the definition away from use without additional benefit. If this shows up in other places, we can add the define then.

I fixed a memleak (precisely speaking, the leak of an mmap of the memfd), and squashed two commits that were fixes to previous commits in the series with those commits, and fixed some more issues in the display code which were found in the meantime (the last two commits are new).

Also, maybe it would be better to make this bvehaviour opt-out, i.e. let's replace the two existing bool params by a new flags param, and then add a new flag: "FORMAT_TIMESTAMP_FAIL_NON_DISPLAYABLE"

... Pfff, maybe, dunno. I would rather not do this in this PR.

poettering · 2018-05-24T14:51:24Z

src/basic/time-util.c

-        if (t > USEC_TIMESTAMP_FORMATTABLE_MAX)
-                return NULL;
+        if (t > USEC_TIMESTAMP_FORMATTABLE_MAX) {
+                assert(l >= strlen("--- XXXX-XX-XX XX:XX:XX"));


STRLEN() rather than strlen() please.

also this is off by one, no space for the trailing NUL is included

Off-by-one fixed.

STRLEN() rather than strlen() please

The compilers (gcc and clang both) are generally able to optimize strlen away to a constant, even at -O0. The reason we have STRLEN is that they still created a VLA despite optimizing the size to a constant. So we should use STRLEN in variable declarations, but otherwise plain strlen is completely fine.

keszybz · 2018-05-27T17:16:05Z

Pushed again, with a rebase (needed because of with-unit that was merged recently), and with 3 commits for #9090.

yuwata · 2018-05-29T05:54:12Z

Ugh, conflicts again. Please rebase this.

keszybz · 2018-05-29T09:08:37Z

Rebased.

poettering

btw, did you see my >= comment earlier?

poettering · 2018-05-30T10:08:18Z

src/basic/string-util.h

@@ -157,6 +157,8 @@ bool string_has_cc(const char *p, const char *ok) _pure_;

 char *ellipsize_mem(const char *s, size_t old_length_bytes, size_t new_length_columns, unsigned percent);
 char *ellipsize(const char *s, size_t length, unsigned percent);
+char *cellescape_impl(char *buf, size_t len, const char *s);
+#define cellescape(buf, s) cellescape_impl(buf, ELEMENTSOF(buf), s)


uh, sorry for not noticing this eralier, but this one is too magic I think. I mean, it might be completely OK to allocate some buffer with new() or so and then pass it to cellescape, and this would fail horribly in that case... I'd avoid such magic.

i still really don't like this bit tbh

it might be completely OK to allocate some buffer with new() or so and then pass it to cellescape

This will fail at compilation time, because ELEMENTSOF is smart enough to catch this. But OK, I'll rework this to use a define for the length and remove the macro.

poettering · 2018-05-30T10:14:08Z

src/shared/logs-show.c

                         * the header, hence let's suppress it here */
-                        if (length >= 9 &&
-                            memcmp(data, "_BOOT_ID=", 9) == 0)
+                        if (length >= 9 && memcmp(data, "_BOOT_ID=", 9) == 0)


might be time to add some macro for this actually, "memory_startswith()" or so, or maybe "startswith_n()"?

static inline void *memory_startswith(const void *p, size_t sz, const char *token) { size_t n; n = strlen(token); if (sz < n) return NULL; if (memcmp(p, token, n) == 0) return (uint8_t*) p + n; return NULL; }

or so? (but it's definitely material for a later PR)

(I am working on adding a helper for this now, btw, for an independent PR)

i posted that pr as #9131 btw

This is in preparation to reusing the RemoteServer in other concepts. I tried to keep changes to minimum: - arg_* global variables are now passed as state in RemoteServer - exported functions get the "journal_remote_" prefix - some variables are renamed In particular, there is an ugly global RemoveServer* variable. It was originally added because µhttpd did not allow state to be passed to the callbacks. I'm not sure if this has been remediated in µhttpd, but either way, this is not changed here, the global variable is only renamed for clarity.

Also remove "b''" from the generated MESSAGE= field.

keszybz · 2018-05-31T11:22:52Z

Rebased on top of #9131. I squashed the last two commits into one.

poettering · 2018-05-31T11:51:42Z

src/basic/journal-importer.c

@@ -347,6 +348,16 @@ int journal_importer_process_data(JournalImporter *imp) {
                        /* chomp newline */
                        n--;

+                        if (!journal_field_valid(line, sep - line, true)) {
+                                char buf[64], *t;


not that this matters too much, but this line still has no comment btw... The one at the top of process_special_field() does however... Given that this already appears twice here I wonder if adding a (local) macro for this might not be worth it after all...

poettering · 2018-05-31T11:52:27Z

src/basic/string-util.h

@@ -157,6 +157,8 @@ bool string_has_cc(const char *p, const char *ok) _pure_;

 char *ellipsize_mem(const char *s, size_t old_length_bytes, size_t new_length_columns, unsigned percent);
 char *ellipsize(const char *s, size_t length, unsigned percent);
+char *cellescape_impl(char *buf, size_t len, const char *s);
+#define cellescape(buf, s) cellescape_impl(buf, ELEMENTSOF(buf), s)


i still really don't like this bit tbh

poettering · 2018-05-31T11:55:05Z

PR looks fine, but the ELEMENTSOF in the macro definition of cellescape() is something i really don't like, it welcomes bugs...

poettering · 2018-05-31T11:55:27Z

(btw, did you see my last review?)

… fields It's not supposed to be the most efficient, but instead fast and simple to use. I kept the logic in ellipsize_mem() to use unicode ellipsis even in non-unicode locales. I'm not quite convinced things should be this way, especially that with this patch it'd actually be simpler to always use "…" in unicode locale and "..." otherwise, but Lennart wanted it this way for some reason.

We shouldn't just log arbitrary stuff, in particular newlines and control chars Now: Unknown dunder line __CURSORFACILITY=6\nSYSLOG_IDENTIFIER=/USR/SBIN/CRON\nMES…, ignoring. Unknown dunder line __REALTIME_TIME[TAMP=1404101101501874\n__MONOTONIC_TIMEST…, ignoring.

`fuzz-journal-remote` seems to be failing under `msan` as soon as it starts: $ sudo infra/helper.py run_fuzzer systemd fuzz-journal-remote Running: docker run --rm -i --privileged -e FUZZING_ENGINE=libfuzzer -v /home/vagrant/oss-fuzz/build/out/systemd:/out -t gcr.io/oss-fuzz-base/base-runner run_fuzzer fuzz-journal-remote Using seed corpus: fuzz-journal-remote_seed_corpus.zip /out/fuzz-journal-remote -rss_limit_mb=2048 -timeout=25 /tmp/fuzz-journal-remote_corpus -max_len=65536 < /dev/null INFO: Seed: 3380449479 INFO: Loaded 2 modules (36336 inline 8-bit counters): 36139 [0x7ff36ea31d39, 0x7ff36ea3aa64), 197 [0x9998c8, 0x99998d), INFO: Loaded 2 PC tables (36336 PCs): 36139 [0x7ff36ea3aa68,0x7ff36eac7d18), 197 [0x999990,0x99a5e0), INFO: 2 files found in /tmp/fuzz-journal-remote_corpus INFO: seed corpus: files: 2 min: 4657b max: 7790b total: 12447b rss: 97Mb Uninitialized bytes in __interceptor_pwrite64 at offset 24 inside [0x7fffdd4d7230, 240) ==15==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x7ff36e685e8a in journal_file_init_header /work/build/../../src/systemd/src/journal/journal-file.c:436:13 #1 0x7ff36e683a9d in journal_file_open /work/build/../../src/systemd/src/journal/journal-file.c:3333:21 #2 0x7ff36e68b8f6 in journal_file_open_reliably /work/build/../../src/systemd/src/journal/journal-file.c:3520:13 #3 0x4a3f35 in open_output /work/build/../../src/systemd/src/journal-remote/journal-remote.c:70:13 #4 0x4a34d0 in journal_remote_get_writer /work/build/../../src/systemd/src/journal-remote/journal-remote.c:136:21 #5 0x4a550f in get_source_for_fd /work/build/../../src/systemd/src/journal-remote/journal-remote.c:183:13 #6 0x4a46bd in journal_remote_add_source /work/build/../../src/systemd/src/journal-remote/journal-remote.c:235:13 #7 0x4a271c in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:36:9 #8 0x4f27cc in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:524:13 #9 0x4efa0b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:448:3 #10 0x4f8e96 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:732:7 #11 0x4f9f73 in fuzzer::Fuzzer::Loop(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:752:3 #12 0x4bf329 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:756:6 #13 0x4ac391 in main /src/libfuzzer/FuzzerMain.cpp:20:10 #14 0x7ff36d14982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) #15 0x41f9d8 in _start (/out/fuzz-journal-remote+0x41f9d8) Uninitialized value was stored to memory at #0 0x7ff36e61cd41 in sd_id128_randomize /work/build/../../src/systemd/src/libsystemd/sd-id128/sd-id128.c:288:16 #1 0x7ff36e685cec in journal_file_init_header /work/build/../../src/systemd/src/journal/journal-file.c:426:13 #2 0x7ff36e683a9d in journal_file_open /work/build/../../src/systemd/src/journal/journal-file.c:3333:21 #3 0x7ff36e68b8f6 in journal_file_open_reliably /work/build/../../src/systemd/src/journal/journal-file.c:3520:13 #4 0x4a3f35 in open_output /work/build/../../src/systemd/src/journal-remote/journal-remote.c:70:13 #5 0x4a34d0 in journal_remote_get_writer /work/build/../../src/systemd/src/journal-remote/journal-remote.c:136:21 #6 0x4a550f in get_source_for_fd /work/build/../../src/systemd/src/journal-remote/journal-remote.c:183:13 #7 0x4a46bd in journal_remote_add_source /work/build/../../src/systemd/src/journal-remote/journal-remote.c:235:13 #8 0x4a271c in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-journal-remote.c:36:9 #9 0x4f27cc in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:524:13 #10 0x4efa0b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:448:3 #11 0x4f8e96 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:732:7 #12 0x4f9f73 in fuzzer::Fuzzer::Loop(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:752:3 #13 0x4bf329 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:756:6 #14 0x4ac391 in main /src/libfuzzer/FuzzerMain.cpp:20:10 #15 0x7ff36d14982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) Uninitialized value was created by an allocation of 't' in the stack frame of function 'sd_id128_randomize' #0 0x7ff36e61cb00 in sd_id128_randomize /work/build/../../src/systemd/src/libsystemd/sd-id128/sd-id128.c:274 SUMMARY: MemorySanitizer: use-of-uninitialized-value /work/build/../../src/systemd/src/journal/journal-file.c:436:13 in journal_file_init_header Exiting MS: 0 ; base unit: 0000000000000000000000000000000000000000 artifact_prefix='./'; Test unit written to ./crash-847911777b3096783f4ee70a69ab6d28380c810b [vagrant@localhost oss-fuzz]$ sudo infra/helper.py check_build --sanitizer=memory systemd Running: docker run --rm -i --privileged -e FUZZING_ENGINE=libfuzzer -e SANITIZER=memory -v /home/vagrant/oss-fuzz/build/out/systemd:/out -t gcr.io/oss-fuzz-base/base-runner test_all INFO: performing bad build checks for /out/fuzz-dhcp-server. INFO: performing bad build checks for /out/fuzz-journal-remote. INFO: performing bad build checks for /out/fuzz-unit-file. INFO: performing bad build checks for /out/fuzz-dns-packet. 4 fuzzers total, 0 seem to be broken (0%). Check build passed. It's a false positive which is most likely caused by google/sanitizers#852. I think it could be got around by avoiding `getrandom` when the code is compiled with `msan`

…nd string operations We'd look for a '=' separator using memchr, i.e. ignoring any nul bytes in the string, but then do a strndup, which would terminate on any nul byte, and then again do a memcmp, which would access memory past the chunk allocated by strndup. Of course, we probably shouldn't allow keys with nul bytes in them. But we currently do, so there might be journal files like that out there. So let's fix the journal-reading code first.

…ject $ build-asan/fuzz-journal-remote test/fuzz-regressions/fuzz-journal-remote/crash-96dee870ea66d03e89ac321eee28ea63a9b9aa45 ... Ignoring invalid field: "S\020" Ignoring invalid field: "S\020" ... If the field name includes nul bytes, we won't print all of the name. But that seems enough of a corner case to ignore.

…ported The parser never accepted "__"-prefixed fields in binary format, but there was a comment questioning this decision. Let's make it official, and remove the comment. Also, for clarity, let's move the dunder field parsing after the field verification check. This doesn't change much, because invalid fields cannot be known special fields, but is seems cleaner to first verify the validity of the name, and then check if it is one of the known ones.

This makes the fuzzing much more efficient. Optionally provide output is $SYSTEMD_FUZZ_OUTPUT is set, which makes debugging of any failures much easier. The case from 056129d is still detected properly.

If the timestamp is above 9999-12-30, (or 2038-something-something on 32 bit), use XXXX-XX-XX XX:XX:XX as the replacement. The problem with refusing to print timestamps is that our code accepts such timestamps, so we can't really just refuse to process them afterwards. Also, it makes journal files non-portable, because suddently we might completely refuse to print entries which are totally OK on a different machine.

Makes the intent a bit clearer.

The journal verification functions would reject such an entry. It would probably still display fine (because we prefer _SOURCE_REALTIME_TIMESTAMP= if present), but it seems wrong to create an entry that would not pass verification.

…ESTAMP entry journalctl -o short would display those entries, but journalctl -o short-full would refuse. If the entry is bad, just fall back to the receive-side realtime timestamp like we would if it was completely missing.

In this commit, this is done only in testing code, i.e. there is no functional change apart from tests.

…ng entries The boot id is stored twice, and different code paths use either one or the other. So we need to store it both in the header and as a field for full compatibility.

Also remove the comma from the comment everywhere, I think the comma unnecessarilly put emphasis on the clause after the comma. Fixes systemd#9090. Reproducer: systemd-journal-remote --split-mode=none -o /tmp/msg6.journal --trust=all --listen-http=8080 systemd-journal-upload -u http://localhost:8080 journalctl --file /tmp/msg6.journal -o verbose -n1

keszybz · 2018-05-31T12:34:51Z

Updated with the requested changes.

…Fuzz project The containers come with an empty machine-id, which causes the fuzzer to fail as soon as it starts. See systemd#9014 (comment)

keszybz added journal tests journal-remote labels May 17, 2018

keszybz force-pushed the fuzz-journal-remote branch from a8df78b to b70d302 Compare May 17, 2018 11:23

evverx previously requested changes May 20, 2018

View reviewed changes

evverx reviewed May 20, 2018

View reviewed changes

keszybz force-pushed the fuzz-journal-remote branch from d56e03d to 1ac4c7d Compare May 21, 2018 12:54

keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label May 21, 2018

evverx reviewed May 21, 2018

View reviewed changes

keszybz removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label May 21, 2018

poettering requested changes May 23, 2018

View reviewed changes

keszybz force-pushed the fuzz-journal-remote branch from ae7779a to 948372b Compare May 24, 2018 06:48

keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 24, 2018

keszybz force-pushed the fuzz-journal-remote branch from 948372b to 0a16cae Compare May 24, 2018 11:35

poettering reviewed May 24, 2018

View reviewed changes

keszybz force-pushed the fuzz-journal-remote branch 2 times, most recently from 31ee78f to f1d36d2 Compare May 27, 2018 17:15

keszybz force-pushed the fuzz-journal-remote branch from f1d36d2 to e2ca5f5 Compare May 29, 2018 09:07

poettering reviewed May 30, 2018

View reviewed changes

keszybz added 4 commits May 31, 2018 13:04

journal: rewrap function args

5d889c1

journal-remote: export handle_raw_source()

864876e

log-generator: make message size configurable, add short options

757ed4f

Also remove "b''" from the generated MESSAGE= field.

keszybz force-pushed the fuzz-journal-remote branch from e2ca5f5 to 33f6ca2 Compare May 31, 2018 11:17

poettering requested changes May 31, 2018

View reviewed changes

keszybz and others added 17 commits May 31, 2018 14:27

fuzz-journal-remote: try all output modes

bbdad08

shared/logs-show: use _cleanup_

9be391d

fuzz-journal-remote: write to /dev/null not stdout

6dbef30

This makes the fuzzing much more efficient. Optionally provide output is $SYSTEMD_FUZZ_OUTPUT is set, which makes debugging of any failures much easier. The case from 056129d is still detected properly.

Use const char* for timestamp strings which we don't plan to modify

4d9685b

Makes the intent a bit clearer.

journal: refuse an entry with invalid timestamp fields

c627395

The journal verification functions would reject such an entry. It would probably still display fine (because we prefer _SOURCE_REALTIME_TIMESTAMP= if present), but it seems wrong to create an entry that would not pass verification.

journal: remove unused args from journal_file_copy_entry()

5a271b0

journal: allow boot_id to be passed to journal_append_entry()

d180c34

In this commit, this is done only in testing code, i.e. there is no functional change apart from tests.

journal-remote: parse the _BOOT_ID field and use the value when writi…

c0b6ada

…ng entries The boot id is stored twice, and different code paths use either one or the other. So we need to store it both in the header and as a field for full compatibility.

keszybz force-pushed the fuzz-journal-remote branch from 33f6ca2 to 0ab896b Compare May 31, 2018 12:34

poettering merged commit 89544ae into systemd:master May 31, 2018

keszybz deleted the fuzz-journal-remote branch June 2, 2018 09:30

RomanSaveljev mentioned this pull request Jun 3, 2018

journalctl does not properly interleave logs from multiple machines #8979

Open

evverx added the fuzzing Implementation of fuzzers and fixes for stuff found through fuzzing label Nov 23, 2018

hg-zt mentioned this pull request May 27, 2021

systemd-journal-remote overwrite all _BOOT_ID with current _BOOT_ID when importing journals from other machines #19744

Open

A fuzzer for journal-remote #9014

A fuzzer for journal-remote #9014

Conversation

keszybz commented May 17, 2018

evverx left a comment

Choose a reason for hiding this comment

keszybz commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering May 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keszybz commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keszybz commented May 27, 2018

yuwata commented May 29, 2018

keszybz commented May 29, 2018

poettering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keszybz commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented May 31, 2018

poettering commented May 31, 2018

keszybz commented May 31, 2018

poettering May 23, 2018 •

edited