>From f39f851b0e52d30fe66dcb03e89800b752bf1b72 Mon Sep 17 00:00:00 2001 From: Peter Bex Date: Sun, 18 Nov 2012 21:03:51 +0100 Subject: [PATCH] Fix select() buffer overrun vulnerability by using POSIX poll() on systems that support it, leaving only those few systems vulnerable that don't (ie, only Windows). --- Makefile.bsd | 1 + Makefile.haiku | 1 + Makefile.linux | 1 + Makefile.macosx | 1 + Makefile.solaris | 1 + NEWS | 4 ++ scheduler.scm | 130 +++++++++++++++++++++++++++++++++++-------------------- 7 files changed, 92 insertions(+), 47 deletions(-) diff --git a/Makefile.bsd b/Makefile.bsd index fb1f244..00a77ca 100644 --- a/Makefile.bsd +++ b/Makefile.bsd @@ -83,6 +83,7 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_LONG_LONG 1" >>$@ echo "#define HAVE_MEMMOVE 1" >>$@ echo "#define HAVE_MEMORY_H 1" >>$@ + echo "#define HAVE_POSIX_POLL 1" >>$@ echo "#define HAVE_SIGACTION 1" >>$@ echo "#define HAVE_SIGSETJMP 1" >>$@ echo "#define HAVE_SIGPROCMASK 1" >>$@ diff --git a/Makefile.haiku b/Makefile.haiku index ff06c5b..407366a 100644 --- a/Makefile.haiku +++ b/Makefile.haiku @@ -71,6 +71,7 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_LONG_LONG 1" >>$@ echo "#define HAVE_MEMMOVE 1" >>$@ echo "#define HAVE_MEMORY_H 1" >>$@ + echo "#define HAVE_POSIX_POLL 1" >>$@ echo "#define HAVE_SIGACTION 1" >>$@ echo "#define HAVE_SIGSETJMP 1" >>$@ echo "#define HAVE_SIGPROCMASK 1" >>$@ diff --git a/Makefile.linux b/Makefile.linux index b04e7f6..e944252 100644 --- a/Makefile.linux +++ b/Makefile.linux @@ -72,6 +72,7 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_LONG_LONG 1" >>$@ echo "#define HAVE_MEMMOVE 1" >>$@ echo "#define HAVE_MEMORY_H 1" >>$@ + echo "#define HAVE_POSIX_POLL 1" >>$@ echo "#define HAVE_SIGACTION 1" >>$@ echo "#define HAVE_SIGSETJMP 1" >>$@ echo "#define HAVE_SIGPROCMASK 1" >>$@ diff --git a/Makefile.macosx b/Makefile.macosx index dbdefe3..5cdf36d 100644 --- a/Makefile.macosx +++ b/Makefile.macosx @@ -96,6 +96,7 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_LONG_LONG 1" >>$@ echo "#define HAVE_MEMMOVE 1" >>$@ echo "#define HAVE_MEMORY_H 1" >>$@ + echo "#define HAVE_POSIX_POLL 1" >>$@ echo "#define HAVE_SIGACTION 1" >>$@ echo "#define HAVE_SIGSETJMP 1" >>$@ echo "#define HAVE_SIGPROCMASK 1" >>$@ diff --git a/Makefile.solaris b/Makefile.solaris index 6a92bf0..4d9f7a7 100644 --- a/Makefile.solaris +++ b/Makefile.solaris @@ -102,6 +102,7 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_LONG_LONG 1" >>$@ echo "#define HAVE_MEMMOVE 1" >>$@ echo "#define HAVE_MEMORY_H 1" >>$@ + echo "#define HAVE_POSIX_POLL 1" >>$@ echo "#define HAVE_SIGACTION 1" >>$@ echo "#define HAVE_STDINT_H 1" >>$@ echo "#define HAVE_STDLIB_H 1" >>$@ diff --git a/NEWS b/NEWS index 226d244..39c5bb2 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,9 @@ 4.8.1 +- Security fixes + - Use POSIX poll() on systems where available. This avoids a design flaw + in select(); it supports no more than FD_SETSIZE descriptors. + - Core libraries - Fixed EINTR handling in process-wait and when reading from file ports. diff --git a/scheduler.scm b/scheduler.scm index 7ff3d5f..f42ae09 100644 --- a/scheduler.scm +++ b/scheduler.scm @@ -31,9 +31,7 @@ (hide ready-queue-head ready-queue-tail ##sys#timeout-list ##sys#update-thread-state-buffer ##sys#restore-thread-state-buffer remove-from-ready-queue ##sys#unblock-threads-for-i/o ##sys#force-primordial - fdset-input-set fdset-output-set fdset-clear - fdset-select-timeout fdset-set fdset-test - create-fdset stderr + fdset-set fdset-test create-fdset stderr ##sys#clear-i/o-state-for-thread! ##sys#abandon-mutexes) (not inline ##sys#interrupt-hook) (unsafe) @@ -74,9 +72,66 @@ C_word C_msleep(C_word ms) { return C_SCHEME_TRUE; } #endif + +#ifdef HAVE_POSIX_POLL +# include +# include + +static int C_fdset_nfds; +static struct pollfd *C_fdset_set = NULL; + +C_inline int C_fd_ready(int fd, int pos, int what) { + assert(fd == C_fdset_set[pos].fd); /* Must match position in ##sys#fd-list! */ + return(C_fdset_set[pos].revents & what); +} + +#define C_fd_input_ready(fd,pos) C_mk_bool(C_fd_ready(C_unfix(fd), C_unfix(pos),POLLIN)) +#define C_fd_output_ready(fd,pos) C_mk_bool(C_fd_ready(C_unfix(fd), C_unfix(pos),POLLOUT)) + +C_inline int C_ready_fds_timeout(int to, double tm) { + return poll(C_fdset_set, C_fdset_nfds, to ? (int)tm : -1); +} + +C_inline void C_prepare_fdset(int length) { + /* TODO: Only realloc when needed? */ + C_fdset_set = realloc(C_fdset_set, sizeof(struct pollfd) * length); + if (C_fdset_set == NULL) + C_halt(C_SCHEME_FALSE); /* Ugly: no message */ + C_fdset_nfds = 0; +} + +/* This *must* be called in order, so position will match ##sys#fd-list */ +C_inline void C_fdset_add(int fd, int input, int output) { + C_fdset_set[C_fdset_nfds].events = ((input ? POLLIN : 0) | (output ? POLLOUT : 0)); + C_fdset_set[C_fdset_nfds++].fd = fd; +} + +#else + +/* Shouldn't we include here? */ static fd_set C_fdset_input, C_fdset_output; -#define C_fd_test_input(fd) C_mk_bool(FD_ISSET(C_unfix(fd), &C_fdset_input)) -#define C_fd_test_output(fd) C_mk_bool(FD_ISSET(C_unfix(fd), &C_fdset_output)) + +#define C_fd_input_ready(fd,pos) C_mk_bool(FD_ISSET(C_unfix(fd), &C_fdset_input)) +#define C_fd_output_ready(fd,pos) C_mk_bool(FD_ISSET(C_unfix(fd), &C_fdset_output)) + +C_inline int C_ready_fds_timeout(int to, double tm) { + struct timeval timeout; + timeout.tv_sec = tm / 1000; + timeout.tv_usec = fmod(tm, 1000) * 1000; + /* we use FD_SETSIZE, but really should use max fd */ + return select(FD_SETSIZE, &C_fdset_input, &C_fdset_output, NULL, to ? &timeout : NULL); +} + +C_inline void C_prepare_fdset(int length) { + FD_ZERO(&C_fdset_input); + FD_ZERO(&C_fdset_output); +} + +C_inline void C_fdset_add(int fd, int input, int output) { + if (input) FD_SET(fd, &C_fdset_input); + if (output) FD_SET(fd, &C_fdset_output); +} +#endif EOF ) ) @@ -329,12 +384,12 @@ EOF (##sys#schedule) ) ) -;;; `select()'-based blocking: +;;; `select()/poll()'-based blocking: (define ##sys#fd-list '()) ; ((FD1 THREAD1 ...) ...) (define (create-fdset) - (fdset-clear) + ((foreign-lambda void "C_prepare_fdset" int) (##sys#length ##sys#fd-list)) (let loop ((lst ##sys#fd-list)) (unless (null? lst) (let ((fd (caar lst))) @@ -345,35 +400,14 @@ EOF (cdar lst)) (loop (cdr lst)))))) -(define fdset-select-timeout - (foreign-lambda* int ([bool to] [double tm]) - "struct timeval timeout;" - "timeout.tv_sec = tm / 1000;" - "timeout.tv_usec = fmod(tm, 1000) * 1000;" - "C_return(select(FD_SETSIZE, &C_fdset_input, &C_fdset_output, NULL, to ? &timeout : NULL));") ) - -(define fdset-clear - (foreign-lambda* void () - "FD_ZERO(&C_fdset_input);" - "FD_ZERO(&C_fdset_output);")) - -(define fdset-input-set - (foreign-lambda* void ([int fd]) - "FD_SET(fd, &C_fdset_input);" ) ) - -(define fdset-output-set - (foreign-lambda* void ([int fd]) - "FD_SET(fd, &C_fdset_output);" ) ) - (define (fdset-set fd i/o) - (dbg "setting fdset for " fd " to " i/o) - (case i/o - ((#t #:input) (fdset-input-set fd)) - ((#f #:output) (fdset-output-set fd)) - ((#:all) - (fdset-input-set fd) - (fdset-output-set fd) ) - (else (panic "fdset-set: invalid i/o direction")))) + (let ((fdset-add! (foreign-lambda void "C_fdset_add" int bool bool))) + (dbg "setting fdset for " fd " to " i/o) + (case i/o + ((#t #:input) (fdset-add! fd #t #f)) + ((#f #:output) (fdset-add! fd #f #t)) + ((#:all) (fdset-add! fd #t #t)) + (else (panic "fdset-set: invalid i/o direction"))))) (define (fdset-test inf outf i/o) (case i/o @@ -408,29 +442,31 @@ EOF (fpmax 0.0 (fp- tmo1 now)) ) 0.0) ) ) ; otherwise immediate timeout. (dbg "waiting for I/O with timeout " tmo) - (let ((n (fdset-select-timeout ; we use FD_SETSIZE, but really should use max fd - (or rq? to?) - tmo))) + (let ((n ((foreign-lambda int "C_ready_fds_timeout" bool double) + (or rq? to?) tmo))) (dbg n " fds ready") (cond [(eq? -1 n) - (dbg "select(2) returned with result -1" ) + (dbg "select(2)/poll(2) returned with result -1" ) (##sys#force-primordial)] [(fx> n 0) (set! ##sys#fd-list - (let loop ([n n] [lst ##sys#fd-list]) + (let loop ((n n) (pos 0) (lst ##sys#fd-list)) (if (or (zero? n) (null? lst)) lst - (let* ([a (car lst)] - [fd (car a)] - [inf (##core#inline "C_fd_test_input" fd)] - [outf (##core#inline "C_fd_test_output" fd)]) + (let* ((a (car lst)) + (fd (car a)) + ;; pos *must* match position of fd in ##sys#fd-list + ;; This is checked in C_fd_ready with assert() + (inf (##core#inline "C_fd_input_ready" fd pos)) + (outf (##core#inline "C_fd_output_ready" fd pos))) (dbg "fd " fd " state: input=" inf ", output=" outf) (if (or inf outf) (let loop2 ((threads (cdr a)) (keep '())) (if (null? threads) (if (null? keep) - (loop (sub1 n) (cdr lst)) - (cons (cons fd keep) (loop (sub1 n) (cdr lst)))) + (loop (sub1 n) (add1 pos) (cdr lst)) + (cons (cons fd keep) + (loop (sub1 n) (add1 pos) (cdr lst)))) (let* ((t (car threads)) (p (##sys#slot t 11)) ) (dbg "checking " t " " p) @@ -452,7 +488,7 @@ EOF (##sys#thread-basic-unblock! t) (loop2 (cdr threads) keep)) (else (loop2 (cdr threads) (cons t keep))))))) - (cons a (loop n (cdr lst))) ) ) ) ) ) ] ))) ) + (cons a (loop n (add1 pos) (cdr lst))) ) ) ) ) ) ] ))) ) ;;; Clear I/O state for unblocked thread -- 1.7.12.2