From: D. Ben Knoble Date: Sun, 22 Mar 2026 16:42:08 +0000 (-0400) Subject: crontab: don't reject non-string-like shell commands (#47) X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=ec25ba288d5d70573e07ec040b287e471ea67d2d;p=thirdparty%2Ffcron.git 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 --- diff --git a/Makefile.in b/Makefile.in index 1f69176..fefd090 100644 --- 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 1fc7549..f6855a3 100644 --- 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 71fd083..13eef5f 100644 --- 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 c021661..9a0a00b 100644 --- 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 index 0000000..d8be1b8 --- /dev/null +++ b/tests/parse_job_test.c @@ -0,0 +1,100 @@ +#include +#include +#include /* setjmp.h is needed by cmocka.h */ +#include + +#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); +}