Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 9 Nov 2017 16:24:12 +0100
From: Jakub Wilk <jwilk@...lk.net>
To: oss-security@...ts.openwall.com
Subject: Re: nvi denial of service

Instead of applying more and more duct tape on nvi's broken design, you 
should stop abusing /var/tmp and store swapfiles in user's home 
directory instead; and drop the virecorver script completely.

* coypu@....org, 2017-11-09, 02:32:
> /*
>+ * Since vi creates recovery files only accessible by the user, files
>+ * accessible by group or others are probably malicious so avoid them.
>+ * This is simpler than checking for getuid() == st.st_uid and we want
>+ * to preserve the functionality that root can recover anything which
>+ * means that root should know better and be careful.
>+ */
>+static int
>+checkok(int fd)
>+{
>+	struct stat sb;
>+
>+	return fstat(fd, &sb) != -1 && S_ISREG(sb.st_mode) &&
>+	    (sb.st_mode & (S_IRWXG|S_IRWXO)) == 0;

Clever, but racy. Between the open() and fstat() calls, the owner could 
change permissions to make this test pass.

>-		if ((fp = fopen(dp->d_name, "r+")) == NULL)
>+		if ((fp = fopen(dp->d_name, "r+efl")) == NULL)

These are non-standard modifiers:
"e" opens the file with O_CLOEXEC;
"l" opens the file with O_NOFOLLOW;
"f" opens only regular files.

("e" is available in glibc; the others are not.)

AFAICS, implementation of the "f" modifier in NetBSD is not atomic: it 
opens the file, then closes it if it's not a regular file.

>-		if ((fd = open(recpath, O_RDWR, 0)) == -1)
>+		if ((fd = open(recpath, O_RDWR|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC,

I believe that even with O_NONBLOCK, opening a non-regular file can have 
side effects.

>+for i in $RECDIR/vi.*; do
>+
>+	case "$i" in
>+	$RECDIR/vi.\*) continue;;
>+	esac

(Not related to security, but this check for non-expanded wildcard is 
not needed, because the code skips non-existent files later anyway.)

>+for i in $RECDIR/recover.*; do
>+
>+	case "$i" in
>+	$RECDIR/recover.\*) continue;;
>+	esac

(Ditto.)

>+	# Delete any recovery files that are zero length, corrupted,
>+	# or that have no corresponding backup file.  Else send mail
>+	# to the user.
>+	recfile=$(awk '/^X-vi-recover-path:/{print $2}' < "$i")

It's still happy to read any file as root. (So it's trivial for a local 
user to make the script hang forever.)

>+	if [ -n "$recfile" ] && [ -s "$recfile" ]; then
>+		$SENDMAIL -t < "$i"

It's still happy to send email arbitrary emails (including non-local 
recipients) as root.

-- 
Jakub Wilk

Powered by blists - more mailing lists

Your e-mail address:

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Powered by Openwall GNU/*/Linux - Powered by OpenVZ