7004965 2001-08-30 05:40 +0400 /207 rader/ Solar Designer <solar@openwall.com>
Sänt av: joel@lysator.liu.se
Importerad: 2001-08-30 04:27 av Brevbäraren
Extern mottagare: bugtraq@securityfocus.com
Extern kopiemottagare: security-audit@ferret.lmh.ox.ac.uk
Mottagare: Bugtraq (import) <19000>
Ärende: xinetd 2.3.0 audit status
------------------------------------------------------------
From: Solar Designer <solar@openwall.com>
To: bugtraq@securityfocus.com
Cc: security-audit@ferret.lmh.ox.ac.uk
Message-ID: <20010830054009.A31413@openwall.com>
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
(7004965) /Solar Designer <solar@openwall.com>/(Ombruten)