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.

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.