Re: [PATCH] nfs: fix host_reliable_addrinfo (try #2)

[Steve Dickson <SteveD@redhat.com>]

On 06/22/2011 11:35 AM, Jeff Layton wrote:
> According to Neil Brown:
> 
>     The point of the word 'reliable' is to check that the name we get
>     really does belong to the host in question - ie that both the
>     forward and reverse maps agree.
> 
>     But the new code doesn't do that check at all.  Rather it simply
>     maps the address to a name, then discards the address and maps the
>     name back to a list of addresses and uses that list of addresses as
>     "where the request came from" for permission checking.
> 
> This bug is exploitable via the following scenario and could allow an
> attacker access to data that they shouldn't be able to access.
> 
>     Suppose you export a filesystem to some subnet or FQDN and also to a
>     wildcard or netgroup, and I know the details of this (maybe
>     showmount -e tells me) Suppose further that I can get IP packets to
>     your server..
> 
>     Then I create a reverse mapping for my ipaddress to a domain that I
>     own, say "black.hat.org", and a forward mapping from that domain to
>     my IP address, and one of your IP addresses.
> 
>     Then I try to mount your filesystem.  The IP address gets correctly
>     mapped to "black.hat.org" and then mapped to both my IP address and
>     your IP address.
> 
>     Then you search through all of your exports and find that one of the
>     addresses: yours - is allowed to access the filesystem.
> 
>     So you create an export based on the addrinfo you have which allows
>     my IP address the same access as your IP address.
> 
> Fix this by instead using the forward lookup of the hostname just to
> verify that the original address is in the list. Then do a numeric
> lookup using the address and stick the hostname in the ai_canonname.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Reviewed-by: NeilBrown <neilb@suse.de>
Committed... 

steved.

> ---
>  support/export/hostname.c |   36 ++++++++++++++++++++++++++++++------
>  1 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/support/export/hostname.c b/support/export/hostname.c
> index efcb75c..3e949a1 100644
> --- a/support/export/hostname.c
> +++ b/support/export/hostname.c
> @@ -258,17 +258,19 @@ host_canonname(const struct sockaddr *sap)
>   * @sap: pointer to socket address to look up
>   *
>   * Reverse and forward lookups are performed to ensure the address has
> - * proper forward and reverse mappings.
> + * matching forward and reverse mappings.
>   *
> - * Returns address info structure with ai_canonname filled in, or NULL
> - * if no information is available for @sap.  Caller must free the returned
> - * structure with freeaddrinfo(3).
> + * Returns addrinfo structure with just the provided address with
> + * ai_canonname filled in. If there is a problem with resolution or
> + * the resolved records don't match up properly then it returns NULL
> + *
> + * Caller must free the returned structure with freeaddrinfo(3).
>   */
>  __attribute_malloc__
>  struct addrinfo *
>  host_reliable_addrinfo(const struct sockaddr *sap)
>  {
> -	struct addrinfo *ai;
> +	struct addrinfo *ai, *a;
>  	char *hostname;
>  
>  	hostname = host_canonname(sap);
> @@ -276,9 +278,31 @@ host_reliable_addrinfo(const struct sockaddr *sap)
>  		return NULL;
>  
>  	ai = host_addrinfo(hostname);
> +	if (!ai)
> +		goto out_free_hostname;
>  
> -	free(hostname);
> +	/* make sure there's a matching address in the list */
> +	for (a = ai; a; a = a->ai_next)
> +		if (nfs_compare_sockaddr(a->ai_addr, sap))
> +			break;
> +
> +	freeaddrinfo(ai);
> +	if (!a)
> +		goto out_free_hostname;
> +
> +	/* get addrinfo with just the original address */
> +	ai = host_numeric_addrinfo(sap);
> +	if (!ai)
> +		goto out_free_hostname;
> +
> +	/* and populate its ai_canonname field */
> +	free(ai->ai_canonname);
> +	ai->ai_canonname = hostname;
>  	return ai;
> +
> +out_free_hostname:
> +	free(hostname);
> +	return NULL;
>  }
>  
>  /**