From 6db706b51af0a6f6ce28bceaefb4157347d2fa18 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com>

Date: Tue, 20 Apr 2021 17:41:10 +0200

Subject: [PATCH 1/6] Log if keyword is not properly separated

The man page states

  Values are separated from directives by whitespace and/or an

  optional =.

But logrotate does accept no separator, like

  rotate7

Log those occurrences with a normal severity, as this usage is not

intended.

Upstream-commit: 2b588b5ec2e5c27bee857c4abeddafa6a9602ebc

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 config.c | 5 +++++

 1 file changed, 5 insertions(+)

diff --git a/config.c b/config.c

index 1bca9e4..8049211 100644

--- a/config.c

+++ b/config.c

@@ -1088,6 +1088,11 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                     key = isolateWord(&start, &buf, length);

                     if (key == NULL)

                         continue;

+                    if (!isspace((unsigned char)*start)) {

+                        message(MESS_NORMAL, "%s:%d keyword '%s' not properly"

+                                " separated, found %#x\n",

+                                configFile, lineNum, key, *start);

+                    }

                     if (!strcmp(key, "compress")) {

                         newlog->flags |= LOG_FLAG_COMPRESS;

                     } else if (!strcmp(key, "nocompress")) {

-- 

2.38.1

From 2a22bf99b41e737fcd8c986be5c4fb761ab101c7 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com>

Date: Tue, 20 Apr 2021 17:41:12 +0200

Subject: [PATCH 2/6] Log error on keyword parse failure

isolateWord() only fails on OOM and EOF.

Upstream-commit: 326179a901b0a8d10e902cae0abab0c68d7abc98

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 config.c | 5 ++++-

 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c

index 8049211..fd6e026 100644

--- a/config.c

+++ b/config.c

@@ -1086,8 +1086,11 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                 if (isalpha((unsigned char)*start)) {

                     free(key);

                     key = isolateWord(&start, &buf, length);

-                    if (key == NULL)

+                    if (key == NULL) {

+                        message(MESS_ERROR, "%s:%d failed to parse keyword\n",

+                                configFile, lineNum);

                         continue;

+                    }

                     if (!isspace((unsigned char)*start)) {

                         message(MESS_NORMAL, "%s:%d keyword '%s' not properly"

                                 " separated, found %#x\n",

-- 

2.38.1

From d3b2d0d058d41dd7efccadff8506285af791711c Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com>

Date: Tue, 20 Apr 2021 17:41:20 +0200

Subject: [PATCH 3/6] Fail on parse error of required option value

Fail on a parse error of a required option value of the directives

include, extension, addextension, rotate, start, minage, maxage,

shredcycles and su.

Failing is better than silently skipping a directive and running with an

undesired configuration.

Upstream-commit: 906ea11981cb1842538c4aaed395885fda693e47

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 config.c | 49 ++++++++++++++++++++++++++++++-------------------

 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/config.c b/config.c

index fd6e026..227feec 100644

--- a/config.c

+++ b/config.c

@@ -1154,8 +1154,11 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         mode_t tmp_mode = NO_MODE;

                         free(key);

                         key = isolateLine(&start, &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            message(MESS_ERROR, "%s:%d failed to parse su option value\n",

+                                    configFile, lineNum);

+                            RAISE_ERROR();

+                        }

                         rv = readModeUidGid(configFile, lineNum, key, "su",

                                             &tmp_mode, &newlog->suUid,

@@ -1268,13 +1271,14 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         free(key);

                         key = isolateValue(configFile, lineNum, "shred cycles",

                                            &start, &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            RAISE_ERROR();

+                        }

                         newlog->shred_cycles = (int)strtoul(key, &chptr, 0);

                         if (*chptr || newlog->shred_cycles < 0) {

                             message(MESS_ERROR, "%s:%d bad shred cycles '%s'\n",

                                     configFile, lineNum, key);

-                            goto error;

+                            RAISE_ERROR();

                         }

                     } else if (!strcmp(key, "hourly")) {

                         set_criterium(&newlog->criterium, ROT_HOURLY, &criterium_set);

@@ -1309,8 +1313,9 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         free(key);

                         key = isolateValue(configFile, lineNum, "rotate count", &start,

                                            &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            RAISE_ERROR();

+                        }

                         newlog->rotateCount = (int)strtol(key, &chptr, 0);

                         if (*chptr || newlog->rotateCount < -1) {

                             message(MESS_ERROR,

@@ -1322,8 +1327,9 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         free(key);

                         key = isolateValue(configFile, lineNum, "start count", &start,

                                            &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            RAISE_ERROR();

+                        }

                         newlog->logStart = (int)strtoul(key, &chptr, 0);

                         if (*chptr || newlog->logStart < 0) {

                             message(MESS_ERROR, "%s:%d bad start count '%s'\n",

@@ -1334,8 +1340,9 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         free(key);

                         key = isolateValue(configFile, lineNum, "minage count", &start,

                                            &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            RAISE_ERROR();

+                        }

                         newlog->rotateMinAge = (int)strtoul(key, &chptr, 0);

                         if (*chptr || newlog->rotateMinAge < 0) {

                             message(MESS_ERROR, "%s:%d bad minimum age '%s'\n",

@@ -1346,8 +1353,9 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         free(key);

                         key = isolateValue(configFile, lineNum, "maxage count", &start,

                                            &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            RAISE_ERROR();

+                        }

                         newlog->rotateAge = (int)strtoul(key, &chptr, 0);

                         if (*chptr || newlog->rotateAge < 0) {

                             message(MESS_ERROR, "%s:%d bad maximum age '%s'\n",

@@ -1519,8 +1527,9 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         free(key);

                         key = isolateValue(configFile, lineNum, "include", &start,

                                            &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            RAISE_ERROR();

+                        }

                         if (key[0] == '~' && key[1] == '/') {

                             /* replace '~' with content of $HOME cause low-level functions

@@ -1582,8 +1591,9 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         free(key);

                         key = isolateValue(configFile, lineNum, "extension name", &start,

                                            &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            RAISE_ERROR();

+                        }

                         freeLogItem (extension);

                         newlog->extension = key;

                         key = NULL;

@@ -1593,8 +1603,9 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                         free(key);

                         key = isolateValue(configFile, lineNum, "addextension name", &start,

                                            &buf, length);

-                        if (key == NULL)

-                            continue;

+                        if (key == NULL) {

+                            RAISE_ERROR();

+                        }

                         freeLogItem (addextension);

                         newlog->addextension = key;

                         key = NULL;

-- 

2.38.1

From 69d2febc6e6e81e34d944b1652144df2e154965d Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com>

Date: Mon, 26 Jul 2021 19:35:00 +0200

Subject: [PATCH 4/6] Do not warn on key value pair separated by only an equal

 sign

Do not warn if a configuration directive is specified with the key and

value separated by just an equal sign, like:

    size=+2048k

The warning is intended for the usage of:

    size2048k

Fixes: 2b588b5e ("Log if keyword is not properly separated")

Fixes: #410

Upstream-commit: a98c38bc867ec59e00625b48262bb3334c8f5728

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 config.c | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c

index 227feec..6eb94d4 100644

--- a/config.c

+++ b/config.c

@@ -1091,7 +1091,7 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                                 configFile, lineNum);

                         continue;

                     }

-                    if (!isspace((unsigned char)*start)) {

+                    if (!isspace((unsigned char)*start) && *start != '=') {

                         message(MESS_NORMAL, "%s:%d keyword '%s' not properly"

                                 " separated, found %#x\n",

                                 configFile, lineNum, key, *start);

-- 

2.38.1

From 3a1f8e746b2753efe7472580b1db4395553b1d34 Mon Sep 17 00:00:00 2001

From: Felix Wilhelm <fwilhelm@google.com>

Date: Thu, 21 Oct 2021 09:47:57 +0000

Subject: [PATCH 5/6] 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.

Upstream-commit: 124e4ca6532b0fe823fa2ec145294547b3aaeb4b

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 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 ++++++++++++

 6 files changed, 61 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

diff --git a/config.c b/config.c

index 6eb94d4..c0fd4ff 100644

--- a/config.c

+++ b/config.c

@@ -1089,12 +1089,13 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig)

                     if (key == NULL) {

                         message(MESS_ERROR, "%s:%d failed to parse keyword\n",

                                 configFile, lineNum);

-                        continue;

+                        RAISE_ERROR();

                     }

                     if (!isspace((unsigned char)*start) && *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;

@@ -1973,7 +1974,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:

diff --git a/test/Makefile.am b/test/Makefile.am

index d6fb7c8..cd357e5 100644

--- a/test/Makefile.am

+++ b/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 \

diff --git a/test/test-0102.sh b/test/test-0102.sh

new file mode 100755

index 0000000..d2550a5

--- /dev/null

+++ b/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

diff --git a/test/test-0103.sh b/test/test-0103.sh

new file mode 100755

index 0000000..bccd8ed

--- /dev/null

+++ b/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

diff --git a/test/test-config.102.in b/test/test-config.102.in

new file mode 100644

index 0000000..cbca4c4

--- /dev/null

+++ b/test/test-config.102.in

@@ -0,0 +1,10 @@

+ELF

+

+&DIR&/test.log {

+ daily

+ size=0

+

+firstaction

+ /bin/sh -c "echo test123"

+ endscript

+}

diff --git a/test/test-config.103.in b/test/test-config.103.in

new file mode 100644

index 0000000..ef4d19c

--- /dev/null

+++ b/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

+}

-- 

2.38.1

From 2ad71221cd9e485e4d45df4f28b47072491df120 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com>

Date: Mon, 13 Dec 2021 21:47:16 +0100

Subject: [PATCH 6/6] Add more testcases for stricter configuration parsing

Upstream-commit: 9cbc22b91caff6cfbd1378737c62276bd9ffe3e7

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 test/Makefile.am        |  4 +++-

 test/test-0102.sh       |  5 +++++

 test/test-0103.sh       |  5 +++++

 test/test-0104.sh       | 19 +++++++++++++++++++

 test/test-0105.sh       | 25 +++++++++++++++++++++++++

 test/test-config.104.in |  8 ++++++++

 test/test-config.105.in |  8 ++++++++

 7 files changed, 73 insertions(+), 1 deletion(-)

 create mode 100755 test/test-0104.sh

 create mode 100755 test/test-0105.sh

 create mode 100644 test/test-config.104.in

 create mode 100644 test/test-config.105.in

diff --git a/test/Makefile.am b/test/Makefile.am

index cd357e5..f1a0062 100644

--- a/test/Makefile.am

+++ b/test/Makefile.am

@@ -91,7 +91,9 @@ TEST_CASES = \

 	test-0100.sh \

 	test-0101.sh \

 	test-0102.sh \

-	test-0103.sh

+	test-0103.sh \

+	test-0104.sh \

+	test-0105.sh

 EXTRA_DIST = \

 	compress \

diff --git a/test/test-0102.sh b/test/test-0102.sh

index d2550a5..367bde9 100755

--- a/test/test-0102.sh

+++ b/test/test-0102.sh

@@ -14,3 +14,8 @@ if [ $? -eq 0 ]; then

    echo "No error, but there should be one."

    exit 3

 fi

+

+checkoutput <

+test.log 0 zero

+test.log.1 0 first

+EOF

diff --git a/test/test-0103.sh b/test/test-0103.sh

index bccd8ed..32a3c19 100755

--- a/test/test-0103.sh

+++ b/test/test-0103.sh

@@ -14,3 +14,8 @@ if [ $? -eq 0 ]; then

    echo "No error, but there should be one."

    exit 3

 fi

+

+checkoutput <

+test.log 0 zero

+test.log.1 0 first

+EOF

diff --git a/test/test-0104.sh b/test/test-0104.sh

new file mode 100755

index 0000000..e3c0009

--- /dev/null

+++ b/test/test-0104.sh

@@ -0,0 +1,19 @@

+#!/bin/sh

+

+. ./test-common.sh

+

+cleanup 104

+

+# ------------------------------- Test 104 ------------------------------------

+# test config with unknown (new?) keyword

+preptest test1.log 104 1

+preptest test2.log 104 1

+

+$RLR test-config.104 --force || exit 23

+

+checkoutput <

+test1.log 0

+test1.log.1 0 zero

+test2.log 0

+test2.log.1 0 zero

+EOF

diff --git a/test/test-0105.sh b/test/test-0105.sh

new file mode 100755

index 0000000..b51e9be

--- /dev/null

+++ b/test/test-0105.sh

@@ -0,0 +1,25 @@

+#!/bin/sh

+

+. ./test-common.sh

+

+cleanup 105

+

+# ------------------------------- Test 105 ------------------------------------

+# test config with garbage keyword bails out

+preptest test1.log 105 1

+preptest test2.log 105 1

+

+$RLR test-config.105 --force

+

+if [ $? -eq 0 ]; then

+   echo "No error, but there should be one."

+   exit 3

+fi

+

+

+checkoutput <

+test1.log 0 zero

+test1.log.1 0 first

+test2.log 0

+test2.log.1 0 zero

+EOF

diff --git a/test/test-config.104.in b/test/test-config.104.in

new file mode 100644

index 0000000..988d902

--- /dev/null

+++ b/test/test-config.104.in

@@ -0,0 +1,8 @@

+&DIR&/test1.log {

+    newkeyword

+    rotate 1

+}

+

+&DIR&/test2.log {

+    rotate 1

+}

diff --git a/test/test-config.105.in b/test/test-config.105.in

new file mode 100644

index 0000000..bfab9b9

--- /dev/null

+++ b/test/test-config.105.in

@@ -0,0 +1,8 @@

+&DIR&/test1.log {

+    g@rbag€[]+#*

+    rotate 1

+}

+

+&DIR&/test2.log {

+    rotate 1

+}

-- 

2.38.1