From 91cd93b013aa57191b3f51d48427afbb3e6fa09a Mon Sep 17 00:00:00 2001 From: KLuka Date: Tue, 13 Jan 2015 13:04:01 +0100 Subject: [PATCH 1/6] Debug info and minor fix on child process exit (service exit) * Added debug information when child process exits - pid of child process (service) - exit code (this should help for debuging issues related to "Session closed") * Fixed status checking from waitpid() when child process exits - before we were checking wrong variable (checks were allways true) - now we use correct status variable --- shellinabox/launcher.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/shellinabox/launcher.c b/shellinabox/launcher.c index 68d8862..69e29e6 100644 --- a/shellinabox/launcher.c +++ b/shellinabox/launcher.c @@ -1616,7 +1616,8 @@ static void launcherDaemon(int fd) { int status; pid_t pid; while (NOINTR(pid = waitpid(-1, &status, WNOHANG)) > 0) { - if (WIFEXITED(pid) || WIFSIGNALED(pid)) { + debug("Child %d exited with exit code %d\n", pid, WEXITSTATUS(status)); + if (WIFEXITED(status) || WIFSIGNALED(status)) { char key[32]; snprintf(&key[0], sizeof(key), "%d", pid); deleteFromHashMap(childProcesses, key); @@ -1636,7 +1637,8 @@ static void launcherDaemon(int fd) { break; } while (NOINTR(pid = waitpid(-1, &status, WNOHANG)) > 0) { - if (WIFEXITED(pid) || WIFSIGNALED(pid)) { + debug("Child %d exited with exit code %d\n", pid, WEXITSTATUS(status)); + if (WIFEXITED(status) || WIFSIGNALED(status)) { char key[32]; snprintf(&key[0], sizeof(key), "%d", pid); deleteFromHashMap(childProcesses, key); From 4e5edd20c495e0f539bcf38eb1f6ff00e015f17e Mon Sep 17 00:00:00 2001 From: KLuka Date: Wed, 14 Jan 2015 15:05:54 +0100 Subject: [PATCH 2/6] Cleanup child process (service) on session timeout (Issue #103, #203) When browser tab/window is closed during active session, child process stays alive forever (even if shellinaboxd is terminated). https://code.google.com/p/shellinabox/issues/detail?id=103 https://code.google.com/p/shellinabox/issues/detail?id=203 * If session timeouts cleanup procedure is triggered. Procedure is executed in launcher process, because this is parent of child (service) process. There we can check, if we have correct child pid (stored in session) and than we can terminate process. * Added debug information about cleaning up child process * Fixed debug formatting from previous commit --- shellinabox/launcher.c | 50 ++++++++++++++++++++++++++++++++++++-- shellinabox/launcher.h | 12 +++++---- shellinabox/session.c | 2 ++ shellinabox/session.h | 2 ++ shellinabox/shellinaboxd.c | 6 ++++- 5 files changed, 64 insertions(+), 8 deletions(-) diff --git a/shellinabox/launcher.c b/shellinabox/launcher.c index 69e29e6..0291c61 100644 --- a/shellinabox/launcher.c +++ b/shellinabox/launcher.c @@ -520,6 +520,7 @@ int launchChild(int service, struct Session *session, const char *url) { ssize_t len = sizeof(struct LaunchRequest) + strlen(u) + 1; check(request = calloc(len, 1)); request->service = service; + request->terminate = -1; request->width = session->width; request->height = session->height; strncat(request->peerName, httpGetPeerName(session->http), @@ -547,6 +548,7 @@ int launchChild(int service, struct Session *session, const char *url) { return -1; } check(bytes == sizeof(pid)); + check(session->pid = pid); struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg); check(cmsg); check(cmsg->cmsg_level == SOL_SOCKET); @@ -555,6 +557,34 @@ int launchChild(int service, struct Session *session, const char *url) { return pid; } +int terminateChild(struct Session *session) { + if (launcher < 0) { + errno = EINVAL; + return -1; + } + + if (session->pid < 1) { + debug("Child pid for termination not valid!"); + return -1; + } + + // Send terminate request to launcher process + struct LaunchRequest *request; + ssize_t len = sizeof(struct LaunchRequest); + check(request = calloc(len, 1)); + request->terminate = session->pid; + if (NOINTR(write(launcher, request, len)) != len) { + debug("Child %d termination request failed!", request->terminate); + free(request); + return -1; + } + + free(request); + session->pid = 0; + session->cleanup = 0; + return 0; +} + struct Utmp { const char pid[32]; int pty; @@ -1616,7 +1646,7 @@ static void launcherDaemon(int fd) { int status; pid_t pid; while (NOINTR(pid = waitpid(-1, &status, WNOHANG)) > 0) { - debug("Child %d exited with exit code %d\n", pid, WEXITSTATUS(status)); + debug("Child %d exited with exit code %d", pid, WEXITSTATUS(status)); if (WIFEXITED(status) || WIFSIGNALED(status)) { char key[32]; snprintf(&key[0], sizeof(key), "%d", pid); @@ -1627,6 +1657,21 @@ static void launcherDaemon(int fd) { continue; } + // Check if we received terminate request from parent process and + // try to terminate child, if child is still running + if (request.terminate > 0) { + errno = 0; + NOINTR(pid = waitpid(request.terminate, &status, WNOHANG)); + if (pid == 0 && errno == 0) { + if (kill(request.terminate, SIGTERM) == 0) { + debug("Terminating child %d (kill)", request.terminate); + } else { + debug("Terminating child failed [%s]", strerror(errno)); + } + } + continue; + } + char *url; check(url = calloc(request.urlLength + 1, 1)); readURL: @@ -1637,7 +1682,7 @@ static void launcherDaemon(int fd) { break; } while (NOINTR(pid = waitpid(-1, &status, WNOHANG)) > 0) { - debug("Child %d exited with exit code %d\n", pid, WEXITSTATUS(status)); + debug("Child %d exited with exit code %d", pid, WEXITSTATUS(status)); if (WIFEXITED(status) || WIFSIGNALED(status)) { char key[32]; snprintf(&key[0], sizeof(key), "%d", pid); @@ -1682,6 +1727,7 @@ static void launcherDaemon(int fd) { childProcesses = newHashMap(destroyUtmpHashEntry, NULL); } addToHashMap(childProcesses, utmp->pid, (char *)utmp); + debug("Child %d launched", pid); } else { int fds[2]; if (!pipe(fds)) { diff --git a/shellinabox/launcher.h b/shellinabox/launcher.h index 945bccc..2d7d145 100644 --- a/shellinabox/launcher.h +++ b/shellinabox/launcher.h @@ -52,15 +52,17 @@ struct LaunchRequest { - int service; - int width, height; - char peerName[128]; - int urlLength; - char url[0]; + int service; + int width, height; + pid_t terminate; + char peerName[128]; + int urlLength; + char url[0]; }; int supportsPAM(void); int launchChild(int service, struct Session *session, const char *url); +int terminateChild(struct Session *session); void setWindowSize(int pty, int width, int height); int forkLauncher(void); void terminateLauncher(void); diff --git a/shellinabox/session.c b/shellinabox/session.c index f18203f..25f5513 100644 --- a/shellinabox/session.c +++ b/shellinabox/session.c @@ -121,6 +121,8 @@ void initSession(struct Session *session, const char *sessionKey, session->height = 0; session->buffered = NULL; session->len = 0; + session->pid = 0; + session->cleanup = 0; } struct Session *newSession(const char *sessionKey, Server *server, URL *url, diff --git a/shellinabox/session.h b/shellinabox/session.h index bd54963..11d2281 100644 --- a/shellinabox/session.h +++ b/shellinabox/session.h @@ -63,6 +63,8 @@ struct Session { int height; char *buffered; int len; + pid_t pid; + int cleanup; }; void addToGraveyard(struct Session *session); diff --git a/shellinabox/shellinaboxd.c b/shellinabox/shellinaboxd.c index d0d787b..ba734e1 100644 --- a/shellinabox/shellinaboxd.c +++ b/shellinabox/shellinaboxd.c @@ -274,8 +274,11 @@ static int completePendingRequest(struct Session *session, } static void sessionDone(void *arg) { - debug("Child terminated"); struct Session *session = (struct Session *)arg; + debug("Session %s done", session->sessionKey); + if (session->cleanup) { + terminateChild(session); + } session->done = 1; addToGraveyard(session); completePendingRequest(session, "", 0, INT_MAX); @@ -301,6 +304,7 @@ static int handleSession(struct ServerConnection *connection, void *arg, if (bytes || timedOut) { if (!session->http && timedOut) { debug("Timeout. Closing session."); + session->cleanup = 1; return 0; } check(!session->done); From efc00707b84e3f18ec2ef67109b0dde32299d25f Mon Sep 17 00:00:00 2001 From: KLuka Date: Fri, 16 Jan 2015 12:35:07 +0100 Subject: [PATCH 3/6] Removed trailing whitespace --- shellinabox/launcher.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shellinabox/launcher.c b/shellinabox/launcher.c index 0291c61..43bd356 100644 --- a/shellinabox/launcher.c +++ b/shellinabox/launcher.c @@ -1657,11 +1657,11 @@ static void launcherDaemon(int fd) { continue; } - // Check if we received terminate request from parent process and + // Check if we received terminate request from parent process and // try to terminate child, if child is still running if (request.terminate > 0) { errno = 0; - NOINTR(pid = waitpid(request.terminate, &status, WNOHANG)); + NOINTR(pid = waitpid(request.terminate, &status, WNOHANG)); if (pid == 0 && errno == 0) { if (kill(request.terminate, SIGTERM) == 0) { debug("Terminating child %d (kill)", request.terminate); From 2c8fc50c69c23d4c5619f5a00667c5479b9bb272 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Thu, 3 Jan 2013 04:01:53 -0500 Subject: [PATCH 4/6] Set SSL options for increased security Disable SSLv2, SSLv3, and compression; generate new DH or ECDH keys during each handshake; always start a new session on server renegotiation; set a strong cipher list. Signed-off-by: Anders Kaseorg [ Patch from https://code.google.com/p/shellinabox/issues/detail?id=215 ] --- libhttp/ssl.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/libhttp/ssl.c b/libhttp/ssl.c index d2788dd..c932deb 100644 --- a/libhttp/ssl.c +++ b/libhttp/ssl.c @@ -583,6 +583,27 @@ static int sslSetCertificateFromFile(SSL_CTX *context, } #endif +static SSL_CTX *sslMakeContext(void) { + SSL_CTX *context; + check(context = SSL_CTX_new(SSLv23_server_method())); + SSL_CTX_set_options(context, SSL_OP_ALL); + SSL_CTX_set_options(context, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3); +#ifdef SSL_OP_NO_COMPRESSION + SSL_CTX_set_options(context, SSL_OP_NO_COMPRESSION); +#elif OPENSSL_VERSION_NUMBER >= 0x00908000L + sk_SSL_COMP_zero(SSL_COMP_get_compression_methods()); +#endif + SSL_CTX_set_options(context, SSL_OP_SINGLE_DH_USE); +#ifdef SSL_OP_SINGLE_ECDH_USE + SSL_CTX_set_options(context, SSL_OP_SINGLE_ECDH_USE); +#endif +#ifdef SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION + SSL_CTX_set_options(context, SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION); +#endif + check(SSL_CTX_set_cipher_list(context, "HIGH:MEDIUM:!aNULL:!MD5")); + return context; +} + #ifdef HAVE_TLSEXT static int sslSNICallback(SSL *sslHndl, int *al ATTR_UNUSED, struct SSLSupport *ssl) { @@ -619,7 +640,7 @@ static int sslSNICallback(SSL *sslHndl, int *al ATTR_UNUSED, serverName+1, NULL); if (context == NULL) { - check(context = SSL_CTX_new(SSLv23_server_method())); + context = sslMakeContext(); check(ssl->sniCertificatePattern); char *certificate = stringPrintfUnchecked(NULL, ssl->sniCertificatePattern, @@ -697,7 +718,7 @@ void sslSetCertificate(struct SSLSupport *ssl, const char *filename, } // Try to set the default certificate. If necessary, (re-)generate it. - check(ssl->sslContext = SSL_CTX_new(SSLv23_server_method())); + ssl->sslContext = sslMakeContext(); if (autoGenerateMissing) { if (sslSetCertificateFromFile(ssl->sslContext, defaultCertificate) < 0) { char hostname[256], buf[4096]; @@ -781,7 +802,7 @@ static char *sslFdToFilename(int fd) { void sslSetCertificateFd(struct SSLSupport *ssl, int fd) { #ifdef HAVE_OPENSSL - check(ssl->sslContext = SSL_CTX_new(SSLv23_server_method())); + ssl->sslContext = sslMakeContext(); char *filename = sslFdToFilename(fd); if (!sslSetCertificateFromFd(ssl->sslContext, fd)) { fatal("Cannot read valid certificate from %s. Check file format.", From 01be2490ea51ef595d9fcde5d07c3bba2e487f4c Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Wed, 10 Dec 2014 20:05:00 +0000 Subject: [PATCH 5/6] Add dynamic linking for functions required by SSL v2/3 disabling patch. --- libhttp/ssl.c | 16 +++++++++++++++- libhttp/ssl.h | 6 ++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/libhttp/ssl.c b/libhttp/ssl.c index c932deb..b96f9d9 100644 --- a/libhttp/ssl.c +++ b/libhttp/ssl.c @@ -136,6 +136,9 @@ int (*SSL_write)(SSL *, const void *, int); SSL_METHOD * (*SSLv23_server_method)(void); X509 * (*d2i_X509)(X509 **px, const unsigned char **in, int len); void (*X509_free)(X509 *a); +int (*x_SSL_CTX_set_cipher_list)(SSL_CTX *ctx, const char *str); +void (*x_sk_zero)(void *st); +void * (*x_SSL_COMP_get_compression_methods)(void); #endif static void sslDestroyCachedContext(void *ssl_, char *context_) { @@ -308,7 +311,9 @@ static void loadSSL(void) { { { &SSL_write }, "SSL_write" }, { { &SSLv23_server_method }, "SSLv23_server_method" }, { { &d2i_X509 }, "d2i_X509" }, - { { &X509_free }, "X509_free" } + { { &X509_free }, "X509_free" }, + { { &x_SSL_CTX_set_cipher_list }, "SSL_CTX_set_cipher_list" }, + { { &x_sk_zero }, "sk_zero" } }; for (unsigned i = 0; i < sizeof(symbols)/sizeof(symbols[0]); i++) { if (!(*symbols[i].var = loadSymbol(path_libssl, symbols[i].fn))) { @@ -320,6 +325,10 @@ static void loadSSL(void) { return; } } + // These are optional + x_SSL_COMP_get_compression_methods = loadSymbol(path_libssl, "SSL_COMP_get_compression_methods"); + // ends + SSL_library_init(); dcheck(!ERR_peek_error()); debug("Loaded SSL suppport"); @@ -590,6 +599,11 @@ static SSL_CTX *sslMakeContext(void) { SSL_CTX_set_options(context, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3); #ifdef SSL_OP_NO_COMPRESSION SSL_CTX_set_options(context, SSL_OP_NO_COMPRESSION); +#endif +#if defined(HAVE_DLOPEN) + if (SSL_COMP_get_compression_methods) { + sk_SSL_COMP_zero(SSL_COMP_get_compression_methods()); + } #elif OPENSSL_VERSION_NUMBER >= 0x00908000L sk_SSL_COMP_zero(SSL_COMP_get_compression_methods()); #endif diff --git a/libhttp/ssl.h b/libhttp/ssl.h index 62d585b..e1477ab 100644 --- a/libhttp/ssl.h +++ b/libhttp/ssl.h @@ -111,6 +111,9 @@ extern int (*x_SSL_write)(SSL *, const void *, int); extern SSL_METHOD *(*x_SSLv23_server_method)(void); extern X509 * (*x_d2i_X509)(X509 **px, const unsigned char **in, int len); extern void (*x_X509_free)(X509 *a); +extern int (*x_SSL_CTX_set_cipher_list)(SSL_CTX *ctx, const char *str); +extern void (*x_sk_zero)(void *st); +extern void *(*x_SSL_COMP_get_compression_methods)(void); #define BIO_ctrl x_BIO_ctrl #define BIO_f_buffer x_BIO_f_buffer @@ -151,6 +154,9 @@ extern void (*x_X509_free)(X509 *a); #define SSLv23_server_method x_SSLv23_server_method #define d2i_X509 x_d2i_X509 #define X509_free x_X509_free +#define SSL_CTX_set_cipher_list x_SSL_CTX_set_cipher_list +#define sk_zero x_sk_zero +#define SSL_COMP_get_compression_methods x_SSL_COMP_get_compression_methods #undef BIO_set_buffer_read_data #undef SSL_CTX_set_tlsext_servername_arg From ee0b1de7a846bdddbee63d00638e13acbcee845f Mon Sep 17 00:00:00 2001 From: KLuka Date: Wed, 28 Jan 2015 20:54:56 +0100 Subject: [PATCH 6/6] =?UTF-8?q?IE=2011=20-=20This=20page=20can=E2=80=99t?= =?UTF-8?q?=20be=20displayed=20(Issue=20#262)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now we are able to identify IE11 as MSIE browser and disable compresion. Patch taken from issue comments. https://code.google.com/p/shellinabox/issues/detail?id=262 --- libhttp/httpconnection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libhttp/httpconnection.c b/libhttp/httpconnection.c index a19ac3f..521d610 100644 --- a/libhttp/httpconnection.c +++ b/libhttp/httpconnection.c @@ -568,7 +568,7 @@ void httpTransfer(struct HttpConnection *http, char *msg, int len) { // also has difficulties with SSL connections that are being proxied. int ieBug = 0; const char *userAgent = getFromHashMap(&http->header, "user-agent"); - const char *msie = userAgent ? strstr(userAgent, "MSIE ") : NULL; + const char *msie = userAgent ? strstr(userAgent, "Trident") : NULL; if (msie) { ieBug++; }