Blob Blame Raw
From 03695c416f7f8311afbded390f3c0ff3637a10d4 Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
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