Fixed a security problem where the custom snprintf() would always be used. Thanks...

[betaftpd] / ftpd.c

diff --git a/ftpd.c b/ftpd.c
index d3587815bd790c595ddd9d107f10601bd90612f2..f013777ec65dcb39a3660f5234baa79a143fe005 100644 (file)
--- a/ftpd.c
+++ b/ftpd.c
@@ -184,6 +184,7 @@ struct ftran *first_ftran = NULL;
 #if WANT_DCACHE
 struct dcache *first_dcache = NULL;
 #endif
+char message_buf[512];
 
 #if HAVE_POLL
 unsigned int highest_fds = 0;
@@ -214,7 +215,7 @@ int sendfile_supported = 1;
  */
 int time_to_check = 1;
 
-#ifndef HAVE_SPRINTF
+#ifndef HAVE_SNPRINTF
 /*
  * snprintf(): snprintf() replacement for systems that miss it. Note
  *             that this implementation does _not_ necessarily protect
@@ -365,7 +366,7 @@ struct conn *alloc_new_conn(const int sock)
        const unsigned int one = 1;
        struct conn *c = (struct conn *)(malloc(sizeof(struct conn)));
 
-       if (c == NULL) return c;
+       if (c == NULL) return NULL;
 
        if (sock != -1) {
                ioctl(sock, FIONBIO, &one);
@@ -390,6 +391,7 @@ struct conn *alloc_new_conn(const int sock)
 #if WANT_ASCII
        c->ascii_mode = 0;
 #endif
+       c->free_me = 0;
 
        /*
         * equals:
@@ -406,7 +408,7 @@ struct conn *alloc_new_conn(const int sock)
 
        time(&(c->last_transfer));
 
-       /*list_clients();*/
+       /* list_clients(); */
 
        return c;
 }
@@ -444,6 +446,9 @@ struct ftran *alloc_new_ftran(const int sock, const struct conn * const c)
 #endif
 
        f->dir_listing = 0;
+#if WANT_UPLOAD
+       f->upload = 0;
+#endif
        return f;
 }
 
@@ -581,7 +586,7 @@ int process_all_clients(const fd_set * const active_clients, const int num_ac)
                c->buf_len += bytes_avail;
                parse_command(c);
 
-               if (fds[c->sock].revents & (POLLERR|POLLHUP|POLLNVAL)) {
+               if (c->free_me || (fds[c->sock].revents & (POLLERR|POLLHUP|POLLNVAL))) {
                         destroy_conn(c);
                 }
        }
@@ -596,7 +601,12 @@ int process_all_clients(const fd_set * const active_clients, const int num_ac)
  */
 void finish_transfer(struct ftran * const f)
 {
-       numeric(f->owner, 226, "Transfer complete.");
+       char finished[] = "226 Transfer complete.\r\n";
+               if (send(f->owner->sock, finished, strlen(finished), 0) == -1 && errno == EPIPE) {
+               destroy_conn(f->owner);
+               return;
+       }
+       
        time(&(f->owner->last_transfer));
 
 #if WANT_XFERLOG
@@ -625,15 +635,14 @@ int process_all_sendfiles(fd_set * const active_clients, const int num_ac)
 {
        struct ftran *f = NULL, *next = first_ftran->next_ftran;
        int checked_through = 0;
-       struct sockaddr tempaddr;
-       int tempaddr_len = sizeof(tempaddr);
+       int tempaddr_len = sizeof(struct sockaddr_in);
  
        while (next != NULL && checked_through < num_ac) {
                f = next;
                next = f->next_ftran;
 
-#if HAVE_UPLOAD
-               if (f->upload == 1 && fds[f->sock].revents & POLLHUP) {
+#if WANT_UPLOAD
+               if ((f->upload == 1) && (fds[f->sock].revents & POLLHUP)) {
                        finish_transfer(f);
                        continue;
                }
@@ -665,8 +674,7 @@ int process_all_sendfiles(fd_set * const active_clients, const int num_ac)
 
                if (f->state == 2) {            /* incoming PASV */
                        const unsigned int one = 1;
-                       const int tempsock = accept(f->sock, (struct sockaddr *)&tempaddr,
-                                                       &tempaddr_len);
+                       const int tempsock = accept(f->sock, &(f->sin), &tempaddr_len);
 
                        del_fd(f->sock);
 
@@ -678,12 +686,25 @@ int process_all_sendfiles(fd_set * const active_clients, const int num_ac)
                        f->sock = tempsock;
                        ioctl(f->sock, FIONBIO, &one);
                        init_file_transfer(f);
+                       
+                       flush_numeric(f->owner);
+                       if (f->owner->free_me) {
+                               destroy_conn(f->owner);
+                               continue;
+                       }
+       
 #if WANT_UPLOAD
                        if (f->upload) continue;
 #endif
                }
                if (f->state < 5) {
                        init_file_transfer(f);
+                       
+                       flush_numeric(f->owner);
+                       if (f->owner->free_me) {
+                               destroy_conn(f->owner);
+                               continue;
+                       }
 #if WANT_UPLOAD
                        if (f->upload) continue;
 #endif
@@ -741,7 +762,7 @@ int do_upload(struct ftran *f)
 #endif
        if (size > 0 && (write(f->local_file, upload_buf, size) == size)) {
                return 1;
-       } else if (size == -1) {
+       } else if (size == -1 && errno != EAGAIN) {
                /* don't write xferlog... or? */
                numeric(f->owner, 426, strerror(errno));
                destroy_ftran(f);
@@ -1119,10 +1140,14 @@ void accept_new_client(int * const server_sock)
                struct conn * const c = alloc_new_conn(tempsock);
                num_err = 0;
                if (c != NULL) {
-                       numeric(c, 220, "BetaFTPD " VERSION " ready.");
+                       char hello[] = "220 BetaFTPD " VERSION " ready.\r\n";
+                       
 #if WANT_STAT
                        memcpy(&(c->addr), &tempaddr, sizeof(struct sockaddr));
 #endif
+
+                       if (send(tempsock, hello, strlen(hello), 0) == -1 && errno == EPIPE)
+                               destroy_conn(c);
                }
        }
 }
@@ -1194,23 +1219,36 @@ void remove_bytes(struct conn * const c, const int num)
  *             you can use this command much the same way as you
  *             would use a printf() (with all the normal %s, %d,
  *             etc.), since it actually uses printf() internally.
+ *
+ *             This command doesn't actually SEND the data -- it
+ *             just puts it in a buffer which is sent after the
+ *             command handler has completed. The reasons for this
+ *             are simple -- it makes error checking and cleanup
+ *             MUCH cleaner, so we won't have to check for errors
+ *             in every single little branch of the code.
  */
 void numeric(struct conn * const c, const int numeric, const char * const format, ...)
 {
-       char buf[256], fmt[256];
+       char fmt[256];
        va_list args;
-       int i, err;
+       int i;
+       int in_buf = strlen(message_buf);
 
        snprintf(fmt, 256, "%03u %s\r\n", numeric, format);
 
        va_start(args, format);
-       i = vsnprintf(buf, 256, fmt, args);
+       i = vsnprintf(message_buf + in_buf, 512 - in_buf, fmt, args);
        va_end(args);
+}
 
-       err = send(c->sock, buf, i, 0);
-       if (err == -1 && errno == EPIPE) {
-               destroy_conn(c);
-       }
+/* flush_numeric():
+ *             Actually flushes the buffer written by numeric() -- but does
+ *             NOT erase it. If an error, sets the "free_me" flag in the socket.
+ */
+void flush_numeric(struct conn * const c)
+{
+       if (send(c->sock, message_buf, strlen(message_buf), 0) == -1 && errno == EPIPE)
+               c->free_me = 1;
 }
 
 /*