From:	 Solar Designer <solar@openwall.com>
To:	 bugtraq@securityfocus.com
Subject: xinetd 2.3.0 audit status
Date:	 Thu, 30 Aug 2001 05:40:09 +0400
Cc:	 security-audit@ferret.lmh.ox.ac.uk

Hi,

As some of you may know, I've performed an audit of the xinetd 2.3.0
source code for certain classes of vulnerabilities.  The audit has
resulted in a significant number of fixes (many are for non-security
issues).  The patch was over 100 KB large and got incorporated into
xinetd starting with 2.3.1.  There were, however, certain issues with
patch merging, and the version of xinetd which finally has all of the
fixes (plus some more, by other people) is 2.3.3.

It is important to understand that no audit is a complete guarantee,
and that the audit I've performed covered only a subset of possible
problems as listed in AUDIT (now included with xinetd itself and also
at the end of this posting).  The USERID and RECORD features remain
dangerous: the code to handle them is overly privileged.  Packagers
should make sure these are disabled by default.

The worst known security problem with 2.3.0 is that it turned out to
not fully fix the string handling vulnerability previously discovered
by Sebastian Krahmer of SuSE Security Team.  A single NUL byte could
still be written beyond the intended area.  This kind of a flaw has
previously been shown to be exploitable in at least some cases with
the help of frame pointers (Olaf Kirch, 1998, "The poisoned NUL byte").
Sebastian's patch didn't have the bug, so the SuSE xinetd updates are
likely not affected by this.

--- xinetd-2.3.*/AUDIT
This is the xinetd-2.3.0 audit status.  The audit has been performed
in order to make the Owl (http://www.openwall.com/Owl/) xinetd package
reasonably secure and of course with the hope that others will find
the results and patches useful as well.

Much of xinetd's logic is left unaudited (other than for generic bug
classes listed below).  In particular, this applies to all network
access control checks.

To summarize the results, xinetd may be reasonably safe to use with
these patches, but the code remains far from clean and certain bugs
are there by design.

The format of this list is one item per line, with subitems indented.
If a line doesn't start with a '+', that item hasn't been completed
(audited and/or patched).

None of the PATCH'es are a part of xinetd-2.3.0; they will be in the
Owl package and hopefully will get incorporated into future versions
of xinetd.

-- 
Solar Designer <solar@openwall.com>

+BUGS	strx_* functions (danger: don't always NUL-terminate)
+PATCH		bug: shouldn't write NUL when len <= 0
+PATCH		may overflow with huge precision values (bad for format bugs)
+BUGS	strx_* calls: some assume NUL-termination, but none look dangerous
+PATCH		__xlog_explain_errno forgets to update size
+PATCH		child.c: child_process may not NUL-terminate name
+PATCH		init.c: syscall_failed may not NUL-terminate err
+PATCH		shutdown.c: safe, but should use print_buf_size-1
+PATCH		signals.c: sig_name should use sizeof(signame_buf)-1
+OK	str_* calls
+OK		none :-)
+OK	strcat, strcpy calls (testsuite calls not checked)
+OK	strn* calls
+PATCH		some inefficient, but correct
+BUGS	memcpy, memmove, bcopy calls
+PATCH		addr.c: addrlist_dump() copies ipv6 address into ipv4 struct
+PATCH		addr.c: host_addr() trusts hep->h_length from gethostbyname()
+PATCH		parsers.c: redir_parser() wouldn't build/work when NO_INET_ATON
+PATCH		parsers.c: redir_parser() wrongly relies on sizeof(he->h_addr)
+PATCH		parsers.c: bind_parser() same as the above
		the use of sizeof() is inconsistent (not always of dst arg)
+PATCH		should change bcopy to memcpy/memmove as appropriate
+BUGS	sio_memcopy calls
+OK		sio.c
+BUGS		siosup.c is too complicated in its handling of data sizes
+OK			expand() is only called with old_size < new_size
+?			buffer_setup() isn't obviously correct, but is safe
+OK			__sio_extend_buffer is correct
+PATCH				comment to sio.h: aux buffer is right below buf
+PATCH			__sio_get_events may cause bufentries < 0 (->overflow)
+OK	conn_setaddr calls: safe, but could use sizeof((cp)->co_remote_address)
+OK	sprintf calls
+BUGS	sscanf calls
+PATCH		addr.c: explicit_mask() has single-byte overflow of saddr[]
+BUGS	formats
+OK		fprintf, sprintf, fscanf, sscanf
+OK		syslog (but relies on %.*s for non-NUL-terminated buffers)
+OK		strx_*
+OK		svc_logprint, prepare_buffer
+BUGS		msg, parsemsg
+PATCH			intcommon.c: int_init() passes fmt as wrong arg
+PATCH			(non-security) should never have '\n' in format
+OK		tabprint
+OK		Sprint
+OK		ostimer.c: terminate
+PATCH		xlog_write() is called on untrusted input w/o XLOG_NO_ERRNO
+OK	ident -- looks mostly safe
+OK		SIGALRM + longjmp() used safely:
+OK			no static accesses, no stdio between START/STOP_TIMER
+OK			immediate return on timeout (-> no clobbering issues)
+OK			signal mask after longjmp() is unimportant
+PATCH			could be safer to also reset SIGALRM handler
+PATCH		verify_line() modifies buf, which is then logged on error
		will log control chars from remote
+BUGS	builtins
+OK		time, daytime, sensor: NO_FORK && stream (safe: FNDELAY)
+PATCH		accept() return value never checked
+PATCH		bad_port_check(): should deny all <1024 (53/udp is just as bad)
+PATCH		stream_daytime writes to wrong fd when wait = yes (gets EPIPE)
+PATCH		*_time sends sizeof(time_t): wrong on at least linux-alpha
		xadmin_handler(): the command parser is unreliable (use sio?)
+BUGS	record (shutdown.c)
		connect_back may be used for portscanning of own machine
		write() return values not checked
+PATCH		will only handle traditional (obsolete) crypt(3) hashes
		special.c: stream_shutdown() will log control chars from remote
	intercept (int[.c]*, {tcp,udp}int.c) -- checked for generic bugs ONLY
		tcpint.c: si_exit() may leave open fd from accept()
+BUGS	signal races, longjmp clobbering
+BUGS		__ostimer_interrupt:
+PATCH			call_level should be volatile
+PATCH			should use &ret_env, not (char *)ret_env (may differ)
+BUGS			ret_env modified non-atomically (with 2 TIMER_LONGJMP's)
+PATCH				should set have_env before and make it volatile
+PATCH			may leave an altered signal mask on longjmp()
				should fallback to plain longjmp in configure
+OK			__ostimer_{add,remove} only called with signals blocked
+OK			timer_s fields not volatile, safe due to block _calls_
+BUGS		check uses of TIMER_LONGJMP flag
+BUGS			confparse.c: get_conf() may jump out of malloc(), etc
				no other uses, perhaps just disable CONF_TIMEOUT
+OK		ident.c: mostly safe (see above)
+BUGS		signals.c: bad_signal(): only on crashes, so not a big issue
+PATCH			*count should be volatile to really avoid looping
			does calls which may cause another SIGSEGV w/o a bug
+PATCH			may leave an altered signal mask on longjmp()
+BUGS		signals.c: general_handler()
			sio and non-reentrant libc calls on unexpected signal
+BUGS		signals.c: handle_signal(), my_handler(): events may be lost
			my_handler may be re-entered, but M_SET isn't atomic:
				should split ps.flags into int-per-flag
+OK		main.c: main(): setjmp() placed in a way avoiding clobbering
+BUGS		access.c: parent_access_control(), alrm_ena() (cps feature)
			alrm_ena() may cause re-entry into syslog(), etc
			SIGALRM handler may be never reset, or --
			the handler and/or alarm may be reset for other needs
				should use the timer queues, not OS timers
+BUGS		int.c: int_sighandler(), intcommon.c: int_init()
			int_sighandler() may cause re-entry into msg(), etc
				installed for multiple signals, doesn't block
				may interrupt msg() in main and cause re-entry
+PATCH		redirect.c: redir_sigpipe() should use _exit(2), not exit(3)
+BUGS	fd_set overflows
		intcommon.c: sets INT_REMOTE(ip) w/o fd_set size check
			{tcp,udp}int.c: si_mux(), di_mux() FD_SET w/o fd check
		internals.c: socket_mask_copy w/o fd checks
		main_loop(): select() on read_mask w/o fd checks
		service.c: svc_activate(): could check ps.rws.mask_max here
		(many files) all references to ps.rws.socket_mask are unchecked
		redirect.c: redir_handler() no checks for rdfd, msfd
		should use fd_grow similarly to OpenBSD inetd
+PATCH		workaround: reduce RLIMIT_NOFILE to FD_SETSIZE
+BUGS	__sio_descriptors overflows
+PATCH		sio functions forget to check fd against n_descriptors
+BUGS	get_fd_limit(), Smorefds()
+PATCH		assume RLIMIT_NOFILE is small (not RLIM_INFINITY)
+OK		orig_max_descriptors and max_descriptors are rlim_t, not int
+BUGS	potential fd leaks to services
+PATCH		init.c: setup_file_descriptors() relies on the rlimit only
		returns from close() never checked -- fixed the worst one
+BUGS	child.c: set_credentials()
+PATCH		should fail if !ps.ros.is_superuser && user/group requested
+OK		is otherwise fail-close and resets groups (fixed long ago)
+BUGS	gethostby*, getaddrinfo (some are common with memcpy bugs)
+PATCH		addr.c: host_addr() trusts hep->h_length from gethostbyname()
		addr.c: host_addr() INET6 doesn't check res->ai_family
		parsers.c: {redir,bind}_parser() don't check res->ai_family
+PATCH		parsers.c: redir_parser() wrongly relies on sizeof(he->h_addr)
+PATCH		parsers.c: bind_parser() same as the above
+PATCH	getpwnam unnecessarily leaves password hashes in address space on BSD
---
+PATCH	gcc format attributes
+	build with gcc -Wall -Wcast-align (x86, alpha) -- mostly clean
+PATCH		many unused vars with ipv6
+PATCH		parsers.c, inet.c: stores strtol() to int, then needs long
+PATCH		several format strings don't match arguments
+	build with ccc -msg_enable level4 or higher
+PATCH		CC= from configure doesn't get into Makefile's
		lots of warnings (250 KB of output), the code is just not clean
+PATCH			some really need fixing
-	use wrapper functions around either strx_* or vsnprintf()?
+		not a good idea, tested strx_* against snprintf instead
?	define strz_* wrappers around strx_*, which would always NUL-terminate
+PATCH	atoi -> strtol with long to int overflow checks
---
	should limit logging rate (= rate of permitted sessions, popa3d-like)
	should drop privileges for ident lookups, builtins, and records
	should have options to build --without-{ident,builtins,record,intercept}
		should generate manpages accordingly