From e9fa9637b895293fac8e27deb153d68f1c2b6ab9 Mon Sep 17 00:00:00 2001
From: Felix Wilhelm <fwilhelm@google.com>
Date: Thu, 21 Oct 2021 09:47:57 +0000
Subject: [PATCH] config.c: enforce stricter parsing of config files

Abort parsing of config files that contain invalid lines.
This makes it harder to abuse logrotate for privilege escalation
attacks where an attacker can partially control a privileged file write.
---
 ChangeLog.md            |  1 +
 config.c                |  7 ++++---
 test/Makefile.am        |  4 +++-
 test/test-0102.sh       | 16 ++++++++++++++++
 test/test-0103.sh       | 16 ++++++++++++++++
 test/test-config.102.in | 10 ++++++++++
 test/test-config.103.in | 12 ++++++++++++
 7 files changed, 62 insertions(+), 4 deletions(-)
 create mode 100755 test/test-0102.sh
 create mode 100755 test/test-0103.sh
 create mode 100644 test/test-config.102.in
 create mode 100644 test/test-config.103.in

Index: logrotate-3.18.1/ChangeLog.md
===================================================================
--- logrotate-3.18.1.orig/ChangeLog.md
+++ logrotate-3.18.1/ChangeLog.md
@@ -5,6 +5,7 @@ All notable changes to this project will
 ## [UNRELEASED]
 
 [UNRELEASED]: https://github.com/logrotate/logrotate/compare/3.18.1...master
+  - enforce stricter parsing of configuration files
 
 ## [3.18.1] - 2021-05-21
   - fix memory leaks on error-handling paths (#383, #387)
Index: logrotate-3.18.1/config.c
===================================================================
--- logrotate-3.18.1.orig/config.c
+++ logrotate-3.18.1/config.c
@@ -1095,12 +1095,13 @@ static int readConfigFile(const char *co
                     if (key == NULL) {
                         message(MESS_ERROR, "%s:%d failed to parse keyword\n",
                                 configFile, lineNum);
-                        continue;
+                        RAISE_ERROR();
                     }
                     if (!isspace((unsigned char)*start)) {
-                        message(MESS_NORMAL, "%s:%d keyword '%s' not properly"
+                        message(MESS_ERROR, "%s:%d keyword '%s' not properly"
                                 " separated, found %#x\n",
                                 configFile, lineNum, key, *start);
+                        RAISE_ERROR();
                     }
                     if (!strcmp(key, "compress")) {
                         newlog->flags |= LOG_FLAG_COMPRESS;
@@ -1978,7 +1979,7 @@ duperror:
                     message(MESS_ERROR, "%s:%d lines must begin with a keyword "
                             "or a filename (possibly in double quotes)\n",
                             configFile, lineNum);
-                    state = STATE_SKIP_LINE;
+                    RAISE_ERROR();
                 }
                 break;
             case STATE_SKIP_LINE:
Index: logrotate-3.18.1/test/Makefile.am
===================================================================
--- logrotate-3.18.1.orig/test/Makefile.am
+++ logrotate-3.18.1/test/Makefile.am
@@ -89,7 +89,9 @@ TEST_CASES = \
 	test-0088.sh \
 	test-0092.sh \
 	test-0100.sh \
-	test-0101.sh
+	test-0101.sh \
+	test-0102.sh \
+	test-0103.sh
 
 EXTRA_DIST = \
 	compress \
Index: logrotate-3.18.1/test/test-0102.sh
===================================================================
--- /dev/null
+++ logrotate-3.18.1/test/test-0102.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+. ./test-common.sh
+
+cleanup 102
+
+# ------------------------------- Test 102 ------------------------------------
+# test invalid config file with binary content
+preptest test.log 102 1
+
+$RLR test-config.102 --force
+
+if [ $? -eq 0 ]; then
+   echo "No error, but there should be one."
+   exit 3
+fi
Index: logrotate-3.18.1/test/test-0103.sh
===================================================================
--- /dev/null
+++ logrotate-3.18.1/test/test-0103.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+. ./test-common.sh
+
+cleanup 103
+
+# ------------------------------- Test 103 ------------------------------------
+# test invalid config file with unknown keywords
+preptest test.log 103 1
+
+$RLR test-config.103 --force
+
+if [ $? -eq 0 ]; then
+   echo "No error, but there should be one."
+   exit 3
+fi
Index: logrotate-3.18.1/test/test-config.102.in
===================================================================
--- /dev/null
+++ logrotate-3.18.1/test/test-config.102.in
@@ -0,0 +1,10 @@
+ELF
+
+&DIR&/test.log {
+ daily
+ size=0
+
+firstaction
+ /bin/sh -c "echo test123"
+ endscript
+}
Index: logrotate-3.18.1/test/test-config.103.in
===================================================================
--- /dev/null
+++ logrotate-3.18.1/test/test-config.103.in
@@ -0,0 +1,12 @@
+random noise
+a b c d
+a::x
+
+&DIR&/test.log {
+ daily
+ size=0
+
+firstaction
+ /bin/sh -c "echo test123"
+ endscript
+}