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 <