journald: when processing a native message, bail more quickly on overbig messages

We'd first parse all or most of the message, and only then consider if it
is not too large. Also, when encountering a single field over the limit,
we'd still process the preceding part of the message. Let's be stricter,
and check size limits early, and let's refuse the whole message if it fails
any of the size limits.

1 files changed, 37 insertions, 28 deletions

diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c
index 951d09205..110ab3641 100644
--- a/src/journal/journald-native.c
+++ b/src/journal/journald-native.c
@@ -109,7 +109,7 @@ static int server_process_entry(
         int priority = LOG_INFO;
         pid_t object_pid = 0;
         const char *p;
-        int r = 0;
+        int r = 1;
 
         p = buffer;
 
@@ -121,8 +121,7 @@ static int server_process_entry(
                 if (!e) {
                         /* Trailing noise, let's ignore it, and flush what we collected */
                         log_debug("Received message with trailing noise, ignoring.");
-                        r = 1; /* finish processing of the message */
-                        break;
+                        break; /* finish processing of the message */
                 }
 
                 if (e == p) {
@@ -132,8 +131,7 @@ static int server_process_entry(
                 }
 
                 if (IN_SET(*p, '.', '#')) {
-                        /* Ignore control commands for now, and
-                         * comments too. */
+                        /* Ignore control commands for now, and comments too. */
                         *remaining -= (e - p) + 1;
                         p = e + 1;
                         continue;
@@ -142,7 +140,6 @@ static int server_process_entry(
                 /* A property follows */
                 if (n > ENTRY_FIELD_COUNT_MAX) {
                         log_debug("Received an entry that has more than " STRINGIFY(ENTRY_FIELD_COUNT_MAX) " fields, ignoring entry.");
-                        r = 1;
                         goto finish;
                 }
 
@@ -152,7 +149,7 @@ static int server_process_entry(
                                     N_IOVEC_META_FIELDS + N_IOVEC_OBJECT_FIELDS +
                                     client_context_extra_fields_n_iovec(context))) {
                         r = log_oom();
-                        break;
+                        goto finish;
                 }
 
                 q = memchr(p, '=', e - p);
@@ -161,6 +158,16 @@ static int server_process_entry(
                                 size_t l;
 
                                 l = e - p;
+                                if (l > DATA_SIZE_MAX) {
+                                        log_debug("Received text block of %zu bytes is too large, ignoring entry.", l);
+                                        goto finish;
+                                }
+
+                                if (entry_size + l + n + 1 > ENTRY_SIZE_MAX) { /* data + separators + trailer */
+                                        log_debug("Entry is too big (%zu bytes after processing %zu entries), ignoring entry.",
+                                                  entry_size + l, n + 1);
+                                        goto finish;
+                                }
 
                                 /* If the field name starts with an underscore, skip the variable, since that indicates
                                  * a trusted field */
@@ -178,7 +185,7 @@ static int server_process_entry(
                         p = e + 1;
                         continue;
                 } else {
-                        uint64_t l;
+                        uint64_t l, total;
                         char *k;
 
                         if (*remaining < e - p + 1 + sizeof(uint64_t) + 1) {
@@ -187,10 +194,16 @@ static int server_process_entry(
                         }
 
                         l = unaligned_read_le64(e + 1);
-
                         if (l > DATA_SIZE_MAX) {
-                                log_debug("Received binary data block of %"PRIu64" bytes is too large, ignoring.", l);
-                                break;
+                                log_debug("Received binary data block of %"PRIu64" bytes is too large, ignoring entry.", l);
+                                goto finish;
+                        }
+
+                        total = (e - p) + 1 + l;
+                        if (entry_size + total + n + 1 > ENTRY_SIZE_MAX) { /* data + separators + trailer */
+                                log_debug("Entry is too big (%"PRIu64"bytes after processing %zu fields), ignoring.",
+                                          entry_size + total, n + 1);
+                                goto finish;
                         }
 
                         if ((uint64_t) *remaining < e - p + 1 + sizeof(uint64_t) + l + 1 ||
@@ -199,7 +212,7 @@ static int server_process_entry(
                                 break;
                         }
 
-                        k = malloc((e - p) + 1 + l);
+                        k = malloc(total);
                         if (!k) {
                                 log_oom();
                                 break;
@@ -228,15 +241,8 @@ static int server_process_entry(
                 }
         }
 
-        if (n <= 0) {
-                r = 1;
+        if (n <= 0)
                 goto finish;
-        }
-
-        if (!client_context_test_priority(context, priority)) {
-                r = 0;
-                goto finish;
-        }
 
         tn = n++;
         iovec[tn] = IOVEC_MAKE_STRING("_TRANSPORT=journal");
@@ -247,6 +253,11 @@ static int server_process_entry(
                 goto finish;
         }
 
+        r = 0; /* Success, we read the message. */
+
+        if (!client_context_test_priority(context, priority))
+                goto finish;
+
         if (message) {
                 if (s->forward_to_syslog)
                         server_forward_syslog(s, syslog_fixup_facility(priority), identifier, message, ucred, tv);
@@ -318,15 +329,13 @@ void server_process_native_file(
         bool sealed;
         int r;
 
-        /* Data is in the passed fd, since it didn't fit in a
-         * datagram. */
+        /* Data is in the passed fd, probably it didn't fit in a datagram. */
 
         assert(s);
         assert(fd >= 0);
 
         /* If it's a memfd, check if it is sealed. If so, we can just
-         * use map it and use it, and do not need to copy the data
-         * out. */
+         * mmap it and use it, and do not need to copy the data out. */
         sealed = memfd_get_sealed(fd) > 0;
 
         if (!sealed && (!ucred || ucred->uid != 0)) {
@@ -397,7 +406,7 @@ void server_process_native_file(
                 ssize_t n;
 
                 if (fstatvfs(fd, &vfs) < 0) {
-                        log_error_errno(errno, "Failed to stat file system of passed file, ignoring: %m");
+                        log_error_errno(errno, "Failed to stat file system of passed file, not processing it: %m");
                         return;
                 }
 
@@ -407,7 +416,7 @@ void server_process_native_file(
                  * https://github.com/systemd/systemd/issues/1822
                  */
                 if (vfs.f_flag & ST_MANDLOCK) {
-                        log_error("Received file descriptor from file system with mandatory locking enabled, refusing.");
+                        log_error("Received file descriptor from file system with mandatory locking enabled, not processing it.");
                         return;
                 }
 
@@ -420,13 +429,13 @@ void server_process_native_file(
                  * and so is SMB. */
                 r = fd_nonblock(fd, true);
                 if (r < 0) {
-                        log_error_errno(r, "Failed to make fd non-blocking, ignoring: %m");
+                        log_error_errno(r, "Failed to make fd non-blocking, not processing it: %m");
                         return;
                 }
 
                 /* The file is not sealed, we can't map the file here, since
                  * clients might then truncate it and trigger a SIGBUS for
-                 * us. So let's stupidly read it */
+                 * us. So let's stupidly read it. */
 
                 p = malloc(st.st_size);
                 if (!p) {