re: bin/46500: Permission of created files in lpr.c wrong.

[jnemeth%victoria.tc.ca@localhost (John Nemeth)]

On Oct 20, 10:06pm, matthew green wrote:
} 
} > > >Description:
} > > Permission of created files */.seq is wrong.
} > > >How-To-Repeat:
} > > 
} > > >Fix:
} > > diff -u -p -r1.45 lpr.c
} > > --- usr.sbin/lpr/lpr/lpr.c        30 Aug 2011 19:27:37 -0000      1.45
} > > +++ usr.sbin/lpr/lpr/lpr.c        30 May 2012 09:07:55 -0000
} > > @@ -698,7 +698,7 @@ mktemps(void)
} > >  
} > >   (void)snprintf(buf, sizeof(buf), "%s/.seq", SD);
} > >   seteuid(euid);
} > > - if ((fd = open(buf, O_RDWR|O_CREAT, 0661)) < 0)
} > > + if ((fd = open(buf, O_RDWR|O_CREAT, 0664)) < 0)
} > >           err(1, "cannot create %s", buf);
} > >   if (flock(fd, LOCK_EX))
} > >           err(1, "cannot lock %s", buf);
} > 
} > what's wrong with this?  your change makes the temp files world
} > readable which seems like a security issue to me.

     All it contains is a three digit sequence number for the next
queue entry.  That doesn't seem to be particularly security critical,
but if you like, it could be changed to 660.  Certainly execute is
wrong.

} additionally, this will break lpd as it uses these execute bits
} modes specific meanings.

     Uh, read the code.  The .seq file is opened, locked, read,
written, and closed (implicitly unlocking) all within the same function
and the fd is never passed to any functions other then the libc
functions required to perform the above operations.  Also, the
containing function is only called from with one place in lpr.c.  In
other words, lpd doesn't even look at the file.

}-- End of excerpt from matthew green