From 2d014758a2944ee9fbf18e7d06cf60c3501e80e0 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Mon, 12 Oct 2026 23:13:30 +0200 Subject: [PATCH] Fix Cron job input validation This patch fixes unsafe validation of Cron job fields before crontab entries are created. It rejects line breaks and invalid schedule, special-time, and date values that could add unexpected fields, commands, or environment variables. It also adds tests for malicious inputs and valid schedules. Additional improvements to accompany. https://github.com/webmin/webmin/commit/c23b6b5b484a644ef9ed5e55a84f0bb8607ecd59. --- cron/cron-lib.pl | 12 ++++++++ cron/lang/en | 3 ++ cron/save_cron.cgi | 4 +++ cron/t/run-tests.t | 74 +++++++++++++++++++++++++++++++++++++++++----- 4 files changed, 86 insertions(+), 7 deletions(-) diff --git a/cron/cron-lib.pl b/cron/cron-lib.pl index 38b91fdce..1587b698f 100755 --- a/cron/cron-lib.pl +++ b/cron/cron-lib.pl @@ -1079,6 +1079,8 @@ if ($in{'special_def'}) { foreach $arr (@pers) { delete($job->{$arr}); } + # Special times are single words like daily or reboot + $in{'special'} =~ /^[a-z]+$/i || &error($text{'save_especial'}); $job->{'special'} = $in{'special'}; } else { @@ -1092,6 +1094,11 @@ else { # Need to work out and simplify ranges selected local (@range, @newrange, $i); @range = split(/\0/, $in{$arr}); + # Selected times are plain numbers + foreach my $v (@range) { + $v =~ /^\d+$/ || &error(&text('save_etime', + $text{"edit_$arr"})); + } @range = sort { $a <=> $b } @range; local $start = -1; for($i=0; $i<@range; $i++) { @@ -1168,6 +1175,11 @@ if ($in->{'range_def'}) { else { # Validate and store range foreach my $r ("start", "end") { + # Dates go into the job's command, so only allow numbers + foreach my $f ("day", "month", "year") { + $in->{'range_'.$r.'_'.$f} =~ /^\d+$/ || + &error($text{'range_e'.$r}); + } eval { timelocal(0, 0, 0, $in->{'range_'.$r.'_day'}, $in->{'range_'.$r.'_month'}-1, $in->{'range_'.$r.'_year'}-1900) }; diff --git a/cron/lang/en b/cron/lang/en index 0141128a1..a51b1cc49 100644 --- a/cron/lang/en +++ b/cron/lang/en @@ -81,6 +81,9 @@ lcedit_special_reboot=when system boots save_err=Failed to save cron job save_ecmd=You did not enter a command to execute +save_enewline=The command and comment cannot contain line breaks +save_especial=Invalid special time selected +save_etime=Invalid $1 selected save_ename=Missing or invalid environment variable name save_evalue=Invalid environment variable value save_euser=You must select a user diff --git a/cron/save_cron.cgi b/cron/save_cron.cgi index 821156871..6a8edb651 100755 --- a/cron/save_cron.cgi +++ b/cron/save_cron.cgi @@ -38,6 +38,10 @@ foreach $f (@files) { &lock_file($f); } if ($in{"cmd"} !~ /\S/ && $access{'command'}) { &error($text{'save_ecmd'}); } +# A line break would add extra lines to the crontab +if ($in{'cmd'} =~ /[\r\n]/ || $in{'comment'} =~ /[\r\n]/) { + &error($text{'save_enewline'}); + } if (&supports_users()) { if (!$in{'user'}) { &error($text{'save_euser'}); diff --git a/cron/t/run-tests.t b/cron/t/run-tests.t index 3a2e8525b..0c3a68ab1 100644 --- a/cron/t/run-tests.t +++ b/cron/t/run-tests.t @@ -81,15 +81,23 @@ ok(!cron_move_affects_env(\@jobs, $jobs[0], $jobs[6]), my $cronfile = "$vardir/crontab"; my $crondir = "$vardir/crontabs"; make_path($crondir); -open(my $modfh, ">", "$confdir/cron/config") or die "cron config: $!"; -print $modfh "single_file=$cronfile\n"; -print $modfh "cron_dir=$crondir\n"; -print $modfh "system_crontab=$vardir/system-crontab\n"; -print $modfh "cronfiles_dir=$vardir/cron.d\n"; -print $modfh "vixie_cron=1\n"; -close($modfh); make_path("$vardir/cron.d"); +# Write the Cron module config, with the given lines added. +sub write_cron_config +{ +my (@lines) = @_; +open(my $fh, ">", "$confdir/cron/config") or die "cron config: $!"; +print $fh map { "$_\n" } (@lines, + "cron_dir=$crondir", + "system_crontab=$vardir/system-crontab", + "cronfiles_dir=$vardir/cron.d", + "vixie_cron=1", + "cron_deny_all=2"); +close($fh); +} +write_cron_config("single_file=$cronfile"); + # Reset the crontab fixture to its original contents. sub write_crontab { @@ -246,6 +254,39 @@ like($result->{'out'}, qr/^Location:/m, is(read_crontab_fixture(), $original, "re-enabling the normal job restores the fixture"); +# Job fields must not add lines or fields to the crontab +seed_acl(%full_acl); +my %edit = ( 'idx' => 0, 'user' => 'root', 'active' => 1, + 'cmd' => 'echo first', 'range_def' => 1, + 'special_def' => 0, 'all_mins' => 1, 'all_hours' => 1, + 'all_days' => 1, 'all_months' => 1, 'all_weekdays' => 1 ); +foreach my $case ( + [ "a line break in the command", + { 'cmd' => "echo first\nPATH=/tmp" }, qr/cannot contain line breaks/ ], + [ "a line break in the comment", + { 'comment' => "x\nPATH=/tmp" }, qr/cannot contain line breaks/ ], + [ "a special time with spaces", + { 'special_def' => 1, 'special' => "reboot echo x #" }, + qr/Invalid special time/ ], + [ "a minute with spaces", + { 'all_mins' => 0, 'mins' => "* * * * * echo x #" }, + qr/Invalid Minutes selected/ ], + [ "a minute with a line break", + { 'all_mins' => 0, 'mins' => "5\nPATH=/tmp" }, + qr/Invalid Minutes selected/ ], + [ "a range date with shell text", + { 'range_def' => 0, 'range_start_day' => '1;echo x;', + 'range_start_month' => 1, 'range_start_year' => 2026, + 'range_end_day' => 31, 'range_end_month' => 12, + 'range_end_year' => 2026 }, qr/invalid start date/i ], + ) { + my ($desc, $form, $re) = @$case; + $result = run_cgi("save_cron.cgi", { %edit, %$form }); + like($result->{'out'}, $re, "save_cron.cgi rejects $desc"); + is(read_crontab_fixture(), $original, + "rejected $desc leaves the crontab unchanged"); + } + seed_acl(%full_acl); $result = run_cgi("move.cgi", { 'idx' => 4, 'top' => 1 }); like($result->{'out'}, qr/^Location:/m, @@ -253,4 +294,23 @@ like($result->{'out'}, qr/^Location:/m, isnt(read_crontab_fixture(), $original, "the permitted top move changes the crontab"); +# Saving a job needs a real Unix user, so check a valid save in a per-user +# crontab, as on Linux. Plain shell commands stand in for the crontab +# command, so the real one never runs. +write_cron_config("cron_copy_command=cat >$crondir/USER", + "cron_delete_command=rm -f $crondir/USER"); +$cronfile = "$crondir/root"; +write_crontab(); +$result = run_cgi("save_cron.cgi", + { %edit, 'all_mins' => 0, 'mins' => [ 5, 6, 30 ], + 'range_def' => 0, 'range_start_day' => 1, + 'range_start_month' => 1, 'range_start_year' => 2026, + 'range_end_day' => 31, 'range_end_month' => 12, + 'range_end_year' => 2026 }); +like($result->{'out'}, qr/^Location:/m, + "save_cron.cgi accepts valid times and dates"); +like(read_crontab_fixture(), + qr/^5-6,30 \* \* \* \* \S+range\.pl 1-1-2026 31-12-2026 echo\\ first$/m, + "the saved job has the expected times and date range"); + done_testing();