Date: Thu, 9 Dec 1999 15:33:20 +0100 From: Andre Albsmeier <andre.albsmeier@mchp.siemens.de> To: Alfred Perlstein <bright@wintelcom.net> Cc: Warner Losh <imp@village.org>, Garance A Drosihn <drosih@rpi.edu>, current@FreeBSD.ORG, stable@FreeBSD.ORG Subject: Re: NO! Re: [PATCHES] Two fixes for lpd/lpc for review and test Message-ID: <19991209153320.A62121@internal> In-Reply-To: <Pine.BSF.4.21.9912071446050.4557-100000@fw.wintelcom.net>; from bright@wintelcom.net on Tue, Dec 07, 1999 at 02:55:37PM -0800 References: <199912072106.OAA44391@harmony.village.org> <Pine.BSF.4.21.9912071446050.4557-100000@fw.wintelcom.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 07-Dec-1999 at 14:55:37 -0800, Alfred Perlstein wrote:
I think the (really big) security hole can be closed by not doing
the chown/chmod commands. I inserted them because I wanted the
file in the spool directory to appear exactly as if lpr would
have copied it.
I am currently running the patch with the chown/chmod removed and
lpd doesn't seem to have any problems with it. The side effect now
is that the file in the spool directory keeps it's permissions.
I don't think that this is a problem because if the file was
set to 666 by the creator before, he doesn't care a lot about
it anyway :-)
What do people think about this? Alfred, Warner ?
For better reference, here is the current patch:
*** lpr.c.ORI Thu Dec 9 15:30:18 1999
--- lpr.c Thu Dec 9 15:30:35 1999
***************
*** 370,375 ****
--- 370,405 ----
}
if (sflag)
printf("%s: %s: not linked, copying instead\n", name, arg);
+ /*
+ * If lpr was invoked with -r we try to move the file to
+ * be printed instead of copying and deleting it later.
+ * This works if the file and lpd's spool directory are
+ * on the same filesystem as it is often the case for files
+ * printed by samba or pcnfsd. In this case, a lot of I/O
+ * and temporary disk space can be avoided. Otherwise, we
+ * will continue normally.
+ */
+ if (f) { /* file should be deleted */
+ seteuid(euid); /* needed for rename() */
+ if (!rename(arg, dfname)) {
+ int i;
+ #if 0
+ chown(dfname, userid, getegid());
+ chmod(dfname, S_IRUSR | S_IWUSR |
+ S_IRGRP | S_IWGRP);
+ #endif
+ seteuid(uid); /* restore old uid */
+ if (format == 'p')
+ card('T', title ? title : arg);
+ for (i = 0; i < ncopies; i++)
+ card(format, &dfname[inchar-2]);
+ card('U', &dfname[inchar-2]);
+ card('N', arg);
+ nact++;
+ continue;
+ }
+ seteuid(uid); /* restore old uid */
+ }
if ((i = open(arg, O_RDONLY)) < 0) {
printf("%s: cannot open %s\n", name, arg);
} else {
> On Tue, 7 Dec 1999, Warner Losh wrote:
>
> > I've been reviewing this patch with someone and I think the last
> > version is ready to commit. I'll take a look at my tree to make
> > sure.
>
> please do not, the patch in PR 11997 introduces a major security flaw.
>
> someone can hardlink to any file and clobber it with a file owned by
> them:
>
> try this:
>
> as root:
>
> # cd /var/tmp ; touch rootfile ; chown root:wheel rootfile ; chmod 600 rootfile
>
> as a user:
>
> % cd /var/tmp ; echo foo > foo
> % lpr -r foo
> sleeping
>
> in another session as user:
>
> % rm foo ; ln rootfile foo
>
> wait a second...
>
> # ls -l rootfile
> -rw-rw---- 3 user daemon 5 Dec 7 13:38 rootfile
> # cat rootfile
> foo
> #
>
> ouch!
>
> -Alfred
>
> use this patch to make the race condition apparrent:
>
>
> Index: usr.sbin/lpr/lpr/lpr.c
> ===================================================================
> RCS file: /home/ncvs/src/usr.sbin/lpr/lpr/lpr.c,v
> retrieving revision 1.27.2.2
> diff -u -u -r1.27.2.2 lpr.c
> --- lpr.c 1999/08/29 15:43:29 1.27.2.2
> +++ lpr.c 1999/12/08 01:47:47
> @@ -370,6 +370,27 @@
> }
> if (sflag)
> printf("%s: %s: not linked, copying instead\n", name, arg);
> + if( f ) { /* means that the file should be deleted */
> + printf("sleeping\n");
> + sleep(5);
> + printf("done.\n");
> + seteuid(euid); /* needed for rename() to succeed */
> + if( ! rename( arg, dfname ) ) {
> + register int i;
> + chmod( dfname, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP );
> + chown( dfname, userid, getgrnam("daemon")->gr_gid );
> + seteuid(uid);
> + if (format == 'p')
> + card('T', title ? title : arg);
> + for (i = 0; i < ncopies; i++)
> + card(format, &dfname[inchar-2]);
> + card('U', &dfname[inchar-2]);
> + card('N', arg);
> + nact++;
> + continue;
> + }
> + seteuid(uid);
> + }
> if ((i = open(arg, O_RDONLY)) < 0) {
> printf("%s: cannot open %s\n", name, arg);
> } else {
>
>
>
>
>
> To Unsubscribe: send mail to majordomo@FreeBSD.org
> with "unsubscribe freebsd-stable" in the body of the message
--
"In a world without walls and fences, who needs windows and gates?"
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-stable" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?19991209153320.A62121>