crontab: don't reject non-string-like shell commands (#47)

* crontab: don't reject non-string-like shell commands

The use of get_string to parse and assign shell commands to jobs
requires that shell commands starting with quotes also terminate with
quotes, like

    * * * * * "/root/my script"

This improperly rejects commands generated by Git maintenance, which
look like (just one example)

    32 1-23 * * * "/usr/libexec/git-core/git" --exec-path="/usr/libexec/git-core" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly

Simpler commands with internal quotes are of course unaffected.

But why are we interpreting shell syntax anyway? That requires a fairly
complex parser.

Instead, do what the documentation says and take the rest of the
(logical) line as the shell command to execute, leaving it to the
shell's parser to interpret the commands. In case we want to later add
some validation of our own, hide this new parsing behind the abstraction
"get_command". Keep removing blanks, since they should be ignored by the
shell anyway.

While at it, add (some) test coverage for the various read_* functions
that parse job lines; these tests are only interested in the shell
parsing for now. Note that the compilation recipe for these tests is a
bit different than other recipes, since it builds based on fcrontab
(where fileconf.o is used). We also have to expose the functions to test
via the header.

Fix: https://github.com/yo8192/fcron/issues/46

* Fixed compilation of parse_job_test, applied indentation, and improved function descriptions.

---------

Co-authored-by: Thibault Godouet <yo8192@users.noreply.github.com>

diff --git a/Makefile.in b/Makefile.in
index 1f691763a7314b6cc3caef0db9553b0575fafcce..fefd090ecb5a205102b7dd48005640c5c2d1d590 100644 (file)
--- a/Makefile.in
+++ b/Makefile.in
@@ -85,7 +85,7 @@ OBJCONV       := convert-fcrontab.o cl.o subs.o mem.o save.o log.o u_list.o env_
 OBJSIG        := fcronsighup.o subs.o mem.o log.o allow.o fcronconf.o mail.o filesubs.o
 HEADERSALL    := config.h $(SRCDIR)/global.h $(SRCDIR)/cl.h $(SRCDIR)/log.h $(SRCDIR)/subs.h $(SRCDIR)/mem.h $(SRCDIR)/save.h $(SRCDIR)/option.h $(SRCDIR)/dyncom.h $(SRCDIR)/mail.h
 TESTS_DIR     := tests
-TESTS         := $(addprefix ${TESTS_DIR}/,fcrondyn_svr_test exe_list_test mail_test)
+TESTS         := $(addprefix ${TESTS_DIR}/,fcrondyn_svr_test exe_list_test mail_test parse_job_test)
 
 # this is a regular expression :
 # do not ci automaticaly generated files and doc (done by doc's Makefile)
@@ -153,6 +153,10 @@ $(TESTS_DIR)/mail_test: $(OBJSD_NO_MAIN) $(TESTS_DIR)/fcron.test.o $(TESTS_DIR)/
        $(CC) $(CFLAGS) -o $@  $^ $(LIBS) -lcmocka -Wno-implicit-function-declaration
        $@
 
+$(TESTS_DIR)/parse_job_test: $(filter-out fcrontab.o,$(OBJSTAB)) $(TESTS_DIR)/fcrontab.test.o $(TESTS_DIR)/parse_job_test.o
+       $(CC) $(CFLAGS) -o $@  $^ $(LIBS) -lcmocka -Wno-implicit-function-declaration
+       $@
+
 tests: $(TESTS)
 
 initscripts:

diff --git a/fileconf.c b/fileconf.c
index 1fc7549d0817c8b06df700176dc73531ab635940..f6855a3071a9c9e38f2cd53660d989925deac243 100644 (file)
--- a/fileconf.c
+++ b/fileconf.c
@@ -27,6 +27,7 @@
 #include "fileconf.h"
 
 char *get_string(char *ptr);
+char *get_command(char *ptr);
 int get_line(char *str, size_t size, FILE * file);
 void init_default_line(cl_t * cl, cf_t * cf);
 char *get_time(char *ptr, time_t * time, int zero_allowed);
@@ -67,9 +68,9 @@ const char *mons_ary[] = {
 
 char *
 get_string(char *ptr)
-    /* read string pointed by ptr, remove blanks and manage
-     * string placed in quotes */
-    /* return NULL on mismatched quotes */
+    /* Read string pointed by ptr, remove blanks and starting/ending quotes. */
+    /* ptr must not be NULL, and a string starting by quote must end with it too. */
+    /* Return a pointer to a new string, or NULL on mismatched quotes. */
 {
     char quote = 0;
     int length = 0;
@@ -97,6 +98,16 @@ get_string(char *ptr)
 
 }
 
+char *
+get_command(char *ptr)
+    /* Read a shell command from ptr to the end of the (logical) line,
+     * removing trailing blanks. */
+    /* ptr must not be NULL. */
+    /* Return a pointer to a new string. */
+{
+    remove_blanks(ptr);
+    return strdup2(ptr);
+}
 
 int
 get_line(char *str, size_t size, FILE *file)
@@ -1438,12 +1449,8 @@ read_shortcut(char *ptr, cf_t *cf)
     /* check for inline runas */
     ptr = check_username(ptr, cf, cl);
 
-    /* get cl_shell field ( remove trailing blanks ) */
-    if ((cl->cl_shell = get_string(ptr)) == NULL) {
-        fprintf(stderr, "%s:%d: Mismatched quotes: skipping line.\n",
-                file_name, line);
-        goto exiterr;
-    }
+    /* get cl_shell field */
+    cl->cl_shell = get_command(ptr);
     if (strcmp(cl->cl_shell, "\0") == 0) {
         fprintf(stderr, "%s:%d: No shell command: skipping line.\n",
                 file_name, line);
@@ -1526,12 +1533,8 @@ read_freq(char *ptr, cf_t *cf)
     /* check for inline runas */
     ptr = check_username(ptr, cf, cl);
 
-    /* get cl_shell field ( remove trailing blanks ) */
-    if ((cl->cl_shell = get_string(ptr)) == NULL) {
-        fprintf(stderr, "%s:%d: Mismatched quotes: skipping line.\n",
-                file_name, line);
-        goto exiterr;
-    }
+    /* get cl_shell field */
+    cl->cl_shell = get_command(ptr);
     if (strcmp(cl->cl_shell, "\0") == 0) {
         fprintf(stderr, "%s:%d: No shell command: skipping line.\n",
                 file_name, line);
@@ -1630,12 +1633,8 @@ read_arys(char *ptr, cf_t *cf)
     /* check for inline runas */
     ptr = check_username(ptr, cf, cl);
 
-    /* get the shell command (remove trailing blanks) */
-    if ((cl->cl_shell = get_string(ptr)) == NULL) {
-        fprintf(stderr, "%s:%d: Mismatched quotes: skipping line.\n",
-                file_name, line);
-        goto exiterr;
-    }
+    /* get the shell command */
+    cl->cl_shell = get_command(ptr);
     if (strcmp(cl->cl_shell, "\0") == 0) {
         fprintf(stderr, "%s:%d: No shell command: skipping line.\n",
                 file_name, line);
@@ -1734,12 +1733,8 @@ read_period(char *ptr, cf_t *cf)
     /* check for inline runas */
     ptr = check_username(ptr, cf, cl);
 
-    /* get the shell command (remove trailing blanks) */
-    if ((cl->cl_shell = get_string(ptr)) == NULL) {
-        fprintf(stderr, "%s:%d: Mismatched quotes: skipping line.\n",
-                file_name, line);
-        goto exiterr;
-    }
+    /* get the shell command */
+    cl->cl_shell = get_command(ptr);
     if (strcmp(cl->cl_shell, "\0") == 0) {
         fprintf(stderr, "%s:%d: No shell command: skipping line.\n",
                 file_name, line);

diff --git a/fileconf.h b/fileconf.h
index 71fd0833f7ec285c42dd15c91271bed67f22a77d..13eef5f771c23677db14e1a7082f4740951cf284 100644 (file)
--- a/fileconf.h
+++ b/fileconf.h
@@ -21,7 +21,6 @@
  *  `LICENSE' that comes with the fcron source distribution.
  */
 
-
 #ifndef __FILECONF_H__
 #define __FILECONF_H__
 
@@ -30,4 +29,10 @@ extern int read_file(char *filename, int fd);
 extern void delete_file(const char *user_name);
 extern int save_file(char *path);
 
+/* for tests */
+int read_shortcut(char *, cf_t *);
+void read_arys(char *, cf_t *);
+void read_freq(char *, cf_t *);
+void read_period(char *, cf_t *);
+
 #endif                          /* __FILECONF_H__ */

diff --git a/subs.c b/subs.c
index c021661c7d3e8287b5a686fe71436f0a87b0a53c..9a0a00b009adf6631a5aceb6a434ed448e73a188 100644 (file)
--- a/subs.c
+++ b/subs.c
@@ -313,8 +313,9 @@ rename_as_user(const char *oldpath, const char *newpath, uid_t renameuid,
 
 int
 remove_blanks(char *str)
-    /* remove blanks at the the end of str */
-    /* return the length of the new string */
+    /* Remove blanks at the the end of str by NULL-terminating the string before them. */
+    /* The pointer str must not be NULL. */
+    /* Return the length of the shortened string. */
 {
     char *c = str;
 

diff --git a/tests/parse_job_test.c b/tests/parse_job_test.c
new file mode 100644 (file)
index 0000000..d8be1b8
--- /dev/null
+++ b/tests/parse_job_test.c
@@ -0,0 +1,100 @@
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>             /* setjmp.h is needed by cmocka.h */
+#include <cmocka.h>
+
+#include "../fcrontab.h"
+#include "../mem.h"
+#include "../fileconf.h"
+
+
+#define TestParser(PARSER, DESC, LINE, EXPECTED_COMMAND) \
+{ \
+    cf_t cf; \
+    char *input = strdup2(LINE); \
+    PARSER(input, &cf); \
+    assert_string_equal(cf.cf_line_base->cl_shell, EXPECTED_COMMAND); \
+    Free_safe(input); \
+}
+
+#define TestArysParser(D, L, E) TestParser(read_arys, D, L, E)
+#define TestPeriodParser(D, L, E) TestParser(read_period, D, L, E)
+#define TestFreqParser(D, L, E) TestParser(read_freq, D, L, E)
+#define TestShortcutParser(D, L, E) TestParser(read_shortcut, D, L, E)
+
+static void
+test_read_arys_shell_parser(void **state)
+{
+    TestArysParser("example 1",
+                   "& 05,35 12-14 * * * mycommand -u me -o file ",
+                   "mycommand -u me -o file");
+    TestArysParser("quoted command",
+                   "* * * * * \"/root/my script\"", "\"/root/my script\"");
+    TestArysParser("Hourly Git Maintenance",
+                   "32 1-23 * * * \"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly",
+                   "\"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly");
+    TestArysParser("Daily Git Maintenance",
+                   "32 0 * * 1-6 \"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=daily",
+                   "\"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=daily");
+    TestArysParser("Weekly Git Maintenance",
+                   "32 0 * * 0 \"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=weekly",
+                   "\"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=weekly");
+}
+
+static void
+test_read_freq_shell_parser(void **state)
+{
+    TestFreqParser("example 1", "@ 30 getmails -all", "getmails -all");
+    TestFreqParser("example 2",
+                   "@mailto(root),forcemail 2d /etc/security/msec/cron-sh/security.sh",
+                   "/etc/security/msec/cron-sh/security.sh");
+    TestFreqParser("quoted command",
+                   "@ 12h02 \"/root/my script\"", "\"/root/my script\"");
+    TestFreqParser("internal quotes", "@ 3d echo \"hi\"", "echo \"hi\"");
+    TestFreqParser("blanks", "@ 3w2d5h1 true  ", "true");
+}
+
+static void
+test_read_period_shell_parser(void **state)
+{
+    TestPeriodParser("example 1",
+                     "%nightly,mail(no) * 21-23,3-5 echo \"a nigthly entry\"",
+                     "echo \"a nigthly entry\"");
+    TestPeriodParser("example 2",
+                     "%hours * 0-22 * * * echo \"Ok.\"", "echo \"Ok.\"");
+    TestPeriodParser("quoted command",
+                     "%hourly 31 \"/root/my script\"", "\"/root/my script\"");
+    TestPeriodParser("internal quotes",
+                     "%middaily 21 7-10 echo \"hi\"", "echo \"hi\"");
+    TestPeriodParser("blanks", "%monthly 59 4 12 true  ", "true");
+}
+
+static void
+test_read_shortcut_shell_parser(void **state)
+{
+    TestShortcutParser("example 1",
+                       "@hourly check_laptop_logs.sh", "check_laptop_logs.sh");
+    TestShortcutParser("example 2",
+                       "@daily check_web_server.sh", "check_web_server.sh");
+    TestShortcutParser("example 3",
+                       "@daily check_file_server.sh", "check_file_server.sh");
+    TestShortcutParser("example 4",
+                       "@monthly compress_home_made_app_log_files.sh",
+                       "compress_home_made_app_log_files.sh");
+    TestShortcutParser("quoted command",
+                       "@weekly \"/root/my script\"", "\"/root/my script\"");
+    TestShortcutParser("internal quotes", "@reboot echo \"hi\"", "echo \"hi\"");
+    TestShortcutParser("quoted command", "@yearly true   ", "true");
+}
+
+int
+main(void)
+{
+    const struct CMUnitTest tests[] = {
+        cmocka_unit_test(test_read_arys_shell_parser),
+        cmocka_unit_test(test_read_freq_shell_parser),
+        cmocka_unit_test(test_read_period_shell_parser),
+        cmocka_unit_test(test_read_shortcut_shell_parser),
+    };
+    return cmocka_run_group_tests(tests, NULL, NULL);
+}