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

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

[Jeff Layton]

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>
---
 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;
 }
 
 /**
-- 
1.7.5.4

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

[Chuck Lever]

Thanks.

This is a security problem, but I don't want to lose sight of the original cause of this bug, which was my lack of understanding of the function of the old code and the API contract (return only _one_ address).

For extra credit, we might check if statd has a similar issue when it matches addresses.

On Jun 22, 2011, at 9: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>
> ---
> 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;
> }
> 
> /**
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

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

[J. Bruce Fields]

On Wed, Jun 22, 2011 at 11:35:53AM -0400, 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.

Thanks to you and Neil for the persistence, that's a good bug....

--b.

> 
> 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>
> ---
>  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;
>  }
>  
>  /**
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff Layton]

On Wed, 22 Jun 2011 10:50:11 -0600
Chuck Lever <chuck.lever@oracle.com> wrote:

> Thanks.
> 
> This is a security problem, but I don't want to lose sight of the original cause of this bug, which was my lack of understanding of the function of the old code and the API contract (return only _one_ address).
> 
> For extra credit, we might check if statd has a similar issue when it matches addresses.
> 

I don't think statd is vulnerable, as best I can tell. I don't see any
places where we do a getnameinfo on an address and then go and use that
to get a list of addresses with getaddrinfo.

Please do double check me on this though. I go on vacation next week
and I think my brain might already have left.

-- 
Jeff Layton <jlayton@redhat.com>

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

[Steve Dickson]

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;
>  }
>  
>  /**

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

[NeilBrown]

On Wed, 22 Jun 2011 13:32:38 -0400 Jeff Layton <jlayton@redhat.com> wrote:

> On Wed, 22 Jun 2011 10:50:11 -0600
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> > Thanks.
> > 
> > This is a security problem, but I don't want to lose sight of the original cause of this bug, which was my lack of understanding of the function of the old code and the API contract (return only _one_ address).
> > 
> > For extra credit, we might check if statd has a similar issue when it matches addresses.
> > 
> 
> I don't think statd is vulnerable, as best I can tell. I don't see any
> places where we do a getnameinfo on an address and then go and use that
> to get a list of addresses with getaddrinfo.
> 
> Please do double check me on this though. I go on vacation next week
> and I think my brain might already have left.
> 

Agggh... my eyes tend to glaze over whenever I try to think about statd :-(

However:
  statd gets MON/UNMON requests from the kernel lockd, and NOTIFY requests
  from other hosts that have rebooted.

  The MON/UNMON requests can only contain a host name or IP that has been
  explicity requested locally so we have to basically trust though - though
  all we do with them it talk to the remote host a bit.

  The NOTIFY request contains a host name and if we are monitoring anything
  that which has that name, we assume that it has rebooted.
  The remote host that sends the NOTIFY could well be lying, but there is no
  a whole lot that we can do about that.  This is a fundamental issue in the
  protocol rather than any mishandling of host name lookup.

So I think statd looks as safe as it ever has been.

NeilBrown