oss-sec mailing list archives
Re: nvi denial of service
From: Jakub Wilk <jwilk () jwilk net>
Date: Thu, 9 Nov 2017 16:24:12 +0100
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 () sdf 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
Current thread:
- nvi denial of service coypu (Nov 08)
- Re: nvi denial of service Jakub Wilk (Nov 09)