[PATCH] Fix for XDR Backend of NFS4ACL_XATTR module to get it working with NFS4.0 ACL Spec

[Sandeep Nashikkar]

On Fri, Jul 20, 2018 at 12:47:16PM +0000, Sandeep Nashikkar via samba-technical wrote:
> 
> *         Changed XDR attribute name to "system.nfs4_acl" which works with native NFSv4 and nfs-ganesha.

Hmmm. I'm going to have to think about this some more
- this changes the read/write possibilities for the attibute to root-only. Is this correct ?

<Sandeep> I used this attribute using local user as well as domain user login through samba and did not see any issue. As per man page of the xattr "Read and write access permissions to system attributes depend on the policy implemented for each system attribute implemented by filesystems in the kernel."

> +             size_t identifiers_size;
> +             int i;

i should be unsigned, can never be negative.

<Sandeep> Noted 

> +                             nfsace40 *nace40 = nfs40acl_get_ace(nacl40, i);
> +                             /* UTf-8 identifier strings are aligned */
> +                             if (nace40->who.utf8string_len % 4) {
> +                                             size_t id_size = nace40->who.utf8string_len;
> +                                             id_size += (4 - (nace40->who.utf8string_len % 4));
> +                                             identifiers_size += id_size;
> +                                             DBG_DEBUG("identifier[%d] size: %ld\n", i, id_size);
> +                             } else {
> +                                             identifiers_size += (nace40->who.utf8string_len);
> +                             }
> +             }

In the above, does utf8string_len come from the client ?
If so we need overflow checks on the additions of utf8string_len to prevent security problems.

As a rule, doing overflow checks on any additions from marshalling/unmarshalling code is a good idea anyway.

<Sandeep> Can you please elaborate more on what kind of limitations to check the string length against? Are there any specific limits as per RFC which restrict the security principal string length (or additions of it) to x number of bytes?

> +             char* strid = malloc(id_len + 1);

Use talloc, not malloc here.

<Sandeep> Noted 

> +             free(strid);

Use talloc, not malloc here.

<Sandeep> Noted

> + = calloc(1, nace40->who.utf8string_len + 1);

Use talloc here, not malloc.

<Sandeep> Noted 

> + = calloc(1, nace40->who.utf8string_len + 1);

Use talloc here, not malloc.

<Sandeep> Noted


Thanks for reviewing the patch Jeremy. I will submit the patch again with some more changes. There are cases when the security principal in the NFSv4 ACL is changed from numeric uid/gid to string by nfs-ganesha after restart. Will handle those cases as well and submit the patch with above fixes. Please let me know what kind of limitations you were referring to while checking overflow conditions after unmarshalling data. Any pointer to existing code with similar checks would be a great help. 
***************************Legal Disclaimer***************************
"This communication may contain confidential and privileged material for the
sole use of the intended recipient. Any unauthorized review, use or distribution
by others is strictly prohibited. If you have received the message by mistake,
please advise the sender by reply email and delete the message. Thank you."
**********************************************************************