36e8a3
From ed028441cc2ef0ffb9771d7266d40f18910f0ae1 Mon Sep 17 00:00:00 2001
36e8a3
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
36e8a3
Date: Wed, 5 Dec 2018 22:50:39 +0100
36e8a3
Subject: [PATCH] journald: when processing a native message, bail more quickly
36e8a3
 on overbig messages
36e8a3
36e8a3
We'd first parse all or most of the message, and only then consider if it
36e8a3
is not too large. Also, when encountering a single field over the limit,
36e8a3
we'd still process the preceding part of the message. Let's be stricter,
36e8a3
and check size limits early, and let's refuse the whole message if it fails
36e8a3
any of the size limits.
36e8a3
36e8a3
(cherry-picked from commit 964ef920ea6735d39f856b05fd8ef451a09a6a1d)
36e8a3
36e8a3
Related: #1664977
36e8a3
---
36e8a3
 src/journal/journald-native.c | 65 ++++++++++++++++++++---------------
36e8a3
 1 file changed, 37 insertions(+), 28 deletions(-)
36e8a3
36e8a3
diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c
4bff0a
index 951d092053..110ab3641c 100644
36e8a3
--- a/src/journal/journald-native.c
36e8a3
+++ b/src/journal/journald-native.c
36e8a3
@@ -109,7 +109,7 @@ static int server_process_entry(
36e8a3
         int priority = LOG_INFO;
36e8a3
         pid_t object_pid = 0;
36e8a3
         const char *p;
36e8a3
-        int r = 0;
36e8a3
+        int r = 1;
36e8a3
 
36e8a3
         p = buffer;
36e8a3
 
36e8a3
@@ -121,8 +121,7 @@ static int server_process_entry(
36e8a3
                 if (!e) {
36e8a3
                         /* Trailing noise, let's ignore it, and flush what we collected */
36e8a3
                         log_debug("Received message with trailing noise, ignoring.");
36e8a3
-                        r = 1; /* finish processing of the message */
36e8a3
-                        break;
36e8a3
+                        break; /* finish processing of the message */
36e8a3
                 }
36e8a3
 
36e8a3
                 if (e == p) {
36e8a3
@@ -132,8 +131,7 @@ static int server_process_entry(
36e8a3
                 }
36e8a3
 
36e8a3
                 if (IN_SET(*p, '.', '#')) {
36e8a3
-                        /* Ignore control commands for now, and
36e8a3
-                         * comments too. */
36e8a3
+                        /* Ignore control commands for now, and comments too. */
36e8a3
                         *remaining -= (e - p) + 1;
36e8a3
                         p = e + 1;
36e8a3
                         continue;
36e8a3
@@ -142,7 +140,6 @@ static int server_process_entry(
36e8a3
                 /* A property follows */
36e8a3
                 if (n > ENTRY_FIELD_COUNT_MAX) {
36e8a3
                         log_debug("Received an entry that has more than " STRINGIFY(ENTRY_FIELD_COUNT_MAX) " fields, ignoring entry.");
36e8a3
-                        r = 1;
36e8a3
                         goto finish;
36e8a3
                 }
36e8a3
 
36e8a3
@@ -152,7 +149,7 @@ static int server_process_entry(
36e8a3
                                     N_IOVEC_META_FIELDS + N_IOVEC_OBJECT_FIELDS +
36e8a3
                                     client_context_extra_fields_n_iovec(context))) {
36e8a3
                         r = log_oom();
36e8a3
-                        break;
36e8a3
+                        goto finish;
36e8a3
                 }
36e8a3
 
36e8a3
                 q = memchr(p, '=', e - p);
36e8a3
@@ -161,6 +158,16 @@ static int server_process_entry(
36e8a3
                                 size_t l;
36e8a3
 
36e8a3
                                 l = e - p;
36e8a3
+                                if (l > DATA_SIZE_MAX) {
36e8a3
+                                        log_debug("Received text block of %zu bytes is too large, ignoring entry.", l);
36e8a3
+                                        goto finish;
36e8a3
+                                }
36e8a3
+
36e8a3
+                                if (entry_size + l + n + 1 > ENTRY_SIZE_MAX) { /* data + separators + trailer */
36e8a3
+                                        log_debug("Entry is too big (%zu bytes after processing %zu entries), ignoring entry.",
36e8a3
+                                                  entry_size + l, n + 1);
36e8a3
+                                        goto finish;
36e8a3
+                                }
36e8a3
 
36e8a3
                                 /* If the field name starts with an underscore, skip the variable, since that indicates
36e8a3
                                  * a trusted field */
36e8a3
@@ -178,7 +185,7 @@ static int server_process_entry(
36e8a3
                         p = e + 1;
36e8a3
                         continue;
36e8a3
                 } else {
36e8a3
-                        uint64_t l;
36e8a3
+                        uint64_t l, total;
36e8a3
                         char *k;
36e8a3
 
36e8a3
                         if (*remaining < e - p + 1 + sizeof(uint64_t) + 1) {
36e8a3
@@ -187,10 +194,16 @@ static int server_process_entry(
36e8a3
                         }
36e8a3
 
36e8a3
                         l = unaligned_read_le64(e + 1);
36e8a3
-
36e8a3
                         if (l > DATA_SIZE_MAX) {
36e8a3
-                                log_debug("Received binary data block of %"PRIu64" bytes is too large, ignoring.", l);
36e8a3
-                                break;
36e8a3
+                                log_debug("Received binary data block of %"PRIu64" bytes is too large, ignoring entry.", l);
36e8a3
+                                goto finish;
36e8a3
+                        }
36e8a3
+
36e8a3
+                        total = (e - p) + 1 + l;
36e8a3
+                        if (entry_size + total + n + 1 > ENTRY_SIZE_MAX) { /* data + separators + trailer */
36e8a3
+                                log_debug("Entry is too big (%"PRIu64"bytes after processing %zu fields), ignoring.",
36e8a3
+                                          entry_size + total, n + 1);
36e8a3
+                                goto finish;
36e8a3
                         }
36e8a3
 
36e8a3
                         if ((uint64_t) *remaining < e - p + 1 + sizeof(uint64_t) + l + 1 ||
36e8a3
@@ -199,7 +212,7 @@ static int server_process_entry(
36e8a3
                                 break;
36e8a3
                         }
36e8a3
 
36e8a3
-                        k = malloc((e - p) + 1 + l);
36e8a3
+                        k = malloc(total);
36e8a3
                         if (!k) {
36e8a3
                                 log_oom();
36e8a3
                                 break;
36e8a3
@@ -228,15 +241,8 @@ static int server_process_entry(
36e8a3
                 }
36e8a3
         }
36e8a3
 
36e8a3
-        if (n <= 0) {
36e8a3
-                r = 1;
36e8a3
+        if (n <= 0)
36e8a3
                 goto finish;
36e8a3
-        }
36e8a3
-
36e8a3
-        if (!client_context_test_priority(context, priority)) {
36e8a3
-                r = 0;
36e8a3
-                goto finish;
36e8a3
-        }
36e8a3
 
36e8a3
         tn = n++;
36e8a3
         iovec[tn] = IOVEC_MAKE_STRING("_TRANSPORT=journal");
36e8a3
@@ -247,6 +253,11 @@ static int server_process_entry(
36e8a3
                 goto finish;
36e8a3
         }
36e8a3
 
36e8a3
+        r = 0; /* Success, we read the message. */
36e8a3
+
36e8a3
+        if (!client_context_test_priority(context, priority))
36e8a3
+                goto finish;
36e8a3
+
36e8a3
         if (message) {
36e8a3
                 if (s->forward_to_syslog)
36e8a3
                         server_forward_syslog(s, syslog_fixup_facility(priority), identifier, message, ucred, tv);
36e8a3
@@ -318,15 +329,13 @@ void server_process_native_file(
36e8a3
         bool sealed;
36e8a3
         int r;
36e8a3
 
36e8a3
-        /* Data is in the passed fd, since it didn't fit in a
36e8a3
-         * datagram. */
36e8a3
+        /* Data is in the passed fd, probably it didn't fit in a datagram. */
36e8a3
 
36e8a3
         assert(s);
36e8a3
         assert(fd >= 0);
36e8a3
 
36e8a3
         /* If it's a memfd, check if it is sealed. If so, we can just
36e8a3
-         * use map it and use it, and do not need to copy the data
36e8a3
-         * out. */
36e8a3
+         * mmap it and use it, and do not need to copy the data out. */
36e8a3
         sealed = memfd_get_sealed(fd) > 0;
36e8a3
 
36e8a3
         if (!sealed && (!ucred || ucred->uid != 0)) {
36e8a3
@@ -397,7 +406,7 @@ void server_process_native_file(
36e8a3
                 ssize_t n;
36e8a3
 
36e8a3
                 if (fstatvfs(fd, &vfs) < 0) {
36e8a3
-                        log_error_errno(errno, "Failed to stat file system of passed file, ignoring: %m");
36e8a3
+                        log_error_errno(errno, "Failed to stat file system of passed file, not processing it: %m");
36e8a3
                         return;
36e8a3
                 }
36e8a3
 
36e8a3
@@ -407,7 +416,7 @@ void server_process_native_file(
36e8a3
                  * https://github.com/systemd/systemd/issues/1822
36e8a3
                  */
36e8a3
                 if (vfs.f_flag & ST_MANDLOCK) {
36e8a3
-                        log_error("Received file descriptor from file system with mandatory locking enabled, refusing.");
36e8a3
+                        log_error("Received file descriptor from file system with mandatory locking enabled, not processing it.");
36e8a3
                         return;
36e8a3
                 }
36e8a3
 
36e8a3
@@ -420,13 +429,13 @@ void server_process_native_file(
36e8a3
                  * and so is SMB. */
36e8a3
                 r = fd_nonblock(fd, true);
36e8a3
                 if (r < 0) {
36e8a3
-                        log_error_errno(r, "Failed to make fd non-blocking, ignoring: %m");
36e8a3
+                        log_error_errno(r, "Failed to make fd non-blocking, not processing it: %m");
36e8a3
                         return;
36e8a3
                 }
36e8a3
 
36e8a3
                 /* The file is not sealed, we can't map the file here, since
36e8a3
                  * clients might then truncate it and trigger a SIGBUS for
36e8a3
-                 * us. So let's stupidly read it */
36e8a3
+                 * us. So let's stupidly read it. */
36e8a3
 
36e8a3
                 p = malloc(st.st_size);
36e8a3
                 if (!p) {