From 452a8cf4fb4a1c818acb90beabff5f5e6b2e7551 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Tue, 20 Dec 2022 17:14:07 +0100 Subject: [PATCH] Resolves: #2148934 - enforce stricter parsing of config files --- ...rotate-3.18.0-stricter-config-parser.patch | 556 ++++++++++++++++++ logrotate.spec | 8 +- 2 files changed, 563 insertions(+), 1 deletion(-) create mode 100644 0005-logrotate-3.18.0-stricter-config-parser.patch diff --git a/0005-logrotate-3.18.0-stricter-config-parser.patch b/0005-logrotate-3.18.0-stricter-config-parser.patch new file mode 100644 index 0000000..282566b --- /dev/null +++ b/0005-logrotate-3.18.0-stricter-config-parser.patch @@ -0,0 +1,556 @@ +From 6db706b51af0a6f6ce28bceaefb4157347d2fa18 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= +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 +--- + 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?= +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 +--- + 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?= +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 +--- + 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?= +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 +--- + 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 +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 +--- + 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?= +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 +--- + 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 < - 3.18.0-8 +- enforce stricter parsing of config files (#2148925) + * Fri May 27 2022 Kamil Dudka - 3.18.0-7 - lockState: do not print `error:` when exit code is unaffected (#2090926)