[stunnel-users] fix resource leaks and potential null dereferences in 4.54

Arthur Mesh arthurmesh at gmail.com
Wed Jan 30 01:36:24 CET 2013


Hi,

I am trying to figure out where bug reports should be submitted. 
https://www.stunnel.org/lists.html references this mailing list, so here
is my attempt at a bug report with a potential fix. If this is not an
appropriate list, please redirect me.

A coverity run has uncovered quite a few memory and other resource
leaks, plus some potential NULL dereferences. Inline is an attempt to
fix them. Imho, these are mostly self-explanatory, but if someone needs
more details, I would be happy to provide those as well.

Thanks

diff -ru stunnel-4.54/src/options.c stunnel-4.54.p/src/options.c
--- stunnel-4.54/src/options.c	2012-10-09 01:50:54.000000000 -0700
+++ stunnel-4.54.p/src/options.c	2013-01-29 16:26:00.000000000 -0800
@@ -1988,7 +1988,7 @@
 
 int parse_conf(char *name, CONF_TYPE type) {
     DISK_FILE *df;
-    char line_text[CONFLINELEN], *errstr;
+    char line_text[CONFLINELEN], *errstr = NULL;
     char config_line[CONFLINELEN], *config_opt, *config_arg;
     int line_number, i;
     SERVICE_OPTIONS *section, *new_section;
@@ -2104,7 +2104,8 @@
         errstr=parse_service_option(CMD_END, section, NULL, NULL);
     }
     if(errstr) {
-        s_log(LOG_ERR, "Service [%s]: %s", section->servname, errstr);
+        s_log(LOG_ERR, "Service [%s]: %s", section ? section->servname : "",
+              errstr);
         return 1;
     }
 
@@ -2253,7 +2254,7 @@
     }
     new_global_options.debug_level=8;    /* illegal level */
     for(fl=levels; fl->name; ++fl) {
-        if(!strcasecmp(fl->name, string)) {
+        if(string && !strcasecmp(fl->name, string)) {
             new_global_options.debug_level=fl->value;
             break;
         }
@@ -2428,6 +2429,7 @@
             if(get_last_socket_error()!=S_ENOPROTOOPT) {
                 s_log(LOG_ERR, "Failed to get %s OS default", ptr->opt_str);
                 sockerror("getsockopt");
+                close(fd);
                 return 1; /* FAILED */
             }
             td=str_dup("write-only");
@@ -2442,6 +2444,7 @@
             ptr->opt_str, ta, tl, tr, td);
         str_free(ta); str_free(tl); str_free(tr); str_free(td);
     }
+    close(fd);
     return 0; /* OK */
 }
 
Only in stunnel-4.54.p/src: options.c.orig
diff -ru stunnel-4.54/src/protocol.c stunnel-4.54.p/src/protocol.c
--- stunnel-4.54/src/protocol.c	2012-10-09 01:46:47.000000000 -0700
+++ stunnel-4.54.p/src/protocol.c	2013-01-29 16:26:00.000000000 -0800
@@ -244,30 +244,36 @@
 /**************************************** smtp */
 
 static void smtp_client(CLI *c) {
-    char *line;
+    char *line = NULL;
 
     do { /* copy multiline greeting */
+        str_free(line); /* okay to str_free(NULL) */
         line=fd_getline(c, c->remote_fd.fd);
         fd_putline(c, c->local_wfd.fd, line);
     } while(isprefix(line, "220-"));
 
     fd_putline(c, c->remote_fd.fd, "EHLO localhost");
     do { /* skip multiline reply */
+        str_free(line);
         line=fd_getline(c, c->remote_fd.fd);
     } while(isprefix(line, "250-"));
     if(!isprefix(line, "250 ")) { /* error */
+        str_free(line);
         s_log(LOG_ERR, "Remote server is not RFC 1425 compliant");
         longjmp(c->err, 1);
     }
 
     fd_putline(c, c->remote_fd.fd, "STARTTLS");
     do { /* skip multiline reply */
+        str_free(line);
         line=fd_getline(c, c->remote_fd.fd);
     } while(isprefix(line, "220-"));
     if(!isprefix(line, "220 ")) { /* error */
+        str_free(line);
         s_log(LOG_ERR, "Remote server is not RFC 2487 compliant");
         longjmp(c->err, 1);
     }
+    str_free(line);
 }
 
 static void smtp_server(CLI *c) {
@@ -290,21 +296,27 @@
     line=fd_getline(c, c->remote_fd.fd);
     if(!isprefix(line, "220")) {
         s_log(LOG_ERR, "Unknown server welcome");
+        str_free(line);
         longjmp(c->err, 1);
     }
     fd_printf(c, c->local_wfd.fd, "%s + stunnel", line);
+    str_free(line);
     line=fd_getline(c, c->local_rfd.fd);
     if(!isprefix(line, "EHLO ")) {
         s_log(LOG_ERR, "Unknown client EHLO");
+        str_free(line);
         longjmp(c->err, 1);
     }
     fd_printf(c, c->local_wfd.fd, "250-%s Welcome", line);
+    str_free(line);
     fd_putline(c, c->local_wfd.fd, "250 STARTTLS");
     line=fd_getline(c, c->local_rfd.fd);
     if(!isprefix(line, "STARTTLS")) {
         s_log(LOG_ERR, "STARTTLS expected");
+        str_free(line);
         longjmp(c->err, 1);
     }
+    str_free(line);
     fd_putline(c, c->local_wfd.fd, "220 Go ahead");
 }
 
@@ -316,15 +328,19 @@
     line=fd_getline(c, c->remote_fd.fd);
     if(!isprefix(line, "+OK ")) {
         s_log(LOG_ERR, "Unknown server welcome");
+        str_free(line);
         longjmp(c->err, 1);
     }
     fd_putline(c, c->local_wfd.fd, line);
+    str_free(line);
     fd_putline(c, c->remote_fd.fd, "STLS");
     line=fd_getline(c, c->remote_fd.fd);
     if(!isprefix(line, "+OK ")) {
         s_log(LOG_ERR, "Server does not support TLS");
+        str_free(line);
         longjmp(c->err, 1);
     }
+    str_free(line);
 }
 
 static void pop3_server(CLI *c) {
@@ -332,17 +348,21 @@
 
     line=fd_getline(c, c->remote_fd.fd);
     fd_printf(c, c->local_wfd.fd, "%s + stunnel", line);
+    str_free(line);
     line=fd_getline(c, c->local_rfd.fd);
     if(isprefix(line, "CAPA")) { /* client wants RFC 2449 extensions */
         fd_putline(c, c->local_wfd.fd, "+OK Stunnel capability list follows");
         fd_putline(c, c->local_wfd.fd, "STLS");
         fd_putline(c, c->local_wfd.fd, ".");
+        str_free(line);
         line=fd_getline(c, c->local_rfd.fd);
     }
     if(!isprefix(line, "STLS")) {
+        str_free(line);
         s_log(LOG_ERR, "Client does not want TLS");
         longjmp(c->err, 1);
     }
+    str_free(line);
     fd_putline(c, c->local_wfd.fd, "+OK Stunnel starts TLS negotiation");
 }
 
@@ -353,18 +373,22 @@
 
     line=fd_getline(c, c->remote_fd.fd);
     if(!isprefix(line, "* OK")) {
+        str_free(line);
         s_log(LOG_ERR, "Unknown server welcome");
         longjmp(c->err, 1);
     }
     fd_putline(c, c->local_wfd.fd, line);
+    str_free(line);
     fd_putline(c, c->remote_fd.fd, "stunnel STARTTLS");
     line=fd_getline(c, c->remote_fd.fd);
     if(!isprefix(line, "stunnel OK")) {
+        str_free(line);
         fd_putline(c, c->local_wfd.fd,
             "* BYE stunnel: Server does not support TLS");
         s_log(LOG_ERR, "Server does not support TLS");
         longjmp(c->err, 2); /* don't reset */
     }
+    str_free(line);
 }
 
 static void imap_server(CLI *c) {
@@ -387,6 +411,7 @@
     /* process server welcome and send it to client */
     line=fd_getline(c, c->remote_fd.fd);
     if(!isprefix(line, "* OK")) {
+        str_free(line);
         s_log(LOG_ERR, "Unknown server welcome");
         longjmp(c->err, 1);
     }
@@ -396,22 +421,29 @@
     if(capa)
         *capa='K'; /* disable CAPABILITY within greeting */
     fd_printf(c, c->local_wfd.fd, "%s (stunnel)", line);
+    str_free(line);
 
     while(1) { /* process client commands */
         line=fd_getline(c, c->local_rfd.fd);
         /* split line into id and tail */
         id=str_dup(line);
         tail=strchr(id, ' ');
-        if(!tail)
+        if(!tail) {
+            str_free(line);
+            str_free(id);
             break;
+        }
         *tail++='\0';
 
         if(isprefix(tail, "STARTTLS")) {
             fd_printf(c, c->local_wfd.fd,
                 "%s OK Begin TLS negotiation now", id);
+            str_free(line);
+            str_free(id);
             return; /* success */
         } else if(isprefix(tail, "CAPABILITY")) {
             fd_putline(c, c->remote_fd.fd, line); /* send it to server */
+            str_free(line);
             line=fd_getline(c, c->remote_fd.fd); /* get the capabilites */
             if(*line=='*') {
                 /*
@@ -421,6 +453,7 @@
                  * LOGIN would fail as "unexpected command", anyway
                  */
                 fd_printf(c, c->local_wfd.fd, "%s STARTTLS", line);
+                str_free(line);
                 line=fd_getline(c, c->remote_fd.fd); /* next line */
             }
             fd_putline(c, c->local_wfd.fd, line); /* forward to the client */
@@ -429,24 +462,35 @@
                 fd_putline(c, c->local_wfd.fd,
                     "* BYE unexpected server response");
                 s_log(LOG_ERR, "Unexpected server response: %s", line);
+                str_free(line);
+                str_free(id);
                 break;
             }
+            str_free(line);
+            str_free(id);
         } else if(isprefix(tail, "LOGOUT")) {
             fd_putline(c, c->local_wfd.fd, "* BYE server terminating");
             fd_printf(c, c->local_wfd.fd, "%s OK LOGOUT completed", id);
+            str_free(id);
+            str_free(line);
             break;
         } else {
             fd_putline(c, c->local_wfd.fd, "* BYE stunnel: unexpected command");
             fd_printf(c, c->local_wfd.fd, "%s BAD %s unexpected", id, tail);
             s_log(LOG_ERR, "Unexpected client command %s", tail);
+            str_free(line);
+            str_free(id);
             break;
         }
     }
     /* clean server shutdown */
     fd_putline(c, c->remote_fd.fd, "stunnel LOGOUT");
     line=fd_getline(c, c->remote_fd.fd);
-    if(*line=='*')
+    if(*line=='*') {
+        str_free(line);
         line=fd_getline(c, c->remote_fd.fd);
+    }
+    str_free(line);
     longjmp(c->err, 2); /* don't reset */
 }
 
@@ -458,15 +502,19 @@
     line=fd_getline(c, c->remote_fd.fd);
     if(!isprefix(line, "200 ") && !isprefix(line, "201 ")) {
         s_log(LOG_ERR, "Unknown server welcome");
+        str_free(line);
         longjmp(c->err, 1);
     }
     fd_putline(c, c->local_wfd.fd, line);
     fd_putline(c, c->remote_fd.fd, "STARTTLS");
+    str_free(line);
     line=fd_getline(c, c->remote_fd.fd);
     if(!isprefix(line, "382 ")) {
         s_log(LOG_ERR, "Server does not support TLS");
+        str_free(line);
         longjmp(c->err, 1);
     }
+    str_free(line);
 }
 
 /**************************************** connect */
@@ -548,13 +596,17 @@
         /* not "HTTP/1.0 200 Connection established" */
         s_log(LOG_ERR, "CONNECT request rejected");
         do { /* read all headers */
+            str_free(line);
             line=fd_getline(c, c->remote_fd.fd);
         } while(*line);
+        str_free(line);
         longjmp(c->err, 1);
     }
+    str_free(line);
     s_log(LOG_INFO, "CONNECT request accepted");
     do {
         line=fd_getline(c, c->remote_fd.fd); /* read all headers */
+        str_free(line);
     } while(*line);
 }
 
@@ -588,10 +640,13 @@
     if(line[9]!='4' || line[10]!='0' || line[11]!='7') { /* code 407 */
         s_log(LOG_ERR, "NTLM authorization request rejected");
         do { /* read all headers */
+            str_free(line);
             line=fd_getline(c, c->remote_fd.fd);
         } while(*line);
+        str_free(line);
         longjmp(c->err, 1);
     }
+    str_free(line);
     ntlm2_txt=NULL;
     do { /* read all headers */
         line=fd_getline(c, c->remote_fd.fd);
@@ -599,6 +654,7 @@
             ntlm2_txt=str_dup(line+25);
         else if(isprefix(line, "Content-Length: "))
             content_length=atol(line+16);
+        str_free(line);
     } while(*line);
     if(!ntlm2_txt) { /* no Proxy-Authenticate: NTLM header */
         s_log(LOG_ERR, "Proxy-Authenticate: NTLM header not found");
Only in stunnel-4.54.p/src: protocol.c.orig
diff -ru stunnel-4.54/src/stunnel.c stunnel-4.54.p/src/stunnel.c
--- stunnel-4.54/src/stunnel.c	2012-08-18 14:11:53.000000000 -0700
+++ stunnel-4.54.p/src/stunnel.c	2013-01-29 16:26:00.000000000 -0800
@@ -119,12 +119,16 @@
         fatal("Could not open /dev/null");
 #endif /* standard Unix */
     main_initialize();
-    if(main_configure(argc>1 ? argv[1] : NULL, argc>2 ? argv[2] : NULL))
+    if(main_configure(argc>1 ? argv[1] : NULL, argc>2 ? argv[2] : NULL)) {
+        close(fd);
         return 1;
+    }
     if(service_options.next) { /* there are service sections -> daemon mode */
 #if !defined(__vms) && !defined(USE_OS2)
-        if(daemonize(fd))
+        if(daemonize(fd)) {
+            close(fd);
             return 1;
+        }
         close(fd);
         /* create_pid() must be called after drop_privileges()
          * or it won't be possible to remove the file on exit */
Only in stunnel-4.54.p/src: stunnel.c.orig



More information about the stunnel-users mailing list