>
> 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.
|