On 2019-03-15 15:33, Mike Gabriel wrote:
> On Fr 15 Mär 2019 15:11:11 CET, Christian Kastner wrote:
>> On 2019-03-15 14:52, Mike Gabriel wrote:
>>> Dear maintainer(s),
>>>
>>> The Debian LTS team would like to fix the security issues which are
>>> currently open in the Jessie version of cron:
>>> https://security-tracker.debian.org/tracker/CVE-2019-9704
>>> https://security-tracker.debian.org/tracker/CVE-2019-9705
>>> https://security-tracker.debian.org/tracker/CVE-2019-9706
>>> https://security-tracker.debian.org/tracker/CVE-2017-9525
>>>
>>> For Debian stretch, these issues have been marked as <no-dsa>. However,
>>> for Debian LTS, we would like to get those issues resolved.
>>>
>>> Would you like to take care of this yourself?
>>
>> I had planned to prepare fixes for jessie and stretch, but in
>> discussion with the security team, it was considered to wait
>> until these fixes migrate to buster before we fix stable and
>> oldstable.
>>
>> If you'd rather not wait, I can prepare fixes earlier, but not
>> before Sunday -- I should probably make Tuesday.
>
> Ok. Please send a .debdiff for jessie to this list (and directly to me,
> too, so I don't miss it).
I was able to cherry-pick the relevant changes with only trivial
adaptations needed. The result built fine in a jessie chroot.
debdiff attached.
I've also pushed these proposed changes to a wip/jessie branch on Salsa.
I'll merge them into the official updates/jessie pending your ACK.
> I'll handle the upload and processing and also backport your changes to
> wheezy ELTS then.
Regards,
Christian
diff -u cron-3.0pl1/cron.h cron-3.0pl1/cron.h
--- cron-3.0pl1/cron.h
+++ cron-3.0pl1/cron.h
@@ -82,6 +82,7 @@
#define MAX_COMMAND 1000 /* max length of internally generated
cmd */
#define MAX_TEMPSTR 1000 /* max length of envvar=value\0 strings
*/
#define MAX_ENVSTR MAX_TEMPSTR /* DO NOT change - buffer
overruns otherwise */
+#define MAX_TAB_LINES 1000 /* max length of crontabs */
#define MAX_UNAME 20 /* max length of username, should be
overkill */
#define ROOT_UID 0 /* don't change this, it really must be
root */
#define ROOT_USER "root" /* ditto */
@@ -101,6 +102,7 @@
#define CRON_TAB(u) "%s/%s", SPOOL_DIR, u
#define REG register
#define PPC_NULL ((char **)NULL)
+#define NHEADER_LINES 3
#ifndef MAXHOSTNAMELEN
#define MAXHOSTNAMELEN 64
@@ -126,6 +128,8 @@
;
#endif /* DEBUGGING */
+#define Stringify_(x) #x
+#define Stringify(x) Stringify_(x)
#define MkLower(ch) (isupper(ch) ? tolower(ch) : ch)
#define MkUpper(ch) (islower(ch) ? toupper(ch) : ch)
#define Set_LineNum(ln) {Debug(DPARS|DEXT,("linenum=%d\n",ln)); \
diff -u cron-3.0pl1/crontab.1 cron-3.0pl1/crontab.1
--- cron-3.0pl1/crontab.1
+++ cron-3.0pl1/crontab.1
@@ -133,6 +133,14 @@
/var/spool/cron/crontabs
.fi
.PP
+The files
+.I /etc/cron.allow
+and
+.I /etc/cron.deny
+if, they exist, must be either world-readable, or readable by group
+``crontab''. If they are not, then cron will deny access to all users until the
+permissions are fixed.
+.PP
There is one file for each user's crontab under the /var/spool/cron/crontabs
directory. Users are not allowed to edit the files under that directory
directly to ensure that only users allowed by the system to run periodic tasks
diff -u cron-3.0pl1/crontab.c cron-3.0pl1/crontab.c
--- cron-3.0pl1/crontab.c
+++ cron-3.0pl1/crontab.c
@@ -46,8 +46,6 @@
#endif
-#define NHEADER_LINES 3
-
enum opt_t { opt_unknown, opt_list, opt_delete, opt_edit, opt_replace };
#if DEBUGGING
@@ -886,7 +884,7 @@
*/
Set_LineNum(1 - NHEADER_LINES)
CheckErrorCount = 0; eof = FALSE;
- while (!CheckErrorCount && !eof) {
+ while (!CheckErrorCount && !eof && LineNumber < MAX_TAB_LINES + 2) {
switch (load_env(envstr, tmp)) {
case ERR:
eof = TRUE;
@@ -903,6 +901,13 @@
}
}
+ if (LineNumber >= MAX_TAB_LINES + 2) {
+ fprintf(stderr, "crontab is too long; maximum number of lines "
+ "is %d.\n", MAX_TAB_LINES);
+ fclose(tmp); unlink(tn);
+ return (-1);
+ }
+
if (CheckErrorCount != 0) {
fprintf(stderr, "errors in crontab file, can't install.\n");
fclose(tmp); unlink(tn);
diff -u cron-3.0pl1/database.c cron-3.0pl1/database.c
--- cron-3.0pl1/database.c
+++ cron-3.0pl1/database.c
@@ -66,7 +66,7 @@
DIR *dir;
struct stat statbuf;
struct stat syscron_stat;
- DIR_T *dp;
+ DIR_T *dp;
cron_db new_db;
user *u, *nu;
#ifdef DEBIAN
@@ -595,10 +595,12 @@
/* Allocate an empty crontab with the specified mtime, add it to new DB
*/
if ((u = (user *) malloc(sizeof(user))) == NULL) {
errno = ENOMEM;
+ return;
}
if ((u->name = strdup(fname)) == NULL) {
free(u);
errno = ENOMEM;
+ return;
}
u->mtime = old_mtime;
u->crontab = NULL;
diff -u cron-3.0pl1/debian/NEWS cron-3.0pl1/debian/NEWS
--- cron-3.0pl1/debian/NEWS
+++ cron-3.0pl1/debian/NEWS
@@ -1,3 +1,13 @@
+cron (3.0pl1-127+deb8u2) unstable; urgency=medium
+
+ * As a reasonable protective measure, crontabs are now limited to 1000 lines
+ in length per crontab.
+ The maintainers find it very unlikely that longer crontabs exist; however,
+ if you do have a use case, please file a bug report with a brief rationale,
+ and we will consider raising this limit.
+
+ -- Christian Kastner <c...@debian.org> Sun, 10 Mar 2019 17:44:13 +0100
+
cron (3.0pl1-119) unstable; urgency=low
The semantics of the -L option of the cron daemon have changed: from
diff -u cron-3.0pl1/debian/changelog cron-3.0pl1/debian/changelog
--- cron-3.0pl1/debian/changelog
+++ cron-3.0pl1/debian/changelog
@@ -1,3 +1,29 @@
+cron (3.0pl1-127+deb8u2) jessie-security; urgency=medium
+
+ * SECURITY: Fix bypass of /etc/cron.{allow,deny} on failure to open
+ If these files exist, then they must be readable by the user executing
+ crontab(1). Users will now be denied by default if they aren't.
+ (LP: #1813833)
+ * SECURITY: Fix for possible DoS by use-after-free
+ A user reported a use-after-free condition in the cron daemon, leading to a
+ possible Denial-of-Service scenario by crashing the daemon.
+ (CVE-2019-9706) (Closes: #809167)
+ * SECURITY: DoS: Fix unchecked return of calloc()
+ Florian Weimer discovered that a missing check for the return value of
+ calloc() could crash the daemon, which could be triggered by a very
+ large crontab created by a user. (CVE-2019-9704)
+ * Enforce maximum crontab line count of 1000 to prevent a malicious user
+ from creating an excessivly large crontab. The daemon will log a warning
+ for existing files, and crontab(1) will refuse to create new ones.
+ (CVE-2019-9705)
+ * SECURITY: group crontab to root escalation
+ via postinst as described by Alexander Peslyak (Solar Designer) in
+ http://www.openwall.com/lists/oss-security/2017/06/08/3
+ (CVE-2017-9525)
+ * Add d/NEWS altering to the new 1000 lines limit.
+
+ -- Christian Kastner <c...@debian.org> Sun, 17 Mar 2019 14:12:24 +0100
+
cron (3.0pl1-127+deb8u1) jessie; urgency=medium
* d/cron.service: Use KillMode=process to kill only the daemon.
diff -u cron-3.0pl1/debian/postinst cron-3.0pl1/debian/postinst
--- cron-3.0pl1/debian/postinst
+++ cron-3.0pl1/debian/postinst
@@ -60,8 +60,32 @@
# It has been disabled to suit cron alternative such as bcron.
cd $crondir/crontabs
set +e
- ls -1 | xargs -r -n 1 --replace=xxx chown 'xxx:crontab' 'xxx'
- ls -1 | xargs -r -n 1 chmod 600
+
+ # Iterate over each entry in the spool directory, perform some sanity
+ # checks (see CVE-2017-9525), and chown/chgroup the crontabs
+ for tab_name in *
+ do
+ tab_type=`stat -c '%F' "$tab_name"`
+ tab_links=`stat -c '%h' "$tab_name"`
+ tab_owner=`stat -c '%U' "$tab_name"`
+
+ if [ "$tab_type" != "regular file" -a "$tab_type" != "regular empty
file" ]
+ then
+ echo "Warning: $tab_name is not a regular file!"
+ continue
+ elif [ "$tab_links" -ne 1 ]
+ then
+ echo "Warning: $tab_name has more than one hard link!"
+ continue
+ elif [ "$tab_name" != "$tab_owner" ]
+ then
+ echo "Warning: $tab_name name differs from owner $tab_owner!"
+ continue
+ fi
+
+ chown "$tab_owner:crontab" "$tab_name"
+ chmod 600 "$tab_name"
+ done
set -e
fi
diff -u cron-3.0pl1/entry.c cron-3.0pl1/entry.c
--- cron-3.0pl1/entry.c
+++ cron-3.0pl1/entry.c
@@ -108,6 +108,10 @@
*/
e = (entry *) calloc(sizeof(entry), sizeof(char));
+ if (e == NULL) {
+ log_it("CRON", getpid(), "OOM", "Out of memory parsing
crontab");
+ return NULL;
+ }
if (ch == '@') {
/* all of these should be flagged and load-limited; i.e.,
diff -u cron-3.0pl1/misc.c cron-3.0pl1/misc.c
--- cron-3.0pl1/misc.c
+++ cron-3.0pl1/misc.c
@@ -479,7 +479,23 @@
init = TRUE;
#if defined(ALLOW_FILE) && defined(DENY_FILE)
allow = fopen(ALLOW_FILE, "r");
+ if (allow == NULL) {
+ /* Only if the file does not exist do we ignore the
+ * error. Otherwise, we deny by default.
+ */
+ if (errno != ENOENT) {
+ perror(ALLOW_FILE);
+ return FALSE;
+ }
+ }
deny = fopen(DENY_FILE, "r");
+ if (allow == NULL) {
+ /* See above */
+ if (errno != ENOENT) {
+ perror(DENY_FILE);
+ return FALSE;
+ }
+ }
Debug(DMISC, ("allow/deny enabled, %d/%d\n", !!allow, !!deny))
#else
allow = NULL;
diff -u cron-3.0pl1/user.c cron-3.0pl1/user.c
--- cron-3.0pl1/user.c
+++ cron-3.0pl1/user.c
@@ -240,6 +240,7 @@
/*
* load the crontab
*/
+ Set_LineNum(1)
do {
status = load_env(envstr, file);
switch (status) {
@@ -289,7 +290,19 @@
}
break;
}
- } while (status >= OK);
+ /* When counting lines, ignore the user-hidden header part, and account
+ * for idiosyncrasies of LineNumber manipulation
+ */
+ } while (status >= OK && LineNumber < MAX_TAB_LINES + NHEADER_LINES +
2);
+
+ if (LineNumber >= MAX_TAB_LINES + NHEADER_LINES + 2) {
+ log_it(fname, getpid(), "ERROR", "crontab must not be longer "
+ "than " Stringify(MAX_TAB_LINES) " lines, "
+ "this crontab file will be ignored");
+ free_user(u);
+ u = NULL;
+ goto done;
+ }
done:
env_free(envp);
