From 03695c416f7f8311afbded390f3c0ff3637a10d4 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Mon, 20 May 2019 11:38:05 -0400 Subject: [PATCH] Ticket 50393 - maxlogsperdir accepting negative values Description: Improve the log "digit" config setting validation for all settings. https://pagure.io/389-ds-base/issue/50393 Reviewed by: tbordaz, firstyear, mhonek, and spichugi (Thanks!!!!) (cherry picked from commit ca70d06fbb7a2c06c62f0ba5b192dba36f24b8e3) --- dirsrvtests/tests/suites/logging/__init__.py | 3 + .../suites/logging/logging_config_test.py | 86 +++++++++++ ldap/servers/slapd/log.c | 143 +++++++++++++----- 3 files changed, 192 insertions(+), 40 deletions(-) create mode 100644 dirsrvtests/tests/suites/logging/__init__.py create mode 100644 dirsrvtests/tests/suites/logging/logging_config_test.py diff --git a/dirsrvtests/tests/suites/logging/__init__.py b/dirsrvtests/tests/suites/logging/__init__.py new file mode 100644 index 000000000..7f812e357 --- /dev/null +++ b/dirsrvtests/tests/suites/logging/__init__.py @@ -0,0 +1,3 @@ +""" + :Requirement: 389-ds-base: Directory Server Logging Configurations +""" diff --git a/dirsrvtests/tests/suites/logging/logging_config_test.py b/dirsrvtests/tests/suites/logging/logging_config_test.py new file mode 100644 index 000000000..4d8d68ab5 --- /dev/null +++ b/dirsrvtests/tests/suites/logging/logging_config_test.py @@ -0,0 +1,86 @@ +import logging +import pytest +import os +import ldap +from lib389._constants import * +from lib389.topologies import topology_st as topo + +DEBUGGING = os.getenv("DEBUGGING", default=False) +if DEBUGGING: + logging.getLogger(__name__).setLevel(logging.DEBUG) +else: + logging.getLogger(__name__).setLevel(logging.INFO) +log = logging.getLogger(__name__) + +big_value = "1111111111111111111111111111111111111111111" + + +@pytest.mark.parametrize("attr, invalid_vals, valid_vals", + [ + ("logexpirationtime", ["-2", "0"], ["1", "-1"]), + ("maxlogsize", ["-2", "0"], ["100", "-1"]), + ("logmaxdiskspace", ["-2", "0"], ["100", "-1"]), + ("logminfreediskspace", ["-2", "0"], ["100", "-1"]), + ("mode", ["888", "778", "77", "7777"], ["777", "000", "600"]), + ("maxlogsperdir", ["-1", "0"], ["1", "20"]), + ("logrotationsynchour", ["-1", "24"], ["0", "23"]), + ("logrotationsyncmin", ["-1", "60"], ["0", "59"]), + ("logrotationtime", ["-2", "0"], ["100", "-1"]) + ]) +def test_logging_digit_config(topo, attr, invalid_vals, valid_vals): + """Validate logging config settings + + :id: a0ef30e5-538b-46fa-9762-01a4435a15e9 + :setup: Standalone Instance + :steps: + 1. Test log expiration time + 2. Test log max size + 3. Test log max disk space + 4. Test log min disk space + 5. Test log mode + 6. Test log max number of logs + 7. Test log rotation hour + 8. Test log rotation minute + 9. Test log rotation time + :expectedresults: + 1. Success + 2. Success + 3. Success + 4. Success + 5. Success + 6. Success + 7. Success + 8. Success + 9. Success + """ + + accesslog_attr = "nsslapd-accesslog-{}".format(attr) + auditlog_attr = "nsslapd-auditlog-{}".format(attr) + auditfaillog_attr = "nsslapd-auditfaillog-{}".format(attr) + errorlog_attr = "nsslapd-errorlog-{}".format(attr) + + # Test each log + for attr in [accesslog_attr, auditlog_attr, auditfaillog_attr, errorlog_attr]: + # Invalid values + for invalid_val in invalid_vals: + with pytest.raises(ldap.LDAPError): + topo.standalone.config.set(attr, invalid_val) + + # Invalid high value + with pytest.raises(ldap.LDAPError): + topo.standalone.config.set(attr, big_value) + + # Non digits + with pytest.raises(ldap.LDAPError): + topo.standalone.config.set(attr, "abc") + + # Valid values + for valid_val in valid_vals: + topo.standalone.config.set(attr, valid_val) + + +if __name__ == '__main__': + # Run isolated + # -s for DEBUG mode + CURRENT_FILE = os.path.realpath(__file__) + pytest.main(["-s", CURRENT_FILE]) diff --git a/ldap/servers/slapd/log.c b/ldap/servers/slapd/log.c index 7dd71541b..2456abf1e 100644 --- a/ldap/servers/slapd/log.c +++ b/ldap/servers/slapd/log.c @@ -817,8 +817,9 @@ log_update_auditfaillogdir(char *pathname, int apply) int log_set_mode(const char *attrname, char *value, int logtype, char *errorbuf, int apply) { - int v = 0; + int64_t v = 0; int retval = LDAP_SUCCESS; + char *endp = NULL; slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (NULL == value) { @@ -833,7 +834,18 @@ log_set_mode(const char *attrname, char *value, int logtype, char *errorbuf, int return LDAP_SUCCESS; } - v = strtol(value, NULL, 8); + errno = 0; + v = strtol(value, &endp, 8); + if (*endp != '\0' || errno == ERANGE || + strlen(value) != 3 || + v > 0777 /* octet of 777 511 */ || + v < 0) + { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s) (%ld), value must be three digits between 000 and 777", + value, attrname, v); + return LDAP_UNWILLING_TO_PERFORM; + } switch (logtype) { case SLAPD_ACCESS_LOG: @@ -895,9 +907,9 @@ int log_set_numlogsperdir(const char *attrname, char *numlogs_str, int logtype, char *returntext, int apply) { slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); - + char *endp = NULL; int rv = LDAP_SUCCESS; - int numlogs; + int64_t numlogs; if (logtype != SLAPD_ACCESS_LOG && logtype != SLAPD_ERROR_LOG && @@ -911,7 +923,14 @@ log_set_numlogsperdir(const char *attrname, char *numlogs_str, int logtype, char return rv; } - numlogs = atoi(numlogs_str); + errno = 0; + numlogs = strtol(numlogs_str, &endp, 10); + if (*endp != '\0' || errno == ERANGE || numlogs < 1) { + PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s), value must be between 1 and 2147483647", + numlogs_str, attrname); + return LDAP_UNWILLING_TO_PERFORM; + } if (numlogs >= 1) { switch (logtype) { @@ -960,21 +979,25 @@ int log_set_logsize(const char *attrname, char *logsize_str, int logtype, char *returntext, int apply) { int rv = LDAP_SUCCESS; - PRInt64 max_logsize; /* in bytes */ - int logsize; /* in megabytes */ + int64_t max_logsize; /* in bytes */ + int64_t logsize; /* in megabytes */ + char *endp = NULL; slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (!apply || !logsize_str || !*logsize_str) return rv; - logsize = atoi(logsize_str); + errno = 0; + logsize = strtol(logsize_str, &endp, 10); + if (*endp != '\0' || errno == ERANGE || logsize < -1 || logsize == 0) { + PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than 0", + logsize_str, attrname); + return LDAP_UNWILLING_TO_PERFORM; + } /* convert it to bytes */ - max_logsize = (PRInt64)logsize * LOG_MB_IN_BYTES; - - if (max_logsize <= 0) { - max_logsize = -1; - } + max_logsize = logsize * LOG_MB_IN_BYTES; switch (logtype) { case SLAPD_ACCESS_LOG: @@ -1101,8 +1124,9 @@ log_set_rotationsync_enabled(const char *attrname, char *value, int logtype, cha int log_set_rotationsynchour(const char *attrname, char *rhour_str, int logtype, char *returntext, int apply) { - int rhour = -1; + int64_t rhour = -1; int rv = LDAP_SUCCESS; + char *endp = NULL; slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (logtype != SLAPD_ACCESS_LOG && @@ -1115,12 +1139,19 @@ log_set_rotationsynchour(const char *attrname, char *rhour_str, int logtype, cha } /* return if we aren't doing this for real */ - if (!apply) { + if (!apply || !rhour_str || !*rhour_str) { return rv; } - if (rhour_str && *rhour_str != '\0') - rhour = atol(rhour_str); + errno = 0; + rhour = strtol(rhour_str, &endp, 10); + if (*endp != '\0' || errno == ERANGE || rhour < 0 || rhour > 23) { + PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s), value must be \"0\" thru \"23\"", + rhour_str, attrname); + return LDAP_UNWILLING_TO_PERFORM; + } + if (rhour > 23) rhour = rhour % 24; @@ -1161,8 +1192,9 @@ log_set_rotationsynchour(const char *attrname, char *rhour_str, int logtype, cha int log_set_rotationsyncmin(const char *attrname, char *rmin_str, int logtype, char *returntext, int apply) { - int rmin = -1; + int64_t rmin = -1; int rv = LDAP_SUCCESS; + char *endp = NULL; slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (logtype != SLAPD_ACCESS_LOG && @@ -1175,14 +1207,18 @@ log_set_rotationsyncmin(const char *attrname, char *rmin_str, int logtype, char } /* return if we aren't doing this for real */ - if (!apply) { + if (!apply || !rmin_str || !*rmin_str) { return rv; } - if (rmin_str && *rmin_str != '\0') - rmin = atol(rmin_str); - if (rmin > 59) - rmin = rmin % 60; + errno = 0; + rmin = strtol(rmin_str, &endp, 10); + if (*endp != '\0' || errno == ERANGE || rmin < 0 || rmin > 59) { + PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s), value must be between \"0\" and \"59\"", + rmin_str, attrname); + return LDAP_UNWILLING_TO_PERFORM; + } switch (logtype) { case SLAPD_ACCESS_LOG: @@ -1229,8 +1265,9 @@ log_set_rotationtime(const char *attrname, char *rtime_str, int logtype, char *r { int runit = 0; - int value, rtime; + int64_t value, rtime; int rv = LDAP_SUCCESS; + char *endp = NULL; slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (logtype != SLAPD_ACCESS_LOG && @@ -1247,7 +1284,14 @@ log_set_rotationtime(const char *attrname, char *rtime_str, int logtype, char *r return rv; } - rtime = atoi(rtime_str); + errno = 0; + rtime = strtol(rtime_str, &endp, 10); + if (*endp != '\0' || errno == ERANGE || rtime < -1 || rtime == 0) { + PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than \"0\"", + rtime_str, attrname); + return LDAP_UNWILLING_TO_PERFORM; + } if (0 == rtime) { rtime = -1; /* Value Range: -1 | 1 to PR_INT32_MAX */ @@ -1332,7 +1376,6 @@ log_set_rotationtimeunit(const char *attrname, char *runit, int logtype, char *e int origvalue = 0, value = 0; int runitType; int rv = 0; - slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (logtype != SLAPD_ACCESS_LOG && @@ -1448,10 +1491,10 @@ int log_set_maxdiskspace(const char *attrname, char *maxdiskspace_str, int logtype, char *errorbuf, int apply) { int rv = 0; - PRInt64 mlogsize = 0; /* in bytes */ - PRInt64 maxdiskspace; /* in bytes */ - int s_maxdiskspace; /* in megabytes */ - + int64_t mlogsize = 0; /* in bytes */ + int64_t maxdiskspace; /* in bytes */ + int64_t s_maxdiskspace; /* in megabytes */ + char *endp = NULL; slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (logtype != SLAPD_ACCESS_LOG && @@ -1465,7 +1508,14 @@ log_set_maxdiskspace(const char *attrname, char *maxdiskspace_str, int logtype, if (!apply || !maxdiskspace_str || !*maxdiskspace_str) return rv; - s_maxdiskspace = atoi(maxdiskspace_str); + errno = 0; + s_maxdiskspace = strtol(maxdiskspace_str, &endp, 10); + if (*endp != '\0' || errno == ERANGE || s_maxdiskspace < -1 || s_maxdiskspace == 0) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than 0", + maxdiskspace_str, attrname); + return LDAP_UNWILLING_TO_PERFORM; + } /* Disk space are in MB but store in bytes */ switch (logtype) { @@ -1538,9 +1588,9 @@ int log_set_mindiskspace(const char *attrname, char *minfreespace_str, int logtype, char *errorbuf, int apply) { int rv = LDAP_SUCCESS; - int minfreespace; /* in megabytes */ - PRInt64 minfreespaceB; /* in bytes */ - + int64_t minfreespace; /* in megabytes */ + int64_t minfreespaceB; /* in bytes */ + char *endp = NULL; slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (logtype != SLAPD_ACCESS_LOG && @@ -1556,11 +1606,18 @@ log_set_mindiskspace(const char *attrname, char *minfreespace_str, int logtype, return rv; } - minfreespace = atoi(minfreespace_str); + errno = 0; + minfreespace = strtol(minfreespace_str, &endp, 10); + if (*endp != '\0' || errno == ERANGE || minfreespace < -1 || minfreespace == 0) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than 0", + minfreespace_str, attrname); + return LDAP_UNWILLING_TO_PERFORM; + } /* Disk space are in MB but store in bytes */ if (minfreespace >= 1) { - minfreespaceB = (PRInt64)minfreespace * LOG_MB_IN_BYTES; + minfreespaceB = minfreespace * LOG_MB_IN_BYTES; switch (logtype) { case SLAPD_ACCESS_LOG: LOG_ACCESS_LOCK_WRITE(); @@ -1602,10 +1659,10 @@ log_set_mindiskspace(const char *attrname, char *minfreespace_str, int logtype, int log_set_expirationtime(const char *attrname, char *exptime_str, int logtype, char *errorbuf, int apply) { - - int eunit, value, exptime; + int64_t eunit, value, exptime; int rsec = 0; int rv = 0; + char *endp = NULL; slapdFrontendConfig_t *fe_cfg = getFrontendConfig(); if (logtype != SLAPD_ACCESS_LOG && @@ -1621,7 +1678,14 @@ log_set_expirationtime(const char *attrname, char *exptime_str, int logtype, cha return rv; } - exptime = atoi(exptime_str); /* <= 0: no exptime */ + errno = 0; + exptime = strtol(exptime_str, &endp, 10); + if (*endp != '\0' || errno == ERANGE || exptime < -1 || exptime == 0) { + slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, + "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than 0", + exptime_str, attrname); + return LDAP_UNWILLING_TO_PERFORM; + } switch (logtype) { case SLAPD_ACCESS_LOG: @@ -1734,7 +1798,6 @@ log_set_expirationtimeunit(const char *attrname, char *expunit, int logtype, cha } else { slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, "%s: invalid time unit \"%s\"", attrname, expunit); rv = LDAP_OPERATIONS_ERROR; - ; } /* return if we aren't doing this for real */ -- 2.17.2