httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Laurie <...@gonzo.ben.algroup.co.uk>
Subject Re: Possible security problem in referer_log_transaction... ? (fwd)
Date Tue, 19 Dec 1995 13:59:02 GMT
> 
> Yes, there are potential security holes here.
> The following patch fixes all the ones that I can find.
> 
> Many of the uses of MAX_STRING_LEN were in code like this (from http_main.c):
> 
> 	    char errstr[MAX_STRING_LEN];
> 	    sprintf (errstr, "Memory hog alert: allocated %ld bytes for %s",
> 	             bytes_in_pool (ptrans), r->the_request);
>             log_error (errstr, r->server);
> 
> so I've created a log_printf routine (with the code copied from rprintf())
> which removes all these cases; there were many of them in mod_include.c
> 
> This left mod_cookies.c, mod_log_common.c (!) and mod_log_referer.c which I
> fixed using pstrcat(). In the process I deleted a couple of occurrences of
> the following horrible piece of code:
> 
>         sprintf(str,"%s%d ",str,r->status);

Hooray. That made my stomach turn when I added logging for Apache-SSL.

> 
> I've uploaded the patch to the for_Apache_1.0.0 directory.

But - there appears to be a small bugette - marked below (this is from reading
the code, I haven't tried anything brave like compiling it - roll on CVS).

Cheers,

Ben.

> 
>  David.
> 
> P.S. I renamed 69.preserve_redirect.patch to 61a.preserve_redirect.patch
> 
> ------- begin file 69.security.patch
> From: drtr@ast.cam.ac.uk (David Robinson)
> Subject: Fix possible security holes due to stack overwriting
> Affects: http_log.c, http_log.h, http_main.c, mod_cookies.c, mod_include.c,
>          mod_log_common.c, mod_log_referer.c
> ChangeLog: Remove uses of MAX_STRING_LEN/HUGE_STRING_LEN from several routines.
> Comments: This patch was made against the 1.0.1 distribution
> 
> *** http_log.c.orig	Sun Dec 17 14:32:29 1995
> --- http_log.c	Tue Dec 19 11:22:17 1995
> ***************
> *** 64,69 ****
> --- 64,71 ----
>   #include "http_config.h"
>   #include "http_log.h"
>   
> + #include <stdarg.h>
> + 
>   void open_error_log(server_rec *s, pool *p)
>   {
>       char *fname;
> ***************
> *** 119,124 ****
> --- 121,139 ----
>   void log_error(char *err, server_rec *s) {
>       fprintf(s->error_log, "[%s] %s\n",get_time(),err);
>       fflush(s->error_log);
> + }
> + 
> + void
> + log_printf(const server_rec *s, const char *fmt, ...)
> + {
> +     va_list args;
> +     
> +     fprintf(s->error_log, "[%s] ", get_time());
> +     va_start (args, fmt);
> +     vfprintf (s->error_log, fmt, args);
> +     va_end (args);
> + 
> +     fputc('\n', s->error_log);
>   }
>   
>   void log_reason(char *reason, char *file, request_rec *r) {
> *** http_log.h.orig	Sun Dec 17 14:32:28 1995
> --- http_log.h	Tue Dec 19 10:46:56 1995
> ***************
> *** 57,61 ****
> --- 57,62 ----
>   
>   void log_pid (pool *p, char *pid_fname);
>   void log_error(char *err, server_rec *s);
> + void log_printf(const server_rec *s, const char *fmt, ...);
>   void log_reason(char *reason, char *fname, request_rec *r);
>   
> *** http_main.c.orig	Sun Dec 17 14:32:29 1995
> --- http_main.c	Tue Dec 19 10:47:48 1995
> ***************
> *** 815,826 ****
>   	r = read_request (current_conn);
>   	if (r) process_request (r); /* else premature EOF --- ignore */
>   		
> ! 	if (bytes_in_pool (ptrans) > 80000) {
> ! 	    char errstr[MAX_STRING_LEN];
> ! 	    sprintf (errstr, "Memory hog alert: allocated %ld bytes for %s",
> ! 	             bytes_in_pool (ptrans), r->the_request);
> !             log_error (errstr, r->server);
> !         }
>   		
>   	fflush (conn_out);
>   	pfclose (ptrans,conn_in);
> --- 815,824 ----
>   	r = read_request (current_conn);
>   	if (r) process_request (r); /* else premature EOF --- ignore */
>   		
> ! 	if (bytes_in_pool (ptrans) > 80000)
> ! 	    log_printf(r->server,
> ! 		       "Memory hog alert: allocated %ld bytes for %s",
> ! 		       bytes_in_pool (ptrans), r->the_request);
>   		
>   	fflush (conn_out);
>   	pfclose (ptrans,conn_in);
> *** mod_cookies.c.orig	Sun Dec 17 14:32:29 1995
> --- mod_cookies.c	Tue Dec 19 10:37:16 1995
> ***************
> *** 171,177 ****
>   {
>       cookie_log_state *cls = get_module_config (orig->server->module_config,
>                              &cookies_module);
> !     char str[HUGE_STRING_LEN];
>       long timz;
>       struct tm *t;
>       char tstr[MAX_STRING_LEN],sign;
> --- 171,177 ----
>   {
>       cookie_log_state *cls = get_module_config (orig->server->module_config,
>                              &cookies_module);
> !     char *str;
>       long timz;
>       struct tm *t;
>       char tstr[MAX_STRING_LEN],sign;
> ***************
> *** 192,212 ****
>       if(timz < 0) 
>           timz = -timz;
>   
> !     strftime(tstr,MAX_STRING_LEN,"%d/%b/%Y:%H:%M:%S",t);
> ! 
> !     sprintf(str,"%s \"%s\" [%s %c%02ld%02ld] ",
> ! 	    cookie+2,		/* Ignore s= */
> ! 	    orig->the_request,
> !             tstr,
> !             sign,
> !             timz/3600,
> !             timz%3600);
> !     
>       if (r->status != -1)
> !         sprintf(str,"%s%d\n",str,r->status);
> !     else
> !         strcat(str,"-\n");
>   
>       write(cls->log_fd, str, strlen(str));
>   
>       return OK;
> --- 192,207 ----
>       if(timz < 0) 
>           timz = -timz;
>   
> !     strftime(tstr,MAX_STRING_LEN,"\" [%d/%b/%Y:%H:%M:%S ",t);
>       if (r->status != -1)
> ! 	sprintf(&tstr[strlen(tstr)], "%c%02ld%02ld] %d\n", sign, timz/3600,
> ! 		timz%3600, r->status);
> ! 	sprintf(&tstr[strlen(tstr)], "%c%02ld%02ld] -\n", sign, timz/3600,
> ! 		timz%3600);
>   
> + /* ignore s= on cookie */
> +     str = pstrcat(orig->pool, cookie + 2, " \", orig->the_request, tstr);

Huh?? Missing a " here?                          ^

> +     
>       write(cls->log_fd, str, strlen(str));
>   
>       return OK;
> *** mod_include.c.orig	Sun Dec 17 14:32:29 1995
> --- mod_include.c	Tue Dec 19 10:54:50 1995
> ***************
> *** 353,359 ****
>   }
>   
>   int handle_include(FILE *in, request_rec *r, char *error, int noexec) {
> !     char tag[MAX_STRING_LEN],errstr[MAX_STRING_LEN];
>       char *tag_val;
>   
>       while(1) {
> --- 353,359 ----
>   }
>   
>   int handle_include(FILE *in, request_rec *r, char *error, int noexec) {
> !     char tag[MAX_STRING_LEN];
>       char *tag_val;
>   
>       while(1) {
> ***************
> *** 396,403 ****
>   	        error_fmt = "unable to include %s in parsed file %s";
>   		    
>               if (error_fmt) {
> !                 sprintf(errstr, error_fmt, tag_val, r->filename);
> !                 log_error (errstr, r->server);
>                   rprintf(r,"%s",error);
>               }            
>   
> --- 396,402 ----
>   	        error_fmt = "unable to include %s in parsed file %s";
>   		    
>               if (error_fmt) {
> !                 log_printf(r->server, error_fmt, tag_val, r->filename);
>                   rprintf(r,"%s",error);
>               }            
>   
> ***************
> *** 406,414 ****
>           else if(!strcmp(tag,"done"))
>               return 0;
>           else {
> !             sprintf(errstr,"unknown parameter %s to tag include in %s",tag,
> ! 		    r->filename);
> !             log_error (errstr, r->server);
>               rprintf(r,"%s",error);
>           }
>       }
> --- 405,412 ----
>           else if(!strcmp(tag,"done"))
>               return 0;
>           else {
> !             log_printf(r->server, "unknown parameter %s to tag include in %s",
> ! 		       tag, r->filename);
>               rprintf(r,"%s",error);
>           }
>       }
> ***************
> *** 495,501 ****
>   
>   int handle_exec(FILE *in, request_rec *r, char *error)
>   {
> !     char tag[MAX_STRING_LEN],errstr[MAX_STRING_LEN];
>       char *tag_val;
>       char *file = r->filename;
>   
> --- 493,499 ----
>   
>   int handle_exec(FILE *in, request_rec *r, char *error)
>   {
> !     char tag[MAX_STRING_LEN];
>       char *tag_val;
>       char *file = r->filename;
>   
> ***************
> *** 504,511 ****
>               return 1;
>           if(!strcmp(tag,"cmd")) {
>               if(include_cmd(tag_val, r) == -1) {
> !                 sprintf(errstr,"failed command exec %s in %s",tag_val,file);
> !                 log_error (errstr, r->server);
>                   rprintf(r,"%s",error);
>               }
>               /* just in case some stooge changed directories */
> --- 502,509 ----
>               return 1;
>           if(!strcmp(tag,"cmd")) {
>               if(include_cmd(tag_val, r) == -1) {
> !                 log_printf(r->server, "failed command exec %s in %s",
> ! 			   tag_val, file);
>                   rprintf(r,"%s",error);
>               }
>               /* just in case some stooge changed directories */
> ***************
> *** 513,520 ****
>           } 
>           else if(!strcmp(tag,"cgi")) {
>               if(include_cgi(tag_val, r) == -1) {
> !                 sprintf(errstr,"invalid CGI ref %s in %s",tag_val,file);
> !                 log_error (errstr, r->server);
>                   rprintf(r,"%s",error);
>               }
>               /* grumble groan */
> --- 511,517 ----
>           } 
>           else if(!strcmp(tag,"cgi")) {
>               if(include_cgi(tag_val, r) == -1) {
> !                 log_printf(r->server, "invalid CGI ref %s in %s",tag_val,file);
>                   rprintf(r,"%s",error);
>               }
>               /* grumble groan */
> ***************
> *** 523,531 ****
>           else if(!strcmp(tag,"done"))
>               return 0;
>           else {
> !             char errstr[MAX_STRING_LEN];
> !             sprintf(errstr,"unknown parameter %s to tag exec in %s",tag,file);
> !             log_error (errstr, r->server);
>               rprintf(r, "%s",error);
>           }
>       }
> --- 520,527 ----
>           else if(!strcmp(tag,"done"))
>               return 0;
>           else {
> !             log_printf(r->server, "unknown parameter %s to tag exec in %s",
> ! 		       tag, file);
>               rprintf(r, "%s",error);
>           }
>       }
> ***************
> *** 547,556 ****
>           } else if(!strcmp(tag,"done"))
>               return 0;
>           else {
> !             char errstr[MAX_STRING_LEN];
> !             sprintf(errstr,"unknown parameter %s to tag echo in %s",
>   		    tag, r->filename);
> -             log_error(errstr, r->server);
>               rprintf (r, "%s", error);
>           }
>       }
> --- 543,550 ----
>           } else if(!strcmp(tag,"done"))
>               return 0;
>           else {
> !             log_printf(r->server, "unknown parameter %s to tag echo in %s",
>   		    tag, r->filename);
>               rprintf (r, "%s", error);
>           }
>       }
> ***************
> *** 584,593 ****
>           else if(!strcmp(tag,"done"))
>               return 0;
>           else {
> !             char errstr[MAX_STRING_LEN];
> !             sprintf(errstr,"unknown parameter %s to tag config in %s",
>                       tag, r->filename);
> -             log_error(errstr, r->server);
>               rprintf (r,"%s",error);
>           }
>       }
> --- 578,585 ----
>           else if(!strcmp(tag,"done"))
>               return 0;
>           else {
> !             log_printf(r->server, "unknown parameter %s to tag config in %s",
>                       tag, r->filename);
>               rprintf (r,"%s",error);
>           }
>       }
> ***************
> *** 598,604 ****
>   int find_file(request_rec *r, char *directive, char *tag, 
>                 char *tag_val, struct stat *finfo, char *error)
>   {
> !     char errstr[MAX_STRING_LEN], dir[MAX_STRING_LEN];
>       char *to_send;
>   
>       if(!strcmp(tag,"file")) {
> --- 590,596 ----
>   int find_file(request_rec *r, char *directive, char *tag, 
>                 char *tag_val, struct stat *finfo, char *error)
>   {
> !     char dir[MAX_STRING_LEN];
>       char *to_send;
>   
>       if(!strcmp(tag,"file")) {
> ***************
> *** 606,615 ****
>           getwd(dir);
>           to_send = make_full_path (r->pool, dir, tag_val);
>           if(stat(to_send,finfo) == -1) {
> !             sprintf(errstr,
>                       "unable to get information about %s in parsed file %s",
>                       to_send, r->filename);
> -             log_error (errstr, r->server);
>               rprintf (r,"%s",error);
>               return -1;
>           }
> --- 598,606 ----
>           getwd(dir);
>           to_send = make_full_path (r->pool, dir, tag_val);
>           if(stat(to_send,finfo) == -1) {
> !             log_printf(r->server,
>                       "unable to get information about %s in parsed file %s",
>                       to_send, r->filename);
>               rprintf (r,"%s",error);
>               return -1;
>           }
> ***************
> *** 623,632 ****
>   	    destroy_sub_req (rr);
>   	    return 0;
>           } else {
> !             sprintf(errstr,
>                       "unable to get information about %s in parsed file %s",
>                       tag_val, r->filename);
> -             log_error(errstr, r->server);
>               rprintf(r,"%s",error);
>   	    destroy_sub_req (rr);
>               return -1;
> --- 614,622 ----
>   	    destroy_sub_req (rr);
>   	    return 0;
>           } else {
> !             log_printf(r->server,
>                       "unable to get information about %s in parsed file %s",
>                       tag_val, r->filename);
>               rprintf(r,"%s",error);
>   	    destroy_sub_req (rr);
>               return -1;
> ***************
> *** 633,641 ****
>           }
>       }
>       else {
> !         sprintf(errstr,"unknown parameter %s to tag %s in %s",
>                   tag, directive, r->filename);
> -         log_error(errstr, r->server);
>           rprintf(r,"%s",error);
>           return -1;
>       }
> --- 623,630 ----
>           }
>       }
>       else {
> !         log_printf(r->server, "unknown parameter %s to tag %s in %s",
>                   tag, directive, r->filename);
>           rprintf(r,"%s",error);
>           return -1;
>       }
> ***************
> *** 701,707 ****
>   void send_parsed_content(FILE *f, request_rec *r)
>   {
>       char directive[MAX_STRING_LEN], error[MAX_STRING_LEN];
> !     char timefmt[MAX_STRING_LEN], errstr[MAX_STRING_LEN];
>       int noexec = allow_options (r) & OPT_INCNOEXEC;
>       int ret, sizefmt;
>   
> --- 690,696 ----
>   void send_parsed_content(FILE *f, request_rec *r)
>   {
>       char directive[MAX_STRING_LEN], error[MAX_STRING_LEN];
> !     char timefmt[MAX_STRING_LEN];
>       int noexec = allow_options (r) & OPT_INCNOEXEC;
>       int ret, sizefmt;
>   
> ***************
> *** 717,725 ****
>                   return;
>               if(!strcmp(directive,"exec")) {
>                   if(noexec) {
> !                     sprintf(errstr,"httpd: exec used but not allowed in %s",
> !                             r->filename);
> !                     log_error (errstr, r->server);
>                       rprintf(r,"%s",error);
>                       ret = find_string(f,ENDING_SEQUENCE,NULL);
>                   } else 
> --- 706,714 ----
>                   return;
>               if(!strcmp(directive,"exec")) {
>                   if(noexec) {
> !                     log_printf(r->server,
> ! 			       "httpd: exec used but not allowed in %s",
> ! 			       r->filename);
>                       rprintf(r,"%s",error);
>                       ret = find_string(f,ENDING_SEQUENCE,NULL);
>                   } else 
> ***************
> *** 736,751 ****
>               else if(!strcmp(directive,"flastmod"))
>                   ret=handle_flastmod(f, r, error, timefmt);
>               else {
> !                 sprintf(errstr,"httpd: unknown directive %s in parsed doc %s",
> !                         directive,r->filename);
> !                 log_error (errstr, r->server);
>                   rprintf (r,"%s",error);
>                   ret=find_string(f,ENDING_SEQUENCE,NULL);
>               }
>               if(ret) {
> !                 sprintf(errstr,"httpd: premature EOF in parsed file %s",
> ! 			r->filename);
> !                 log_error(errstr, r->server);
>                   return;
>               }
>           } else 
> --- 725,739 ----
>               else if(!strcmp(directive,"flastmod"))
>                   ret=handle_flastmod(f, r, error, timefmt);
>               else {
> !                 log_printf(r->server, 
> ! 			   "httpd: unknown directive %s in parsed doc %s",
> ! 			   directive, r->filename);
>                   rprintf (r,"%s",error);
>                   ret=find_string(f,ENDING_SEQUENCE,NULL);
>               }
>               if(ret) {
> !                 log_printf(r->server, "httpd: premature EOF in parsed file %s",
> ! 			   r->filename);
>                   return;
>               }
>           } else 
> *** mod_log_common.c.orig	Sun Dec 17 14:32:29 1995
> --- mod_log_common.c	Tue Dec 19 12:16:26 1995
> ***************
> *** 145,154 ****
>       common_log_state *cls = get_module_config (orig->server->module_config,
>   					       &common_log_module);
>     
> !     char str[HUGE_STRING_LEN];
>       long timz;
>       struct tm *t;
> !     char tstr[MAX_STRING_LEN],sign;
>       conn_rec *c = orig->connection;
>       request_rec *r;
>   
> --- 145,154 ----
>       common_log_state *cls = get_module_config (orig->server->module_config,
>   					       &common_log_module);
>     
> !     char *str;
>       long timz;
>       struct tm *t;
> !     char tstr[MAX_STRING_LEN], status[MAX_STRING_LEN], sign;
>       conn_rec *c = orig->connection;
>       request_rec *r;
>   
> ***************
> *** 167,194 ****
>       if(timz < 0) 
>           timz = -timz;
>   
> !     strftime(tstr,MAX_STRING_LEN,"%d/%b/%Y:%H:%M:%S",t);
>   
> !     sprintf(str,"%s %s %s [%s %c%02ld%02ld] \"%s\" ",
> !             c->remote_name,
> !             (c->remote_logname ? c->remote_logname : "-"),
> ! 	    (c->user ? c->user : "-"),
> !             tstr,
> !             sign,
> !             timz/3600,
> !             timz%3600,
> !             (orig->the_request ? orig->the_request : "NULL") );
> !     
> !     if (r->status != -1)
> !         sprintf(str,"%s%d ",str,r->status);
> !     else
> !         strcat(str,"- ");
>   
>       if(r->bytes_sent != -1)
> !         sprintf(str,"%s%d\n",str,r->bytes_sent);
>       else
> !         strcat(str,"-\n");
>   
>       write(cls->log_fd, str, strlen(str));
>   
>       return OK;
> --- 167,192 ----
>       if(timz < 0) 
>           timz = -timz;
>   
> !     strftime(tstr,MAX_STRING_LEN," [%d/%b/%Y:%H:%M:%S",t);
> !     sprintf(&tstr[strlen(tstr)], " %c%02ld%02ld] \"", sign, timz/3600,
> ! 	    timz%3600);
>   
> !     if (r->status != -1) sprintf(status,"%d ", r->status);
> !     else                 strcpy(status, "- ");
>   
>       if(r->bytes_sent != -1)
> ! 	sprintf(&status[strlen(status)], "%d\n", r->bytes_sent);
>       else
> !         strcat(status, "-\n");
>   
> +     str = pstrcat(orig->pool, c->remote_name, " ",
> + 		  (c->remote_logname != NULL ? c->remote_logname : "-"),
> + 		  " ",
> + 		  (c->user != NULL ? c->user : "-"),
> + 		  tstr, 
> + 		  (orig->the_request != NULL ? orig->the_request : "NULL"), 
> + 		  "\" ", status, NULL);
> +     
>       write(cls->log_fd, str, strlen(str));
>   
>       return OK;
> *** mod_log_referer.c.orig	Sun Dec 17 14:32:29 1995
> --- mod_log_referer.c	Tue Dec 19 11:18:23 1995
> ***************
> *** 162,168 ****
>       referer_log_state *cls = get_module_config (orig->server->module_config,
>   					       &referer_log_module);
>     
> !     char str[HUGE_STRING_LEN];
>       char *referer;
>       request_rec *r;
>   
> --- 162,168 ----
>       referer_log_state *cls = get_module_config (orig->server->module_config,
>   					       &referer_log_module);
>     
> !     char *str;
>       char *referer;
>       request_rec *r;
>   
> ***************
> *** 199,206 ****
>   	    }
>   	  
>   	  
> ! 	  sprintf(str, "%s -> %s\n", referer,
> ! 		  r->uri);
>   	  write(cls->referer_fd, str, strlen(str));
>         }
>   
> --- 199,205 ----
>   	    }
>   	  
>   	  
> ! 	  str = pstrcat(orig->pool, referer, " -> ", r->uri, "\n", NULL);
>   	  write(cls->referer_fd, str, strlen(str));
>         }
>   
> -------- end file 69.security.patch

-- 
Ben Laurie                  Phone: +44 (181) 994 6435
Freelance Consultant        Fax:   +44 (181) 994 6472
and Technical Director      Email: ben@algroup.co.uk
A.L. Digital Ltd,           URL: http://www.algroup.co.uk
London, England.

Mime
View raw message