Re: Bug fix for mod_access check of remotehost

[Ben Laurie <be...@gonzo.ben.algroup.co.uk>]

+1 except that:

     while (*host && ((*host == '.') || isdigit(*host))) host++;

should really be:

     while (*host == '.' || isdigit(*host))
	host++;

Cheers,

Ben.

Roy T. Fielding wrote:
> 
> I believe the following patch correctly fixes the problem noted by
> Dean a long time ago, namely
> 
> >P.P.S.  Hostnames such as "123.hotwired.com" are valid, yet find_allowdeny
> >does not properly handle them.  This should be put on Known Bugs.  Be
> >careful when fixing this because just removing the isalpha() check creates
> >a security hole, consider the DNS map "1.1.1.1.in-addr.arpa IN PTR 2.2.2."
> >if the user has a config line "allow from 2.2.2" it will allow 1.1.1.1 in
> >(unless -DMAXIMUM_DNS).  -- which is bad because it breaks people who
> >understand double reverse lookup and are trying to avoid it by using
> >only ip addresses on allow/deny statements.
> 
> .....Roy
> 
> Index: mod_access.c
> ===================================================================
> RCS file: /export/home/cvs/apache/src/mod_access.c,v
> retrieving revision 1.10
> diff -c -r1.10 mod_access.c
> *** mod_access.c	1996/11/18 19:40:49	1.10
> --- mod_access.c	1996/11/30 13:39:25
> ***************
> *** 167,178 ****
>       return (what[l] == '\0' || what[l] == '.');
>   }
>   
>   int find_allowdeny (request_rec *r, array_header *a, int method)
>   {
>       allowdeny *ap = (allowdeny *)a->elts;
>       int mmask = (1 << method);
> !     int i, gothost=0;
> !     const char *remotehost=NULL;
>   
>       for (i = 0; i < a->nelts; ++i) {
>           if (!(mmask & ap[i].limited))
> --- 167,185 ----
>       return (what[l] == '\0' || what[l] == '.');
>   }
>   
> + static int is_ip(const char *host)
> + {
> +     while (*host && ((*host == '.') || isdigit(*host))) host++;
> +     return (*host == '\0');
> + }
> + 
>   int find_allowdeny (request_rec *r, array_header *a, int method)
>   {
>       allowdeny *ap = (allowdeny *)a->elts;
>       int mmask = (1 << method);
> !     int i;
> !     int gothost = 0;
> !     const char *remotehost = NULL;
>   
>       for (i = 0; i < a->nelts; ++i) {
>           if (!(mmask & ap[i].limited))
> ***************
> *** 191,205 ****
>   	
>   	if (!strcmp (ap[i].from, "all"))
>   	    return 1;
> ! 	if (!gothost)
> ! 	{
>   	    remotehost = get_remote_host(r->connection, r->per_dir_config,
> ! 					 REMOTE_HOST);
> ! 	    gothost = 1;
>   	}
> !         if (remotehost != NULL && isalpha(remotehost[0]))
> !             if (in_domain(ap[i].from, remotehost))
> !                 return 1;
>           if (in_ip (ap[i].from, r->connection->remote_ip))
>               return 1;
>       }
> --- 198,217 ----
>   	
>   	if (!strcmp (ap[i].from, "all"))
>   	    return 1;
> ! 
> ! 	if (!gothost) {
>   	    remotehost = get_remote_host(r->connection, r->per_dir_config,
> ! 	                                 REMOTE_HOST);
> ! 
> ! 	    if ((remotehost == NULL) || is_ip(remotehost))
> ! 	        gothost = 1;
> ! 	    else
> ! 	        gothost = 2;
>   	}
> ! 
> !         if ((gothost == 2) && in_domain(ap[i].from, remotehost))
> !             return 1;
> ! 
>           if (in_ip (ap[i].from, r->connection->remote_ip))
>               return 1;
>       }

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