linux-kernel - Re: [RFC v3 36/45] NFSv4: Fix GETATTR bitmap verification

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150528203332.GD31663@fieldses.org>
Date:	Thu, 28 May 2015 16:33:32 -0400
From:	bfields@...ldses.org (J. Bruce Fields)
To:	Andreas Gruenbacher <andreas.gruenbacher@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-nfs@...r.kernel.org
Subject: Re: [RFC v3 36/45] NFSv4: Fix GETATTR bitmap verification

On Fri, Apr 24, 2015 at 01:04:33PM +0200, Andreas Gruenbacher wrote:
> The NFSv4 client sends the server GETATTR requests with different sets of
> requested attributes depending on the situation.  The requested set of
> attributes is encoded in a bitmap; the server replies with the set of
> attributes it could return.  These bitmaps can be several words wide.  The
> bitmap returned by the server is a subset of the bitmap sent by the client.
> 
> While decoding the reply, the client tries to verify the reply bitmap: it
> checks if any previous, unexpected attributes are left in the same word of the
> bitmap for each attribute it tries to decode, then it clears the current
> attribute's bit in the bitmap for the next decode function.
> 
> The client fails to detect when unexpected attributes are sent after the last
> expected attribute in each word in the bitmap.

Is it important that the client catch that?

> Fix this by checking the entire bitmap for unexpected attributes first.  The
> server can still send attributes which the client understands but which it
> didn't request; this doesn't cause any harm.

I don't understand that last sentence.  On a skim it looks like after
this patch we *will* still error out if a server does that, right?  (As
we should, that would be a server bug.)

...
> +static int verify_attrs_allowed(uint32_t *bitmap, const uint32_t *attrs_allowed)
> +{
> +	if (unlikely(bitmap[0] & ~attrs_allowed[0] ||
> +		     bitmap[1] & ~attrs_allowed[1] ||
> +		     bitmap[2] & ~attrs_allowed[2]))
> +		return -EIO;
> +	return 0;
> +}
> +
>  static int decode_change_info(struct xdr_stream *xdr, struct nfs4_change_info *cinfo)
>  {
>  	__be32 *p;
> @@ -4296,6 +4195,11 @@ out_overflow:
>  
>  static int decode_server_caps(struct xdr_stream *xdr, struct nfs4_server_caps_res *res)
>  {
> +	static const uint32_t attrs_allowed[3] = {
> +		[0] = FATTR4_WORD0_SUPPORTED_ATTRS | FATTR4_WORD0_FH_EXPIRE_TYPE |
> +		      FATTR4_WORD0_LINK_SUPPORT | FATTR4_WORD0_SYMLINK_SUPPORT |
> +		      FATTR4_WORD0_ACLSUPPORT,
> +	};

This is the same list as in nfs4-xdr_enc_server_caps.  Could we avoid
that duplication?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/