From ed028441cc2ef0ffb9771d7266d40f18910f0ae1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Wed, 5 Dec 2018 22:50:39 +0100
Subject: [PATCH] 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.

(cherry-picked from commit 964ef920ea6735d39f856b05fd8ef451a09a6a1d)

Related: #1664977
---
 src/journal/journald-native.c | 65 ++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c
index 951d092053..110ab3641c 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) {