AUDIT

   1 This is the xinetd-2.3.0 audit status.  The audit has been performed
   2 in order to make the Owl (http://www.openwall.com/Owl/) xinetd package
   3 reasonably secure and of course with the hope that others will find
   4 the results and patches useful as well.
   5
   6 Much of xinetd's logic is left unaudited (other than for generic bug
   7 classes listed below).  In particular, this applies to all network
   8 access control checks.
   9
  10 To summarize the results, xinetd may be reasonably safe to use with
  11 these patches, but the code remains far from clean and certain bugs
  12 are there by design.
  13
  14 The format of this list is one item per line, with subitems indented.
  15 If a line doesn't start with a '+', that item hasn't been completed
  16 (audited and/or patched).
  17
  18 None of the PATCH'es are a part of xinetd-2.3.0; they will be in the
  19 Owl package and hopefully will get incorporated into future versions
  20 of xinetd.
  21
  22 --
  23 Solar Designer <solar@openwall.com>
  24
  25 +BUGS   strx_* functions (danger: don't always NUL-terminate)
  26 +PATCH          bug: shouldn't write NUL when len <= 0
  27 +PATCH          may overflow with huge precision values (bad for format bugs)
  28 +BUGS   strx_* calls: some assume NUL-termination, but none look dangerous
  29 +PATCH          __xlog_explain_errno forgets to update size
  30 +PATCH          child.c: child_process may not NUL-terminate name
  31 +PATCH          init.c: syscall_failed may not NUL-terminate err
  32 +PATCH          shutdown.c: safe, but should use print_buf_size-1
  33 +PATCH          signals.c: sig_name should use sizeof(signame_buf)-1
  34 +OK     str_* calls
  35 +OK             none :-)
  36 +OK     strcat, strcpy calls (testsuite calls not checked)
  37 +OK     strn* calls
  38 +PATCH          some inefficient, but correct
  39 +BUGS   memcpy, memmove, bcopy calls
  40 +PATCH          addr.c: addrlist_dump() copies ipv6 address into ipv4 struct
  41 +PATCH          addr.c: host_addr() trusts hep->h_length from gethostbyname()
  42 +PATCH          parsers.c: redir_parser() wouldn't build/work when NO_INET_ATON
  43 +PATCH          parsers.c: redir_parser() wrongly relies on sizeof(he->h_addr)
  44 +PATCH          parsers.c: bind_parser() same as the above
  45                 the use of sizeof() is inconsistent (not always of dst arg)
  46 +PATCH          should change bcopy to memcpy/memmove as appropriate
  47 +BUGS   sio_memcopy calls
  48 +OK             sio.c
  49 +BUGS           siosup.c is too complicated in its handling of data sizes
  50 +OK                     expand() is only called with old_size < new_size
  51 +?                      buffer_setup() isn't obviously correct, but is safe
  52 +OK                     __sio_extend_buffer is correct
  53 +PATCH                          comment to sio.h: aux buffer is right below buf
  54 +PATCH                  __sio_get_events may cause bufentries < 0 (->overflow)
  55 +OK     conn_setaddr calls: safe, but could use sizeof((cp)->co_remote_address)
  56 +OK     sprintf calls
  57 +BUGS   sscanf calls
  58 +PATCH          addr.c: explicit_mask() has single-byte overflow of saddr[]
  59 +BUGS   formats
  60 +OK             fprintf, sprintf, fscanf, sscanf
  61 +OK             syslog (but relies on %.*s for non-NUL-terminated buffers)
  62 +OK             strx_*
  63 +OK             svc_logprint, prepare_buffer
  64 +BUGS           msg, parsemsg
  65 +PATCH                  intcommon.c: int_init() passes fmt as wrong arg
  66 +PATCH                  (non-security) should never have '\n' in format
  67 +OK             tabprint
  68 +OK             Sprint
  69 +OK             ostimer.c: terminate
  70 +PATCH          xlog_write() is called on untrusted input w/o XLOG_NO_ERRNO
  71 +OK     ident -- looks mostly safe
  72 +OK             SIGALRM + longjmp() used safely:
  73 +OK                     no static accesses, no stdio between START/STOP_TIMER
  74 +OK                     immediate return on timeout (-> no clobbering issues)
  75 +OK                     signal mask after longjmp() is unimportant
  76 +PATCH                  could be safer to also reset SIGALRM handler
  77 +PATCH          verify_line() modifies buf, which is then logged on error
  78                 will log control chars from remote
  79 +BUGS   builtins
  80 +OK             time, daytime, sensor: NO_FORK && stream (safe: FNDELAY)
  81 +PATCH          accept() return value never checked
  82 +PATCH          bad_port_check(): should deny all <1024 (53/udp is just as bad)
  83 +PATCH          stream_daytime writes to wrong fd when wait = yes (gets EPIPE)
  84 +PATCH          *_time sends sizeof(time_t): wrong on at least linux-alpha
  85                 xadmin_handler(): the command parser is unreliable (use sio?)
  86 +BUGS   record (shutdown.c)
  87                 connect_back may be used for portscanning of own machine
  88                 write() return values not checked
  89 +PATCH          will only handle traditional (obsolete) crypt(3) hashes
  90                 special.c: stream_shutdown() will log control chars from remote
  91         intercept (int[.c]*, {tcp,udp}int.c) -- checked for generic bugs ONLY
  92                 tcpint.c: si_exit() may leave open fd from accept()
  93 +BUGS   signal races, longjmp clobbering
  94 +BUGS           __ostimer_interrupt:
  95 +PATCH                  call_level should be volatile
  96 +PATCH                  should use &ret_env, not (char *)ret_env (may differ)
  97 +BUGS                   ret_env modified non-atomically (with 2 TIMER_LONGJMP's)
  98 +PATCH                          should set have_env before and make it volatile
  99 +PATCH                  may leave an altered signal mask on longjmp()
 100                                 should fallback to plain longjmp in configure
 101 +OK                     __ostimer_{add,remove} only called with signals blocked
 102 +OK                     timer_s fields not volatile, safe due to block _calls_
 103 +BUGS           check uses of TIMER_LONGJMP flag
 104 +BUGS                   confparse.c: get_conf() may jump out of malloc(), etc
 105                                 no other uses, perhaps just disable CONF_TIMEOUT
 106 +OK             ident.c: mostly safe (see above)
 107 +BUGS           signals.c: bad_signal(): only on crashes, so not a big issue
 108 +PATCH                  *count should be volatile to really avoid looping
 109                         does calls which may cause another SIGSEGV w/o a bug
 110 +PATCH                  may leave an altered signal mask on longjmp()
 111 +BUGS           signals.c: general_handler()
 112                         sio and non-reentrant libc calls on unexpected signal
 113 +BUGS           signals.c: handle_signal(), my_handler(): events may be lost
 114                         my_handler may be re-entered, but M_SET isn't atomic:
 115                                 should split ps.flags into int-per-flag
 116 +OK             main.c: main(): setjmp() placed in a way avoiding clobbering
 117 +BUGS           access.c: parent_access_control(), alrm_ena() (cps feature)
 118                         alrm_ena() may cause re-entry into syslog(), etc
 119                         SIGALRM handler may be never reset, or --
 120                         the handler and/or alarm may be reset for other needs
 121                                 should use the timer queues, not OS timers
 122 +BUGS           int.c: int_sighandler(), intcommon.c: int_init()
 123                         int_sighandler() may cause re-entry into msg(), etc
 124                                 installed for multiple signals, doesn't block
 125                                 may interrupt msg() in main and cause re-entry
 126 +PATCH          redirect.c: redir_sigpipe() should use _exit(2), not exit(3)
 127 +BUGS   fd_set overflows
 128                 intcommon.c: sets INT_REMOTE(ip) w/o fd_set size check
 129                         {tcp,udp}int.c: si_mux(), di_mux() FD_SET w/o fd check
 130                 internals.c: socket_mask_copy w/o fd checks
 131                 main_loop(): select() on read_mask w/o fd checks
 132                 service.c: svc_activate(): could check ps.rws.mask_max here
 133                 (many files) all references to ps.rws.socket_mask are unchecked
 134                 redirect.c: redir_handler() no checks for rdfd, msfd
 135                 should use fd_grow similarly to OpenBSD inetd
 136 +PATCH          workaround: reduce RLIMIT_NOFILE to FD_SETSIZE
 137 +BUGS   __sio_descriptors overflows
 138 +PATCH          sio functions forget to check fd against n_descriptors
 139 +BUGS   get_fd_limit(), Smorefds()
 140 +PATCH          assume RLIMIT_NOFILE is small (not RLIM_INFINITY)
 141 +OK             orig_max_descriptors and max_descriptors are rlim_t, not int
 142 +BUGS   potential fd leaks to services
 143 +PATCH          init.c: setup_file_descriptors() relies on the rlimit only
 144                 returns from close() never checked -- fixed the worst one
 145 +BUGS   child.c: set_credentials()
 146 +PATCH          should fail if !ps.ros.is_superuser && user/group requested
 147 +OK             is otherwise fail-close and resets groups (fixed long ago)
 148 +BUGS   gethostby*, getaddrinfo (some are common with memcpy bugs)
 149 +PATCH          addr.c: host_addr() trusts hep->h_length from gethostbyname()
 150                 addr.c: host_addr() INET6 doesn't check res->ai_family
 151                 parsers.c: {redir,bind}_parser() don't check res->ai_family
 152 +PATCH          parsers.c: redir_parser() wrongly relies on sizeof(he->h_addr)
 153 +PATCH          parsers.c: bind_parser() same as the above
 154 +PATCH  getpwnam unnecessarily leaves password hashes in address space on BSD
 155 ---
 156 +PATCH  gcc format attributes
 157 +       build with gcc -Wall -Wcast-align (x86, alpha) -- mostly clean
 158 +PATCH          many unused vars with ipv6
 159 +PATCH          parsers.c, inet.c: stores strtol() to int, then needs long
 160 +PATCH          several format strings don't match arguments
 161 +       build with ccc -msg_enable level4 or higher
 162 +PATCH          CC= from configure doesn't get into Makefile's
 163                 lots of warnings (250 KB of output), the code is just not clean
 164 +PATCH                  some really need fixing
 165 -       use wrapper functions around either strx_* or vsnprintf()?
 166 +               not a good idea, tested strx_* against snprintf instead
 167 ?       define strz_* wrappers around strx_*, which would always NUL-terminate
 168 +PATCH  atoi -> strtol with long to int overflow checks
 169 ---
 170         should limit logging rate (= rate of permitted sessions, popa3d-like)
 171         should drop privileges for ident lookups, builtins, and records
 172         should have options to build --without-{ident,builtins,record,intercept}
 173                 should generate manpages accordingly