Hi Jean-Philippe,
Jean-Philippe Ouellet wrote on Wed, Mar 12, 2014 at 07:11:05PM -0400:
> On Wed, Mar 12, 2014 at 11:09:14PM +0100, Ingo Schwarze wrote:
>> I don't really like the warnx(3) call from the bye() ALRM handler
>> either, but that's a separate matter.
> Me neither.
> Maybe something like this instead?
Your second try is incorrect, see below.
I prefer to not attempt to produce one big patch fixing everything
at once, but get things done in steps.
Your first patch (just fixing the double message and changing hi()
to not be a signal handler) was correct and solved a real issue.
Any OKs for that?
Yours,
Ingo
P.S.
Here are some more thoughts and suggestions regarding this program:
> Index: lock.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/lock/lock.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 lock.c
> --- lock.c 22 Aug 2013 04:43:40 -0000 1.27
> +++ lock.c 12 Mar 2014 23:02:36 -0000
> @@ -61,13 +61,14 @@
>
> #define TIMEOUT 15
>
> -void bye(int);
> -void hi(int);
> +void time_remaining(void);
> +void do_timeout(int);
>
> struct timeval timeout;
> struct timeval zerotime;
> time_t nexttime; /* keep the timeout time */
> int no_timeout; /* lock terminal forever */
> +sig_atomic_t done;
According to the sigaction(3) manual, "volatile sig_atomic_t" would
be better. If i understand correctly, overzealous compilers might
otherwise optimize checks away.
> extern char *__progname;
>
> @@ -162,10 +163,10 @@ main(int argc, char *argv[])
> }
>
> /* set signal handlers */
> - (void)signal(SIGINT, hi);
> - (void)signal(SIGQUIT, hi);
> - (void)signal(SIGTSTP, hi);
> - (void)signal(SIGALRM, bye);
> + (void)signal(SIGINT, SIG_IGN);
> + (void)signal(SIGQUIT, SIG_IGN);
> + (void)signal(SIGTSTP, SIG_IGN);
> + (void)signal(SIGALRM, do_timeout);
>
> ntimer.it_interval = zerotime;
> ntimer.it_value = timeout;
> @@ -183,10 +184,15 @@ main(int argc, char *argv[])
> __progname, ttynam, hostname, sectimeout, date);
> }
>
> - for (cnt = 0;;) {
> + for (cnt = 0, done = 0;;) {
> + if (done) {
> + if (!no_timeout)
That condition is always true.
> + warnx("timeout");
> + _exit(1);
> + }
As we are no longer in a signal handler, the whole test can be
simplified to:
if (done)
errx(1, "timeout");
However, either way is incorrect. There is a race condition.
The ALRM signal may arrive after the if(done), but before the
call to readpassphrase(). Yes, that's narrow, but still.
In that case, the lock utility will sit at the "Key:" prompt
for good, even though a timeout was requested.
Then you just hit "enter" at the "Key:" prompt, and bang,
it says "timeout" and gives you the shell. Ouch.
> if (!readpassphrase("Key: ", s, sizeof(s), RPP_ECHO_OFF) ||
> *s == '\0') {
[...]
That said, i consider having a lock(1) utility time out stupid
in the first place. It is conceptually insecure. Would anybody
be opposed to either of the following changes?
1) On timeout and before exiting, send a -HUP signal to the
process group [i.e. kill(0, SIGHUP)]. That way, you get the
terminal back on timeout, but without the login shell still
open.
OR (i guess i'd even prefer that, the above seems slightly fragile):
2) Make -n the default and silently ignore the -t option.
The lock(1) utility still won't become a model of robustness and
security, but at least a bit safer. Right now, it is horribly
insecure by default. Imagine an operator being called away from
the terminal, quickly typing "lock". How easy is it to forget "-n"?
Now if the attacker manages to distract the operator for 15 minutes,
he gets the shell for free.
Well, lock(1) is part of the 2BSD legacy, one of the few tools
remaining from that time. But i think we shouldn't be awestruck
but still ensure a minimum level of sanity.
I certainly don't feel like setting up the usual pipe(2) to self
and a select(2) or poll(2) loop merely to correctly implement a
feature that's an awful idea in the first place...
Yours,
Ingo
