Hi,
lpd wants to verify that it doesn't open a symbolic link, checking with
lstat(), then open()ing the file. The only reason I can see that the
code does not simply use O_NOFOLLOW is a different return value if
it encounters a symlink (maybe I am wrong here, would like to get feedback
on this assumption).
In either way, an attacker could create a regular file, waiting for the
lstat() to happen and replace it with a symlink right after the call,
before the open() function is called.
I suggest to skip lstat() and check the fstat() result later on,
continuing the train of thought of current lpd behaviour -- lpd will
just _always_ do the file check.
Also, don't assume that everything is alright if fstat() fails.
Tobias
Index: printjob.c
===================================================================
RCS file: /var/www/cvs/src/usr.sbin/lpr/lpd/printjob.c,v
retrieving revision 1.49
diff -u -p -r1.49 printjob.c
--- printjob.c 10 Dec 2013 16:38:04 -0000 1.49
+++ printjob.c 17 Jan 2014 20:35:01 -0000
@@ -539,17 +539,16 @@ print(int format, char *file)
int n, fi, fo, p[2], stopped = 0, nofile;
PRIV_START;
- if (lstat(file, &stb) < 0 || (fi = safe_open(file, O_RDONLY, 0)) < 0) {
+ if ((fi = safe_open(file, O_RDONLY, 0)) < 0) {
PRIV_END;
return(ERROR);
}
PRIV_END;
/*
- * Check to see if data file is a symbolic link. If so, it should
- * still point to the same file or someone is trying to print
- * something he shouldn't.
+ * Check if expected file was opened. If not, someone is
+ * trying to print something he shouldn't.
*/
- if (S_ISLNK(stb.st_mode) && fstat(fi, &stb) == 0 &&
+ if (fstat(fi, &stb) != 0 ||
(stb.st_dev != fdev || stb.st_ino != fino))
return(ACCESS);
if (!SF && !tof) { /* start on a fresh page */
@@ -876,18 +875,16 @@ sendfile(int type, char *file)
int sizerr, resp;
PRIV_START;
- if (lstat(file, &stb) < 0 || (f = safe_open(file, O_RDONLY, 0)) < 0) {
+ if ((f = safe_open(file, O_RDONLY, 0)) < 0) {
PRIV_END;
return(ERROR);
}
PRIV_END;
/*
- * Check to see if data file is a symbolic link. If so, it should
- * still point to the same file or someone is trying to print something
- * he shouldn't.
+ * Check if expected file was opened. If not, someone is
+ * trying to print something he shouldn't.
*/
- if (S_ISLNK(stb.st_mode) && fstat(f, &stb) == 0 &&
- (stb.st_dev != fdev || stb.st_ino != fino))
+ if (fstat(f, &stb) != 0 || (stb.st_dev != fdev || stb.st_ino != fino))
return(ACCESS);
if ((amt = snprintf(buf, sizeof(buf), "%c%lld %s\n", type,
(long long)stb.st_size, file)) >= sizeof(buf) || amt == -1)
