Bug 11500 - Issues with libsmbclient library api - SMBC_setxattr()
Issues with libsmbclient library api - SMBC_setxattr()
Status: NEW
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient
4.1.12
All All
: P5 major
: ---
Assigned To: Samba QA Contact
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-07 06:52 UTC by hargagan
Modified: 2015-09-07 07:59 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description hargagan 2015-09-07 06:52:22 UTC
I was seeing issues with the samba 4 based libsmbclient library while using SMBC_setxattr() apis for setting the net security descriptor. Had a discussion on the samba-technical mailing list, where the problem is confirmed by Jeremy. Hence raising this issue. 

Putting the mail thread here for the quick reference :



On Thu, Sep 03, 2015 at 02:22:07AM -0600, Har Gagan Sahai wrote:
> 
> Hi Samba Team, 
> 
> 
> I have a question regarding the SMBC_setxattr() used to set the nt-security-descriptor. During the process of creating the request it is setting the 'ret = -1' which is causing the failure of the API. Here is the code snippet in cacl_set() [notice the statement in bold ] that I found to be causing the problem  : 
> 
> 
> case SMBC_XATTR_MODE_ADD: 
>                 for (i=0;sd->dacl && i<sd->dacl->num_aces;i++) { 
>                         bool found = False; 
> 
> 
>                         for (j=0;old->dacl && j<old->dacl->num_aces;j++) { 
>                                 if (dom_sid_equal(&sd->dacl->aces[i].trustee, 
>                                               &old->dacl->aces[j].trustee)) { 
>                                         if (!(flags & SMBC_XATTR_FLAG_CREATE)) { 
>                                                 err = EEXIST; 
>                                                 ret = -1; 
>                                                 goto failed; 
>                                         } 
>                                         old->dacl->aces[j] = sd->dacl->aces[i]; 
>                                         ret = -1; 
>                                         found = True; 
>                                 } 
>                         } 
> 
> 
>                         if (!found && (flags & SMBC_XATTR_FLAG_REPLACE)) { 
>                                 err = ENOATTR; 
>                                 ret = -1; 
>                                 goto failed; 
>                         } 
> 
> 
>                         for (i=0;sd->dacl && i<sd->dacl->num_aces;i++) { 
>                                 add_ace(&old->dacl, &sd->dacl->aces[i], ctx); 
>                         } 
>                 } 
>                 dacl = old->dacl; 
>                 break; 
> 
> 
> Kindly let me know the reason behind this condition as this causing the failure of the usage. Incase this is not as intended let me know I can file a bug for this.  

Yes, I'm pretty sure the 'ret = -1' is a bug in
this case. It should be:

                          for (j=0;old->dacl && j<old->dacl->num_aces;j++) {
                                  if (dom_sid_equal(&sd->dacl->aces[i].trustee,
                                                &old->dacl->aces[j].trustee)) {
                                          if (!(flags & SMBC_XATTR_FLAG_CREATE)) {
                                                  err = EEXIST;
                                                  ret = -1;
                                                  goto failed;
                                          }
                                          old->dacl->aces[j] = sd->dacl->aces[i];
                                          found = True;
                                  }
                          }


However, I also think the check:

	if (!(flags & SMBC_XATTR_FLAG_CREATE)) {

is incorrect. That flag means (from source3/include/libsmbclient.h):

/*
 * Flags for smbc_setxattr()
 *   Specify a bitwise OR of these, or 0 to add or replace as necessary
 */
#define SMBC_XATTR_FLAG_CREATE       0x1 /* fail if attr already exists */
#define SMBC_XATTR_FLAG_REPLACE      0x2 /* fail if attr does not exist */

so I think the correct code here should be:

	if (flags & SMBC_XATTR_FLAG_CREATE) {

Also, the logic here of just overwriting the ace entry
with the passed in one, and then adding it again in
the code:

	for (i=0;sd->dacl && i<sd->dacl->num_aces;i++) {
		add_ace(&old->dacl, &sd->dacl->aces[i], ctx);
	}

looks wrong again. That way we get 2 copies of the entry.

Please log a bug and let's use that to coordinate
working out what this code should really do.

Jeremy.