entry.c:396 load_entry() info: redundant null check on e->pwd calling free() entry.c:398 load_entry() info: redundant null check on e->cmd calling free() /usr/include/bits/fcntl.h:48:10: warning: preprocessor token O_NOFOLLOW redefined macros.h:136:9: this was the original definition security.c:98:44: warning: non-ANSI function declaration of function 'cron_restore_default_security_context' crontab.c:439:29: warning: non-ANSI function declaration of function 'tmp_path' Signed-off-by: Sami Kerola <kerolasa(a)iki.fi&gt; --- src/crontab.c | 2 +- src/entry.c | 6 ++---- src/macros.h | 2 ++ src/security.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/crontab.c b/src/crontab.c index bfff182..5703a3b 100644 --- a/src/crontab.c +++ b/src/crontab.c @@ -436,7 +436,7 @@ static void check_error(const char *msg) { fprintf(stderr, "\"%s\":%d: %s\n", Filename, LineNumber - 1, msg); } -static const char *tmp_path() { +static const char *tmp_path(void) { const char *tmpdir = NULL; if ((getuid() == geteuid()) && (getgid() == getegid())) { diff --git a/src/entry.c b/src/entry.c index 0b482b9..e9142f5 100644 --- a/src/entry.c +++ b/src/entry.c @@ -392,10 +392,8 @@ entry *load_entry(FILE * file, void (*error_func) (), struct passwd *pw, eof: if (e->envp) env_free(e->envp); - if (e->pwd) - free(e->pwd); - if (e->cmd) - free(e->cmd); + free(e->pwd); + free(e->cmd); free(e); while (ch != '\n' && !feof(file)) ch = get_char(file); diff --git a/src/macros.h b/src/macros.h index 215753a..6c7ca6d 100644 --- a/src/macros.h +++ b/src/macros.h @@ -25,6 +25,7 @@ #ifdef HAVE_LIMITS_H #include <limits.h> #endif + /* these are really immutable, and are * defined for symbolic convenience only * TRUE, FALSE, and ERR must be distinct @@ -131,6 +132,7 @@ * we will just have to live without it. In order for this to be an * issue an attacker would have to subvert group CRON_GROUP. */ +#include <fcntl.h> #ifndef O_NOFOLLOW #define O_NOFOLLOW 0 #endif diff --git a/src/security.c b/src/security.c index b483134..c5fbc5e 100644 --- a/src/security.c +++ b/src/security.c @@ -100,7 +100,7 @@ static int cron_get_job_range(user * u, security_context_t * ucontextp, char **jobenv); #endif -void cron_restore_default_security_context() { +void cron_restore_default_security_context(void) { #ifdef WITH_SELINUX setexeccon(NULL); #endif -- 1.8.1.1

The commit makes the random be more chaotic than it used to be. This change also makes the instance in readtab.c where random() is used a little bit easier to read. Signed-off-by: Sami Kerola <kerolasa(a)iki.fi&gt; --- anacron/main.c | 25 +++++++++++++++++++------ anacron/readtab.c | 15 ++++++++------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/anacron/main.c b/anacron/main.c index 7889983..88afaca 100644 --- a/anacron/main.c +++ b/anacron/main.c @@ -431,23 +431,36 @@ explain_intentions(void) explain("Jobs will be executed sequentially"); } +/* Seed random(3). */ +static void +srandomdev(void) { + struct timeval tv; + unsigned int seed; + FILE *fd; + + if ((fd = fopen("/dev/urandom", "r"))) { + fread(&seed, sizeof seed, 1, fd); + fclose(fd); + } else { + gettimeofday(&tv, NULL); + /* NOTE: intentional use of uninitialized variable */ + seed ^= (getpid() << 16) ^ tv.tv_sec ^ tv.tv_usec; + } + srandom(seed); +} + int main(int argc, char *argv[]) { int j; int cwd; - struct timeval tv; - struct timezone tz; anacrontab = NULL; spooldir = NULL; setlocale(LC_ALL, ""); - if (gettimeofday(&tv, &tz) != 0) - explain("Can't get exact time, failure."); - - srandom(getpid()+tv.tv_usec); + srandomdev(); if((program_name = strrchr(argv[0], '/')) == NULL) program_name = argv[0]; diff --git a/anacron/readtab.c b/anacron/readtab.c index e378faf..2d403e4 100644 --- a/anacron/readtab.c +++ b/anacron/readtab.c @@ -281,13 +281,14 @@ parse_tab_line(char *line) Debug(("Jobs will start in the %02d:00-%02d:00 range.", range_start, range_stop)); } if (strncmp(env_var, "RANDOM_DELAY", 12) == 0) { - r = match_rx("^([[:digit:]]+)$", value, 0); - if (r != -1) { - int i = random(); - double x = 0; - x = (double) i / (double) RAND_MAX * (double) (atoi(value)); - random_number = (int)x; - Debug(("Randomized delay set: %d", random_number)); + r = match_rx("^([[:digit:]]+)$", value, 0); + if (r != -1) { + int divisor = atoi(value); + random_number = random(); + /* The random_number is expected vary in between 0 and + * 'value', so divisor need to be value + 1. */ + random_number %= (divisor + 1); + Debug(("Randomized delay set: %d", random_number)); } else goto reg_invalid; } -- 1.8.1.1

These inline functions will allow hostname lenght to vary per system, which is significant because on some sysctl-based systems, gethostname() doesn't write anything if the output buffer is too small. While MAXHOSTNAMELEN is a good guess how long the name might be it is best to try to get the value from sysconf(), and fallback to values provided in headers when sysconf() does not work. Reference: http://cr.yp.to/docs/unixport.html Reference: http://h21007.www2.hp.com/portal/download/files/unprot/stk/solaris_stk/im... Reference: http://refspecs.linuxfoundation.org/LSB_4.0.0/LSB-Core-generic/LSB-Core-g... Signed-off-by: Sami Kerola <kerolasa(a)iki.fi&gt; --- cronie_common.h | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/cronie_common.h b/cronie_common.h index 4ce172e..87699af 100644 --- a/cronie_common.h +++ b/cronie_common.h @@ -20,6 +20,8 @@ #ifndef CRONIE_COMMON_H #define CRONIE_COMMON_H +#include <stdlib.h> + #ifndef __attribute__ # if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 8) # define __attribute__(x) /* empty */ @@ -34,4 +36,40 @@ # define ATTRIBUTE_UNUSED __attribute__ ((__unused__)) #endif +static inline size_t get_hostname_max(void) { + size_t len = sysconf(_SC_HOST_NAME_MAX); + + if (0 < len) + return len; +#ifdef MAXHOSTNAMELEN + return MAXHOSTNAMELEN; +#elif HOST_NAME_MAX + return HOST_NAME_MAX; +#endif + return 64; +} + +static inline char *alloc_hostname_array(void) { + char *array; + size_t sz = get_hostname_max() + 1; + + array = malloc(sizeof (char) * sz); + while (!array) { + usleep(1000); + array = malloc(sizeof (char) * sz); + } + return array; +} + +static inline char *xgethostname(void) { + char *name = alloc_hostname_array(); + size_t sz = get_hostname_max(); + + if (gethostname(name, sz) != 0) + return NULL; + + name[sz - 1] = '\0'; + return name; +} + #endif /* CRONIE_COMMON_H */ -- 1.8.1.1

This commit fixes couple corner case buffer overflows at few locations where earlier a hostname[MAXHOSTNAMELEN] array had been defined exactly to maximum lenght of a hostname. If a host is using maxium hostname length the string will not be null terminated. Reference: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=ker... Signed-off-by: Sami Kerola <kerolasa(a)iki.fi&gt; --- anacron/runjob.c | 8 ++++---- src/crontab.c | 25 +++++++++++++++---------- src/database.c | 10 +++++++--- src/do_command.c | 6 ++++-- 4 files changed, 30 insertions(+), 19 deletions(-) diff --git a/anacron/runjob.c b/anacron/runjob.c index 18e16d7..8f72412 100644 --- a/anacron/runjob.c +++ b/anacron/runjob.c @@ -36,6 +36,7 @@ #include "global.h" #include <langinfo.h> +#include "cronie_common.h" static int temp_file(job_rec *jr) @@ -236,13 +237,11 @@ launch_job(job_rec *jr) { pid_t pid; int fd; - char hostname[512]; + char *hostname; char *mailto; /* get hostname */ - if (gethostname(hostname, 512)) { - strcpy (hostname,"unknown machine"); - } + hostname = xgethostname(); setup_env(jr); @@ -276,6 +275,7 @@ launch_job(job_rec *jr) xwrite(fd, "' on "); xwrite(fd, hostname); xwrite(fd, "\n\n"); + free(hostname); jr->mail_header_size = file_size(fd); diff --git a/src/crontab.c b/src/crontab.c index 5703a3b..7a4f4a3 100644 --- a/src/crontab.c +++ b/src/crontab.c @@ -91,7 +91,7 @@ static char *selinux_context = 0; static PID_T Pid; static char User[MAX_UNAME], RealUser[MAX_UNAME]; static char Filename[MAX_FNAME], TempFilename[MAX_FNAME]; -static char Host[MAXHOSTNAMELEN]; +static char *Host; static FILE *NewCrontab; static int CheckErrorCount; static int PromptOnDelete; @@ -327,9 +327,13 @@ static void parse_args(int argc, char *argv[]) { if (Option == opt_hostset && argv[optind] != NULL) { HostSpecified = 1; - if (strlen(argv[optind]) >= sizeof Host) + if (get_hostname_max() < strlen(argv[optind])) usage("hostname too long"); - (void) strcpy(Host, argv[optind]); + Host = strdup(argv[optind]); + if (Host == NULL) { + fprintf(stderr, "host argument strdup failed\n"); + exit(ERROR_EXIT); + } optind++; } @@ -455,10 +459,7 @@ static char *host_specific_filename(const char *filename, int prefix) */ static char safename[MAX_FNAME]; - char hostname[MAXHOSTNAMELEN]; - - if (gethostname(hostname, sizeof hostname) != 0) - return NULL; + char *hostname = xgethostname(); if (prefix) { if (!glue_strings(safename, sizeof safename, hostname, filename, '.')) @@ -909,7 +910,7 @@ static int hostset_cmd(void) { char *safename; if (!HostSpecified) - gethostname(Host, sizeof Host); + Host = xgethostname(); safename = host_specific_filename("tmp.XXXXXXXXXX", 1); if (!safename || !glue_strings(TempFilename, sizeof TempFilename, SPOOL_DIR, @@ -967,6 +968,7 @@ static int hostset_cmd(void) { (void) unlink(TempFilename); TempFilename[0] = '\0'; } + free(Host); return (error); } @@ -984,12 +986,14 @@ static int hostget_cmd(void) { fprintf(stderr, "File %s not found\n", n); else perror(n); - return (-2); + return (-2); } - if (get_string(Host, sizeof Host, f, "\n") == EOF) { + Host = alloc_hostname_array(); + if (get_string(Host, get_hostname_max() + 1, f, "\n") == EOF) { fprintf(stderr, "Error reading from %s\n", n); fclose(f); + free(Host); return (-2); } @@ -999,6 +1003,7 @@ static int hostget_cmd(void) { fflush(stdout); log_it(RealUser, Pid, "GET HOST", Host, 0); + free(Host); return (0); } diff --git a/src/database.c b/src/database.c index 61764d7..30d0f00 100644 --- a/src/database.c +++ b/src/database.c @@ -43,6 +43,7 @@ # include <sys/inotify.h> #endif +#include "cronie_common.h" #include "funcs.h" #include "globals.h" #include "pathnames.h" @@ -255,7 +256,7 @@ cluster_host_is_local(void) char filename[MAXNAMLEN+1]; int is_local; FILE *f; - char hostname[MAXHOSTNAMELEN], myhostname[MAXHOSTNAMELEN]; + char *hostname, *myhostname; if (!EnableClustering) return (1); @@ -275,14 +276,17 @@ cluster_host_is_local(void) is_local = 0; if (glue_strings(filename, sizeof filename, SPOOL_DIR, CRON_HOSTNAME, '/')) { if ((f = fopen(filename, "r"))) { + hostname = alloc_hostname_array(); + myhostname = xgethostname(); - if (EOF != get_string(hostname, MAXHOSTNAMELEN, f, "\n") && - gethostname(myhostname, MAXHOSTNAMELEN) == 0) { + if (EOF != get_string(hostname, get_hostname_max() + 1, f, "\n")) { is_local = (strcmp(myhostname, hostname) == 0); } else { Debug(DLOAD, ("cluster: hostname comparison error\n")); } + free(hostname); + free(myhostname); fclose(f); } else { Debug(DLOAD, ("cluster: file %s not found\n", filename)); diff --git a/src/do_command.c b/src/do_command.c index cb3d9e9..45330a8 100644 --- a/src/do_command.c +++ b/src/do_command.c @@ -31,6 +31,7 @@ #include <sys/wait.h> #include <unistd.h> +#include "cronie_common.h" #include "externs.h" #include "funcs.h" #include "globals.h" @@ -413,12 +414,12 @@ static int child_process(entry * e, char **jobenv) { && strncmp(MailCmd,"off",4) && !SyslogOutput) { char **env; char mailcmd[MAX_COMMAND]; - char hostname[MAXHOSTNAMELEN]; + char *hostname; char *content_type = env_get("CONTENT_TYPE", jobenv), *content_transfer_encoding = env_get("CONTENT_TRANSFER_ENCODING", jobenv); - gethostname(hostname, MAXHOSTNAMELEN); + hostname = xgethostname(); if (MailCmd[0] == '\0') { if (snprintf(mailcmd, sizeof mailcmd, MAILFMT, MAILARG, mailfrom) @@ -439,6 +440,7 @@ static int child_process(entry * e, char **jobenv) { fprintf(mail, "To: %s\n", mailto); fprintf(mail, "Subject: Cron <%s@%s> %s\n", usernm, first_word(hostname, "."), e->cmd); + free(hostname); #ifdef MAIL_DATE fprintf(mail, "Date: %s\n", arpadate(&StartTime)); -- 1.8.1.1