[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20171109152412.qmh4gw62lcrd5a4h@jwilk.net>
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
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.
