Fix: race conditions / stack overflow on FreeBSD / minor bugs

Dockerfile.busybox

@@ -40,7 +40,7 @@ COPY --from=wolfssl /usr/local /usr/local
 COPY . 3proxy
 RUN cd 3proxy && mkdir -p bin &&\
  make -f Makefile.Linux LIBSTATIC=true PAM_CHECK=false WOLFSSL_CHECK=true \
-      LDFLAGS="-L/usr/local/lib" &&\
+      EXTRA_LDFLAGS="-L/usr/local/lib" &&\
  strip bin/3proxy &&\
  strip bin/*so &&\
  mkdir /dist &&\

Dockerfile.minimal

@@ -46,7 +46,7 @@ COPY --from=wolfssl /usr/local /usr/local
 COPY . 3proxy
 RUN cd 3proxy && mkdir -p bin &&\
  make -f Makefile.Linux STATIC=true WOLFSSL_CHECK=true \
-      LDFLAGS="-static -L/usr/local/lib" &&\
+      EXTRA_LDFLAGS="-L/usr/local/lib" &&\
  strip bin/3proxy
 
 FROM scratch

Makefile.FreeBSD

@@ -15,7 +15,12 @@ CFLAGS += -c -fno-strict-aliasing -DFD_SETSIZE=4096 -DWITH_POLL -DWITH_UN
 COUT = -o 
 LN ?= ${CC}
 LDFLAGS ?= -O3 -flto
-LDFLAGS += -pthread -fno-strict-aliasing 
+LDFLAGS += -pthread -fno-strict-aliasing
+# Use EXTRA_CFLAGS/EXTRA_LDFLAGS to add flags from the make command line.
+# Setting CFLAGS or LDFLAGS there instead overrides every assignment in this
+# makefile, including the += above and the STATIC/LIBSTATIC handling below.
+CFLAGS += $(EXTRA_CFLAGS)
+LDFLAGS += $(EXTRA_LDFLAGS)
 # -lpthreads may be reuiured on some platforms instead of -pthreads
 # -ldl or -lld may be required for some platforms
 DCFLAGS ?= -fPIC

Makefile.Linux

@@ -16,6 +16,11 @@ LN ?= ${CC}
 DCFLAGS ?= -fPIC
 LDFLAGS ?= -O3 -flto
 LDFLAGS += -fno-strict-aliasing -pthread
+# Use EXTRA_CFLAGS/EXTRA_LDFLAGS to add flags from the make command line.
+# Setting CFLAGS or LDFLAGS there instead overrides every assignment in this
+# makefile, including the += above and the STATIC/LIBSTATIC handling below.
+CFLAGS += $(EXTRA_CFLAGS)
+LDFLAGS += $(EXTRA_LDFLAGS)
 DLFLAGS ?= -shared
 DLSUFFICS = .ld.so
 # -lpthreads may be reuqired on some platforms instead of -pthreads

Makefile.unix

@@ -18,6 +18,11 @@ COUT = -o
 LN ?= $(CC)
 LDFLAGS ?= -O3 -flto
 LDFLAGS += -pthread -fno-strict-aliasing
+# Use EXTRA_CFLAGS/EXTRA_LDFLAGS to add flags from the make command line.
+# Setting CFLAGS or LDFLAGS there instead overrides every assignment in this
+# makefile, including the += above and the STATIC/LIBSTATIC handling below.
+CFLAGS += $(EXTRA_CFLAGS)
+LDFLAGS += $(EXTRA_LDFLAGS)
 # -lpthreads may be reuqired on some platforms instead of -pthreads
 # -ldl or -lld may be required for some platforms
 DCFLAGS ?= -fPIC

Makefile.win

@@ -15,6 +15,11 @@ COUT = -o
 LN ?= $(CC)
 LDFLAGS ?= -O3 -flto
 LDFLAGS += -fno-strict-aliasing -mthreads
+# Use EXTRA_CFLAGS/EXTRA_LDFLAGS to add flags from the make command line.
+# Setting CFLAGS or LDFLAGS there instead overrides every assignment in this
+# makefile, including the += above and the STATIC/LIBSTATIC handling below.
+CFLAGS += $(EXTRA_CFLAGS)
+LDFLAGS += $(EXTRA_LDFLAGS)
 DLFLAGS ?= -shared
 DLSUFFICS = .dll
 LIBS += -lws2_32 -lodbc32 -ladvapi32 -luser32 -lbcrypt

man/3proxy.cfg.5

@@ -1178,6 +1178,19 @@ crash on request processing, try to set some positive value. You may start with
 stacksize 65536
 and then find the minimal value for the service to work. If you experience
 memory shortage, you can try to experiment with negative values.
+.br
+ With SQL logging (log &ODBC_string) the value is automatically raised to
+32768 if it is smaller, because ODBC drivers require more stack. A
+.BR stacksize
+command placed after the
+.BR log
+command overrides this.
+.br
+ The base stack size the value is added to is 49152. On FreeBSD, NetBSD,
+OpenBSD and DragonFly it is 65536, because libc functions such as vfprintf()
+called by syslog() use significantly more stack there. The result is never
+lowered below PTHREAD_STACK_MIN, so a large negative value can not disable
+the thread stack.
 
 .SH PLUGINS
 

src/3proxy.c

@@ -519,15 +519,10 @@ int WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPWSTR lpCmdLine, int
 #ifndef NORADIUS
   _3proxy_mutex_init(&rad_mutex);
 #endif
-#ifdef _WIN32
-  conf.threadinit = CreateSemaphore(NULL, 1, 1, NULL);
-  if(!conf.threadinit){
+  if(_3proxy_sem_init(conf.threadinit, 1, 1)){
     fprintf(stderr, "semaphore init failed\n");
     return 1;
   }
-#else
-  _3proxy_mutex_init(&conf.threadinit);
-#endif
 
 #ifdef WITH_SSL
   ssl_install();

src/common.c

@@ -134,12 +134,47 @@ int timeouts[12] = {
 	0
 };
 
-struct extparam conf = {
-#ifdef _WIN32
-	.threadinit = NULL,
-#else
-	.threadinit = 0,
+#ifndef _WIN32
+/* PTHREAD_STACK_MIN is 128K on glibc/aarch64 and glibc/powerpc and may be
+   a sysconf() call with _GNU_SOURCE. pthread_attr_setstacksize() fails with
+   EINVAL below it and the thread silently gets the 8M system default stack.
+ */
+size_t threadstacksize(int extra){
+	long size = BASESTACKSIZE + extra;
+
+	if(size < (long)PTHREAD_STACK_MIN) size = (long)PTHREAD_STACK_MIN;
+	return (size_t)size;
+}
+
+int _3proxy_sem_init_f(_3proxy_sem_t *sem, unsigned count, unsigned maxcount){
+	sem->count = count;
+	sem->maxcount = maxcount;
+	if(pthread_mutex_init(&sem->mutex, NULL)) return 1;
+	if(pthread_cond_init(&sem->cond, NULL)){
+		pthread_mutex_destroy(&sem->mutex);
+		return 1;
+	}
+	return 0;
+}
+
+void _3proxy_sem_lock_f(_3proxy_sem_t *sem){
+	pthread_mutex_lock(&sem->mutex);
+	while(!sem->count) pthread_cond_wait(&sem->cond, &sem->mutex);
+	sem->count--;
+	pthread_mutex_unlock(&sem->mutex);
+}
+
+void _3proxy_sem_unlock_f(_3proxy_sem_t *sem){
+	pthread_mutex_lock(&sem->mutex);
+	if(sem->count < sem->maxcount){
+		sem->count++;
+		pthread_cond_signal(&sem->cond);
+	}
+	pthread_mutex_unlock(&sem->mutex);
+}
 #endif
+
+struct extparam conf = {
 	.timeouts = timeouts,
 	.acl = NULL,
 	.conffile = NULL,

src/conf.c

@@ -156,7 +156,7 @@ int start_proxy_thread(struct child * chp){
 	if(h)CloseHandle(h);
 #else
 	pthread_attr_init(&pa);
-	pthread_attr_setstacksize(&pa,PTHREAD_STACK_MIN + (32768+conf.stacksize));
+	pthread_attr_setstacksize(&pa,threadstacksize(conf.stacksize));
 	pthread_attr_setdetachstate(&pa,PTHREAD_CREATE_DETACHED);
 	pthread_create(&thread, &pa, startsrv, (void *)chp);
 	pthread_attr_destroy(&pa);
@@ -301,7 +301,13 @@ static int h_external(int argc, unsigned char ** argv){
 }
 
 
-static int h_log(int argc, unsigned char ** argv){ 
+/* ODBC drivers require noticeably more stack than the file logger, raise
+   the client thread stack size unless a larger one is configured
+   explicitly. An explicit stacksize placed after the log command still wins.
+ */
+#define LOGSTACKSIZE 32768
+
+static int h_log(int argc, unsigned char ** argv){
 	unsigned char tmpbuf[8192];
 	int notchanged = 0;
 
@@ -330,6 +336,7 @@ static int h_log(int argc, unsigned char ** argv){
 #ifdef WITH_ODBC
 		else if(*argv[1]=='&'){
 			conf.logfunc = logsql;
+			if(conf.stacksize < LOGSTACKSIZE) conf.stacksize = LOGSTACKSIZE;
 			if(notchanged) return 0;
 			_3proxy_mutex_lock(&log_mutex);
 			close_sql();

src/proxy.h

@@ -112,6 +112,21 @@ void daemonize(void);
 #endif
 #endif
 
+/* Thread stack size, stacksize command value is added to it. BSD libc uses
+   significantly more stack, e.g. in vfprintf() called by syslog().
+ */
+#ifndef BASESTACKSIZE
+#if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__)
+#define BASESTACKSIZE 65536
+#else
+#define BASESTACKSIZE 49152
+#endif
+#endif
+
+#ifndef _WIN32
+size_t threadstacksize(int extra);
+#endif
+
 #ifdef WITH_ODBC
 #ifndef _WIN32
 #include <sqltypes.h>
@@ -248,14 +263,18 @@ unsigned char* en64 (const unsigned char *in, unsigned char *out, int inlen);
 void tohex(unsigned char *in, unsigned char *out, int len);
 void fromhex(unsigned char *in, unsigned char *out, int len);
 
+extern _3proxy_sem_t udpinit;
 #ifdef _WIN32
-extern HANDLE udpinit;
+#define _3proxy_sem_init(x, count, maxcount) (((x) = CreateSemaphore(NULL, (count), (maxcount), NULL))? 0 : 1)
 #define _3proxy_sem_lock(x) WaitForSingleObject(x, INFINITE)
 #define _3proxy_sem_unlock(x) ReleaseSemaphore(x, 1, NULL)
 #else
-extern _3proxy_mutex_t udpinit;
-#define _3proxy_sem_lock(x) pthread_mutex_lock(&x)
-#define _3proxy_sem_unlock(x) pthread_mutex_unlock(&x)
+int _3proxy_sem_init_f(_3proxy_sem_t *sem, unsigned count, unsigned maxcount);
+void _3proxy_sem_lock_f(_3proxy_sem_t *sem);
+void _3proxy_sem_unlock_f(_3proxy_sem_t *sem);
+#define _3proxy_sem_init(x, count, maxcount) _3proxy_sem_init_f(&x, (count), (maxcount))
+#define _3proxy_sem_lock(x) _3proxy_sem_lock_f(&x)
+#define _3proxy_sem_unlock(x) _3proxy_sem_unlock_f(&x)
 #endif
 
 

src/proxymain.c

@@ -11,6 +11,18 @@
 #include <sched.h>
 #endif
 
+/* Child functions do not call each other, a child requesting redirection to
+   another child returns it instead of calling it, to keep the stack flat.
+   NULL is returned by the child which completed the request, it has already
+   released param and it must not be accessed after the call.
+ */
+void * childfunc(struct clientparam * param){
+ PROXYFUNC pf = param->srv->pf;
+
+ while(pf) pf = (PROXYFUNC)(*pf)(param);
+ return NULL;
+}
+
 #define param ((struct clientparam *) p)
 #ifdef _WIN32
 DWORD WINAPI threadfunc(LPVOID p) {
@@ -367,6 +379,7 @@ int MODULEMAINFUNC (int argc, char** argv){
 	if(!srv.udpbuf || !srv.udpbuf2) {
 #ifndef STDMAIN
 		haveerror = 2;
+		_3proxy_sem_unlock(conf.threadinit);
 #endif
 		return 11;
 	}
@@ -608,7 +621,6 @@ int MODULEMAINFUNC (int argc, char** argv){
 	if (error || i!=argc) {
 #ifndef STDMAIN
 		haveerror = 1;
-		_3proxy_sem_unlock(conf.threadinit);
 #endif
 		fprintf(stderr, "%s of %s\n"
 			"Usage: %s options\n"
@@ -628,6 +640,9 @@ int MODULEMAINFUNC (int argc, char** argv){
 			""
 #endif
 		);
+#ifndef STDMAIN
+		_3proxy_sem_unlock(conf.threadinit);
+#endif
 
 		return (1);
 	}
@@ -640,7 +655,6 @@ int MODULEMAINFUNC (int argc, char** argv){
 	if (error || argc != i+3 || *argv[i]=='-'|| (*SAPORT(&srv.intsa) = htons((uint16_t)atoi(argv[i])))==0 || (srv.targetport = htons((uint16_t)atoi(argv[i+2])))==0) {
 #ifndef STDMAIN
 		haveerror = 1;
-		_3proxy_sem_unlock(conf.threadinit);
 #endif
 		fprintf(stderr, "%s of %s\n"
 			"Usage: %s options"
@@ -661,6 +675,9 @@ int MODULEMAINFUNC (int argc, char** argv){
 			""
 #endif
 		);
+#ifndef STDMAIN
+		_3proxy_sem_unlock(conf.threadinit);
+#endif
 		return (1);
 	}
 	srv.target = (unsigned char *)strdup(argv[i+1]);
@@ -917,7 +934,7 @@ int MODULEMAINFUNC (int argc, char** argv){
  
 #ifndef _WIN32
  pthread_attr_init(&pa);
- pthread_attr_setstacksize(&pa,PTHREAD_STACK_MIN + (32768 + srv.stacksize));
+ pthread_attr_setstacksize(&pa,threadstacksize(srv.stacksize));
  pthread_attr_setdetachstate(&pa,PTHREAD_CREATE_DETACHED);
 #endif
 
@@ -1051,9 +1068,12 @@ int MODULEMAINFUNC (int argc, char** argv){
 	else {
 		struct clientparam *toparam;
 
-		srv.udplen = sockrecvfrom(NULL, srv.srvsock, (struct sockaddr *)&defparam.sincr, srv.udpbuf, UDPBUFSIZE, 0);
-		if(srv.udplen <= 0) continue;
 		_3proxy_sem_lock(udpinit);
+		srv.udplen = sockrecvfrom(NULL, srv.srvsock, (struct sockaddr *)&defparam.sincr, srv.udpbuf, UDPBUFSIZE, 0);
+		if(srv.udplen <= 0) {
+			_3proxy_sem_unlock(udpinit);
+			continue;
+		}
 		if(hashresolv(&udp_table, &defparam, &toparam, NULL)) {
 		    int i, len=0;
 		
@@ -1172,12 +1192,7 @@ int MODULEMAINFUNC (int argc, char** argv){
 
 #ifndef NOUDPMAIN
 int udpinited = 0;
-#ifdef _WIN32
-HANDLE udpinit;
-#else
-_3proxy_mutex_t udpinit;
-#endif
-
+_3proxy_sem_t udpinit;
 #endif
 
 void srvinit(struct srvparam * srv, struct clientparam *param){
@@ -1216,11 +1231,7 @@ void srvinit(struct srvparam * srv, struct clientparam *param){
  _3proxy_mutex_init(&srv->counter_mutex);
 #ifndef NOUDPMAIN
  if(!udpinited){
-#ifdef _WIN32
-    udpinit = CreateSemaphore(NULL, 1, 1, NULL);
-#else
-    _3proxy_mutex_init(&udpinit);
-#endif
+    (void)_3proxy_sem_init(udpinit, 1, 1);
  }
  udpinited = 1;
 #endif

src/ssllib.c

@@ -72,10 +72,11 @@ static size_t bin2hex (const unsigned char* bin, size_t bin_length, char* str, s
 	return p - str;
 }
 
-static int add_ext(X509 *cert, int nid, char *value)
+static int add_ext(X509 *cert, int nid, const char *value)
 {
 	X509_EXTENSION *ex;
 	X509V3_CTX ctx;
+	int err;
 	/* This sets the 'context' of the extensions. */
 	/* No configuration database */
 	X509V3_set_ctx_nodb(&ctx);
@@ -83,13 +84,14 @@ static int add_ext(X509 *cert, int nid, char *value)
 	 * no request and no CRL
 	 */
 	X509V3_set_ctx(&ctx, cert, cert, NULL, NULL, 0);
-	ex = X509V3_EXT_conf_nid(NULL, &ctx, nid, value);
+	/* value is char * prior to OpenSSL 1.1.0 */
+	ex = X509V3_EXT_conf_nid(NULL, &ctx, nid, (char *)value);
 	if (!ex)
 		return 0;
 
-	X509_add_ext(cert,ex,-1);
+	err = X509_add_ext(cert,ex,-1);
 	X509_EXTENSION_free(ex);
-	return 1;
+	return err > 0;
 }
 
 SSL_CERT ssl_copy_cert(SSL_CERT cert, SSL_CONFIG *config)
@@ -138,9 +140,10 @@ SSL_CERT ssl_copy_cert(SSL_CERT cert, SSL_CONFIG *config)
 	if ( dst_cert == NULL ) {
 		return NULL;
 	}
-	X509_set_version(dst_cert, X509_get_version(src_cert));
-	X509_set_serialNumber(dst_cert, X509_get_serialNumber(src_cert));
-	if(!X509_set_subject_name(dst_cert, X509_get_subject_name(src_cert))
+	/* v3 is required, extensions are added below */
+	X509_set_version(dst_cert, 2);
+	if(!X509_set_serialNumber(dst_cert, X509_get_serialNumber(src_cert))
+	|| !X509_set_subject_name(dst_cert, X509_get_subject_name(src_cert))
 	|| !X509_set_issuer_name(dst_cert, X509_get_subject_name(config->CA_cert))){
 		X509_free(dst_cert);
 		return NULL;
@@ -150,13 +153,35 @@ SSL_CERT ssl_copy_cert(SSL_CERT cert, SSL_CONFIG *config)
 		X509_free(dst_cert);
 		return NULL;
 	}
-	X509_set_notBefore(dst_cert, X509_get_notBefore(src_cert));
-	X509_set_notAfter(dst_cert, X509_get_notAfter(src_cert));
+#if !defined(WITH_WOLFSSL) && OPENSSL_VERSION_NUMBER < 0x10100000L
+	if(!X509_set_notBefore(dst_cert, X509_get_notBefore(src_cert))
+	|| !X509_set_notAfter(dst_cert, X509_get_notAfter(src_cert))){
+#else
+	if(!X509_set1_notBefore(dst_cert, X509_get0_notBefore(src_cert))
+	|| !X509_set1_notAfter(dst_cert, X509_get0_notAfter(src_cert))){
+#endif
+		X509_free(dst_cert);
+		return NULL;
+	}
 	san_idx = X509_get_ext_by_NID(src_cert, NID_subject_alt_name, -1);
 	if(san_idx >= 0){
 	    X509_EXTENSION *san;
 	    san = X509_get_ext(src_cert, san_idx);
-	    if(san) X509_add_ext(dst_cert, san, -1);
+	    if(san && !X509_add_ext(dst_cert, san, -1)){
+		X509_free(dst_cert);
+		return NULL;
+	    }
+	}
+	/* Extensions required from an end entity certificate. Without EKU
+	 * serverAuth Apple's TLS stack (and Chrome on macOS/iOS, which uses it)
+	 * rejects the certificate. keyUsage is intentionally not set: it depends
+	 * on the type of the key being reused for every generated certificate,
+	 * and an absent keyUsage places no restriction.
+	 */
+	if(!add_ext(dst_cert, NID_basic_constraints, "critical,CA:FALSE")
+	|| !add_ext(dst_cert, NID_ext_key_usage, "serverAuth")){
+		X509_free(dst_cert);
+		return NULL;
 	}
 	err = X509_sign(dst_cert, config->CA_key, EVP_sha256());
 	if(!err){

src/structures.h

@@ -42,6 +42,12 @@ extern "C" {
 #define _3proxy_mutex_destroy pthread_mutex_destroy
 #define _3proxy_mutex_lock pthread_mutex_lock
 #define _3proxy_mutex_unlock pthread_mutex_unlock
+typedef struct _3proxy_sem_s {
+	pthread_mutex_t mutex;
+	pthread_cond_t cond;
+	unsigned count;
+	unsigned maxcount;
+} _3proxy_sem_t;
 #else
 #include <winsock2.h>
 #include <ws2tcpip.h>
@@ -50,6 +56,7 @@ extern "C" {
 #define _3proxy_mutex_lock(x) EnterCriticalSection(x)
 #define _3proxy_mutex_unlock(x) LeaveCriticalSection(x)
 #define _3proxy_mutex_destroy(x) DeleteCriticalSection(x)
+#define _3proxy_sem_t HANDLE
 #ifdef MSVC
 #pragma warning (disable : 4996)
 #endif
@@ -682,11 +689,7 @@ struct filemon {
 
 
 struct extparam {
-#ifdef _WIN32
-	HANDLE threadinit;
-#else
-	_3proxy_mutex_t threadinit;
-#endif
+	_3proxy_sem_t threadinit;
 	int *timeouts;
 	struct ace * acl;
 	char * conffile;

src/tlspr.c

@@ -190,7 +190,7 @@ int clistarttls(struct clientparam *param, PROXYSERVICE proto){
 			return 0;
 		}
 		if(!strncasecmp((char *)buf, "EHLO ", 5)){
-			socksend(param, param->clisock, (unsigned char *)"250-Proxy\r\n250 STARTTLS\r\n", 24, conf.timeouts[STRING_S]);
+			socksend(param, param->clisock, (unsigned char *)"250-Proxy\r\n250 STARTTLS\r\n", 25, conf.timeouts[STRING_S]);
 			continue;
 		}
 		if(!strncasecmp((char *)buf, "HELO ", 5)){
@@ -201,8 +201,10 @@ int clistarttls(struct clientparam *param, PROXYSERVICE proto){
 			socksend(param, param->clisock, (unsigned char *)"221 Proxy\r\n", 11, conf.timeouts[STRING_S]);
 			return -1;
 		}
-		socksend(param, param->clisock, (unsigned char *)"530 5.7.0 Must issue a STARTTLS command first\r\n", 45, conf.timeouts[STRING_S]);
+		socksend(param, param->clisock, (unsigned char *)"530 5.7.0 Must issue a STARTTLS command first\r\n", 47, conf.timeouts[STRING_S]);
 	}
+ default:
+	break;
  }
  return 1;
 }