Thank you very much for the feedback. On 3/14/14 9:38 AM, Ingo Schwarze wrote: > According to the sigaction(3) manual, "volatile sig_atomic_t" would > be better. If i understand correctly, overzealous compilers might > otherwise optimize checks away.
Dammit, of course. I should have caught that. > 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. Right. That's why I wasn't sure where to place the check. Now I realize why the entire approach is incorrect. > 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. The following is based on the assumption that you meant session rather than process group [i.e. kill(getsid(), SIGHUP)]: I don't think we should introduce such behavior because I can think of several situations where it wouldn't work as expected (like w/ su), and it encourages the user to rely on functionality which has certain caveats that they would probably ignore or forget, even if well documented. The possibility of behaving in unexpected ways is quite dangerous, especially when you'd only notice after the timeout period, and even more so when it directly leads to an attacker with a shell. > 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'm in favor of that. Following from that, what would you think of making -p default too and providing a way to negate it if desired. The thinking is that an inattentive operator may think to type "lock" while leaving his workstation in a hurry, but may not notice or remember to enter a password (because he is distracted and leaving). If we're going to break backwards compatibility anyway, this seems like a more sane default to me. > 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... How about just making the signal handler safe then? I think that's the simplest approach. I should have tried that to begin with. Would this do it? (applies on top of my first lock(1) patch http://marc.info/?l=openbsd-tech&m=139465495417843&q=raw ) --- lock.c.post-patch1 Mon Mar 17 13:10:43 2014 +++ lock.c Mon Mar 17 13:50:53 2014 @@ -61,8 +61,9 @@ #define TIMEOUT 15 -void bye(int); -void hi(void); +void time_remaining(void); +static size_t sigsafe_strlen(const char *); +void do_timeout(int); struct timeval timeout; struct timeval zerotime; @@ -165,7 +166,7 @@ (void)signal(SIGINT, SIG_IGN); (void)signal(SIGQUIT, SIG_IGN); (void)signal(SIGTSTP, SIG_IGN); - (void)signal(SIGALRM, bye); + (void)signal(SIGALRM, do_timeout); ntimer.it_interval = zerotime; ntimer.it_value = timeout; @@ -186,7 +187,7 @@ for (cnt = 0;;) { if (!readpassphrase("Key: ", s, sizeof(s), RPP_ECHO_OFF) || *s == '\0') { - hi(); + time_remaining(); continue; } if (usemine) { @@ -220,7 +221,7 @@ } void -hi(void) +time_remaining(void) { char buf[1024], buf2[1024]; time_t left; @@ -237,12 +238,33 @@ (void) write(STDERR_FILENO, buf, strlen(buf)); } +/* + * Duplicate of our libc's strlen. I don't like duplicating code, but POSIX + * doesn't guarantee strlen() is signal-safe, so we need to assume it may + * become unsafe in the future, maybe due to some crazy hardware-enhanced + * optimization that stores state or something... + */ +static size_t +sigsafe_strlen(const char *str) +{ + const char *s; + + for (s = str; *s; ++s) + ; + return (s - str); +} + /*ARGSUSED*/ void -bye(int signo) +do_timeout(int signo) { + extern char *__progname; - if (!no_timeout) - warnx("timeout"); + if (!no_timeout) { + #define TIMEOUT_SUFFIX ": timeout\n" + write(STDERR_FILENO, __progname, sigsafe_strlen(__progname)); + write(STDERR_FILENO, TIMEOUT_SUFFIX, + sigsafe_strlen(TIMEOUT_SUFFIX)); + } _exit(1); }
