loreb
commented
5 years ago Here's a first, fugly patch to run entries in /etc/cron.d as their own user. Among other things, I used C++ style comments to make them stand out more. Regular entries run as usual, cron.d entries work with the proper user, except for the
failed parsing crontab for user root: PATH="/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"
which is still there.
The biggest problem (other than esthetics) is that debug messages assume that all files in /etc/cron.d run as root.
Also, while unrelated, there's an indentation warning (-Wmisleading-indentation) in database.c, could you look at it? It looks like a genuine bug to me.
diff --git a/database.c b/database.c
index c0cdc11..10d2490 100644
--- a/database.c
+++ b/database.c
@@ -26,7 +26,7 @@ Prototype int ArmJob(CronFile *file, CronLine *line, time_t t1, time_t t2);
Prototype void RunJobs(void);
Prototype int CheckJobs(void);
-void SynchronizeFile(const char *dpath, const char *fname, const char *uname);
+void SynchronizeFile(const char *dpath, const char *fname, const char *uname, int kludge);
void DeleteFile(CronFile **pfile);
char *ParseInterval(int *interval, char *ptr);
char *ParseField(char *userName, char *ary, int modvalue, int offset, int onvalue, const char **names, char *ptr);
@@ -126,11 +126,11 @@ CheckUpdates(const char *dpath, const char *user_override, time_t t1, time_t t2)
fname = strtok_r(buf, " \t\n", &ptok);
if (user_override)
- SynchronizeFile(dpath, fname, user_override);
+ SynchronizeFile(dpath, fname, user_override, 1);
else if (!getpwnam(fname))
printlogf(LOG_WARNING, "ignoring %s/%s (non-existent user)\n", dpath, fname);
else if (*ptok == 0 || *ptok == '\n') {
- SynchronizeFile(dpath, fname, fname);
+ SynchronizeFile(dpath, fname, fname, 0);
ReadTimestamps(fname);
} else {
/* if fname is followed by whitespace, we prod any following jobs */
@@ -221,9 +221,9 @@ SynchronizeDir(const char *dpath, const char *user_override, int initial_scan)
if (strcmp(den->d_name, CRONUPDATE) == 0)
continue;
if (user_override) {
- SynchronizeFile(dpath, den->d_name, user_override);
+ SynchronizeFile(dpath, den->d_name, user_override, 1);
} else if (getpwnam(den->d_name)) {
- SynchronizeFile(dpath, den->d_name, den->d_name);
+ SynchronizeFile(dpath, den->d_name, den->d_name, 0);
} else {
printlogf(LOG_WARNING, "ignoring %s/%s (non-existent user)\n",
dpath, den->d_name);
@@ -308,8 +308,9 @@ ReadTimestamps(const char *user)
}
}
+// kludge is for entries in cron.d, which have an extra field - the username to run as.
void
-SynchronizeFile(const char *dpath, const char *fileName, const char *userName)
+SynchronizeFile(const char *dpath, const char *fileName, const char *userName, int kludge)
{
CronFile **pfile;
CronFile *file;
@@ -631,7 +632,26 @@ SynchronizeFile(const char *dpath, const char *fileName, const char *userName)
/*
* copy command string
*/
- line.cl_Shell = strdup(ptr);
+ if (!kludge) {
+ line.cl_Shell = strdup(ptr);
+ line.cl_Setuidgid = 0; // TODO check that the file is owned by root?
+ } else {
+ // ptr = "username cmd...", possibly with blanks
+ char *cmd;
+ // skip whitespace (if any)
+ ptr += strspn(ptr, " \t\n");
+ cmd = ptr;
+ cmd += strcspn(cmd, " \t\n"); // immediately after user - " cmd..."
+ if(*cmd) {
+ *cmd = '\0';
+ cmd++;
+ }
+ // XXX TODO we happily ignore setting uid to "" atm!
+ // The reason is that the cleanup is complicated - see above.
+ line.cl_Setuidgid = strdup(ptr);
+ line.cl_Shell = strdup(cmd);
+ }
+
if (line.cl_Delay > 0) {
if (!(line.cl_Timestamp = concat(TSDir, "/", userName, ".", line.cl_JobName, NULL))) {
@@ -913,6 +933,7 @@ DeleteFile(CronFile **pfile)
} else {
*pline = line->cl_Next;
free(line->cl_Shell);
+ free(line->cl_Setuidgid);
if (line->cl_JobName)
/* this frees both cl_Description and cl_JobName
@@ -1145,6 +1166,7 @@ TestStartupJobs(void)
t1 = t1 - t1 % 60 + 60;
for (file = FileBase; file; file = file->cf_Next) {
+ // TODO with cron.d, each __line__ has its user!!!
if (DebugOpt)
printlogf(LOG_DEBUG, "TestStartup for FILE %s/%s USER %s:\n",
file->cf_DPath, file->cf_FileName, file->cf_UserName);
diff --git a/defs.h b/defs.h
index cf77b5f..184cdab 100644
--- a/defs.h
+++ b/defs.h
@@ -137,6 +137,7 @@ typedef struct CronFile {
typedef struct CronLine {
struct CronLine *cl_Next;
char *cl_Shell; /* shell command */
+ char *cl_Setuidgid; /* every *line*: override cf_UserName in SCRONTABS */
char *cl_Description; /* either "<cl_Shell>" or "job <cl_JobName>" */
char *cl_JobName; /* job name, if any */
char *cl_Timestamp; /* path to timestamp file, if cl_Freq defined */
diff --git a/job.c b/job.c
index 017ca3d..723ad20 100644
--- a/job.c
+++ b/job.c
@@ -36,7 +36,7 @@ RunJob(CronFile *file, CronLine *line)
line->cl_MailFlag = 1;
/* if we didn't specify a -m Mailto, use the local user */
if (!value)
- value = file->cf_UserName;
+ value = file->cf_UserName; // XXX cron.d
fdprintf(mailFd, "To: %s\nSubject: cron for user %s %s\n\n",
value,
file->cf_UserName,
@@ -69,6 +69,14 @@ RunJob(CronFile *file, CronLine *line)
);
exit(0);
}
+ // change TWICE - printlogf?
+ if (line->cl_Setuidgid && ChangeUser(line->cl_Setuidgid, TempDir) < 0) {
+ printlogf(LOG_ERR, "unable to ChangeUser (user %s %s)\n",
+ line->cl_Setuidgid,
+ line->cl_Description
+ );
+ exit(0);
+ }
/* from this point we are unpriviledged */
hills
$ cat /etc/cron.d/zfs-auto-snapshot
git clone, make, ./crond -f
root also gets an email saying "/bin/sh: 1: root: not found"
This is from a linux distro (voidlinux fyi); I don't know if the dragonflybsd convention is different, but here at least files in /etc/cron.d are supposed to be that way (see eg http://deb.debian.org/debian/pool/main/a/anacron/anacron_2.3-27.debian.tar.xz), ie the first field after the time specification is a username to run the command as, and lines like "foo=bar" mean environment assignment (the one above looks pointless to me, and I'm not sure if any other cron implementation gets confused by the quotes; I'm assuming they are ok since https://bugs.debian.org/cgi-bin/pkgreport.cgi?pkg=zfs-auto-snapshot;dist=unstable doesn't say anything)
If there's any more info or anything you need please do let me know.