system/5394: authpf: protect critical code regions, check for authpf.conf as early as possible

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

system/5394: authpf: protect critical code regions, check for authpf.conf as early as possible

Stefan Krah
>Number:         5394
>Category:       system
>Synopsis:       authpf: protect critical code regions, check for authpf.conf as early as possible
>Confidential:   yes
>Severity:       serious
>Priority:       medium
>Responsible:    bugs
>State:          open
>Quarter:        
>Keywords:      
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Feb 24 13:50:02 GMT 2007
>Closed-Date:
>Last-Modified:
>Originator:     Stefan Krah
>Release:        OpenBSD 4.0
>Organization:
net
>Environment:
        <machine, os, target, libraries (multiple lines)>
        System      : OpenBSD 4.0
        Architecture: OpenBSD.i386
        Machine     : i386
>Description:

1)

Authpf depends on a controlled shutdown after acquiring the lock on the
pidfile and (especially) after changing the filter rules. - Currently
there is a short window of opportunity for termination signals to occur
before the signal handlers are installed.

Also, the pfctl child process is not protected against signals.


2)

The check for authpf.conf should probably go back to the start as stated
in the man page:

"authpf will not run if..."


>How-To-Repeat:
>Fix:

Index: authpf.c
===================================================================
RCS file: /cvs/src/usr.sbin/authpf/authpf.c,v
retrieving revision 1.101
diff -u -r1.101 authpf.c
--- authpf.c 22 Feb 2007 21:54:23 -0000 1.101
+++ authpf.c 24 Feb 2007 11:49:08 -0000
@@ -50,6 +50,8 @@
 
 #include "pathnames.h"
 
+#define ASIZE(a) (sizeof (a) / sizeof *(a))
+
 static int read_config(FILE *);
 static void print_message(char *);
 static int allowed_luser(char *);
@@ -71,6 +73,7 @@
 
 struct timeval Tstart, Tend; /* start and end times of session */
 
+static sigset_t bset, oset;
 volatile sig_atomic_t want_death;
 static void need_death(int signo);
 static __dead void do_death(int);
@@ -84,6 +87,8 @@
 int
 main(int argc, char *argv[])
 {
+ int catchsig[] = { SIGTERM, SIGINT, SIGALRM, SIGPIPE,
+ SIGHUP, SIGQUIT, SIGTSTP };
  int lockcnt = 0, n, pidfd;
  FILE *config;
  struct in6_addr ina;
@@ -94,7 +99,23 @@
  char *shell;
  login_cap_t *lc;
 
- config = fopen(PATH_CONFFILE, "r");
+
+ /* if authpf.conf does not exist, exit right away */
+ if ((config = fopen(PATH_CONFFILE, "r")) == NULL) {
+ syslog(LOG_ERR, "%s does not exist", PATH_CONFFILE);
+ exit(1);
+ }
+
+ /* signal handlers should be installed before acquiring the lock */
+ for (n = 0; n < (int)ASIZE(catchsig); n++)
+ if (signal(catchsig[n], need_death) == SIG_ERR)
+ exit(1);
+
+ /* protect pfctl child process (inherits the signal mask) */
+ sigemptyset(&bset);
+ for (n = 0; n < (int)ASIZE(catchsig); n++)
+ sigaddset(&bset, catchsig[n]);
+ sigprocmask(SIG_BLOCK, &bset, &oset);
 
  if ((cp = getenv("SSH_TTY")) == NULL) {
  syslog(LOG_ERR, "non-interactive session connection for authpf");
@@ -277,10 +298,8 @@
  do_death(0);
  }
 
- if (config == NULL || read_config(config)) {
- syslog(LOG_INFO, "bad or nonexistent %s", PATH_CONFFILE);
+ if (read_config(config)) /* read_config displays error messages */
  do_death(0);
- }
 
  if (remove_stale_rulesets()) {
  syslog(LOG_INFO, "error removing stale rulesets");
@@ -303,23 +322,17 @@
  do_death(0);
  }
 
- signal(SIGTERM, need_death);
- signal(SIGINT, need_death);
- signal(SIGALRM, need_death);
- signal(SIGPIPE, need_death);
- signal(SIGHUP, need_death);
- signal(SIGQUIT, need_death);
- signal(SIGTSTP, need_death);
+ printf("\r\nHello %s. ", luser);
+ printf("You are authenticated from host \"%s\"\r\n", ipsrc);
+ setproctitle("%s@%s", luser, ipsrc);
+ print_message(PATH_MESSAGE);
+
+ /* unblock signals */
+ sigprocmask(SIG_SETMASK, &oset, NULL);
  while (1) {
- printf("\r\nHello %s. ", luser);
- printf("You are authenticated from host \"%s\"\r\n", ipsrc);
- setproctitle("%s@%s", luser, ipsrc);
- print_message(PATH_MESSAGE);
- while (1) {
- sleep(10);
- if (want_death)
- do_death(1);
- }
+ if (want_death)
+ do_death(1);
+ sleep(10);
  }
 
  /* NOTREACHED */
@@ -817,6 +830,8 @@
 static void
 need_death(int signo)
 {
+ /* protect pfctl again */
+ sigprocmask(SIG_BLOCK, &bset, NULL);
  want_death = 1;
 }


>Release-Note:
>Audit-Trail:
>Unformatted: