From patchwork Wed Jul 5 05:08:42 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Shirish Pargaonkar X-Patchwork-Id: 9825839 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2DBA960317 for ; Wed, 5 Jul 2017 05:08:47 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1B279265B9 for ; Wed, 5 Jul 2017 05:08:47 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0C08226E3E; Wed, 5 Jul 2017 05:08:47 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID, T_TVD_MIME_EPI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B6EF3265B9 for ; Wed, 5 Jul 2017 05:08:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750871AbdGEFIp (ORCPT ); Wed, 5 Jul 2017 01:08:45 -0400 Received: from mail-pg0-f44.google.com ([74.125.83.44]:35408 "EHLO mail-pg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750825AbdGEFIo (ORCPT ); Wed, 5 Jul 2017 01:08:44 -0400 Received: by mail-pg0-f44.google.com with SMTP id j186so118396610pge.2 for ; Tue, 04 Jul 2017 22:08:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=k1QT1BJOIanpDftrNEMT1AjkoV2F1iZFnOB44LaKtKQ=; b=L30Oib4/wDpHccaamX+a1DnTHgHrihSUX93fq12SyQj3IjuJ3KDdylo14BKKpNAaj/ TGZdpQcQdfvrjtqw7mXPeyTacHV3aK3EKFUYxzP0byOwLByoo3tdr9AiLJxC4MvXeEhD P0SjqfWInl8deKQNulrMRP/A+GIRPKSBjINj8NujRZwBYfh0fc867CptPUD73ANopJlu oSf0HSw0h9tcBBXAsjScEbf9cGCfZAlJNKfDQbWVfM910Po+yInq6+Ij7+A+jWCIW00y LhvNvUN2ISz2nLUFtwWS5xhuTqZLd2UzqbZ5Byw0QZqDQpT5b2ZRtdDxR70u1d3EdPUW +CPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=k1QT1BJOIanpDftrNEMT1AjkoV2F1iZFnOB44LaKtKQ=; b=VxMwDwulg5QyVoz52o5PlnkkaOh7TtMObIb9C08fem3Sy6SVronaH8glMpaec7q/oV Xbn+UzNh5HQmeUnC1apuJ7zeDKqoKC7M5E6GqmeUzyGujUKSFrym63umh0enCLsuGPQM 3NNMJxE6G2HsA5dld0hxbgDjygE/zEMp5tZX+qri3dHUobcB7ZEX/giMU2RJT2gaS+eK ULYN1Q6bKOsbQYfohRQgkK6AyPu3efaZLoVw5TkFFjj9R0EqGRN/ISPusMH2K031GabT Zm0tJTdPEL9h8yItEjk5oqvkmJVqYJkANsjbf40MkKItDEYtZ2JL5LecVk0mj7FhnnTc UItw== X-Gm-Message-State: AIVw112mw8YNus2Qk9u9FownKOd9GPwRpNzwCcvzGRfKH1hh6u/9ckja /Oqq+FH3Nuua3AR0lcxxvEHu7PyS2A== X-Received: by 10.99.121.13 with SMTP id u13mr19086499pgc.147.1499231323454; Tue, 04 Jul 2017 22:08:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.100.140.206 with HTTP; Tue, 4 Jul 2017 22:08:42 -0700 (PDT) In-Reply-To: References: From: Shirish Pargaonkar Date: Wed, 5 Jul 2017 00:08:42 -0500 Message-ID: Subject: Re: cifs-utils: setcifsacl receives STATUS_INVALID_SECURITY_DESCR reply from custom CIFS implementation To: Paul van Schayck Cc: linux-cifs Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Can you try this patch, I did preliminary testing... On Tue, Jul 4, 2017 at 4:17 PM, Shirish Pargaonkar wrote: > Hi Paul, > > So something like this works with smb 2.1 with the patchset I posted > on the mailing list > to make set acl work over smb 2 onwards... > > $ sudo ls -l /mnt/ > total 1 > -rwx------ 1 root root 32 Jan 26 2015 file1.txt > -rwx------ 1 root root 0 Nov 30 2014 file2.txt > drwx------ 2 root root 0 Nov 30 2014 folder1 > drwx------ 2 root root 0 Dec 22 2014 folder2 > > $ sudo setcifsacl -S > "ACL:S-1-5-21-609720889-20222076-3464071719-1001:ALLOWED/0x0/FULL,ACL:S-1-5-21-609720889-20222076-3464071719-513:ALLOWED/0x0/0x1f01b9,ACL:\Everyone:ALLOWED/0x0/0x1f01b9" > /mnt/file1.txt > > $ sudo ls -l /mnt/ > total 1 > -rwxr-xr-x 1 root root 32 Jan 26 2015 file1.txt > -rwx------ 1 root root 0 Nov 30 2014 file2.txt > drwx------ 2 root root 0 Nov 30 2014 folder1 > drwx------ 2 root root 0 Dec 22 2014 folder2 > > I then changed it back to 700 using smb1 set cifs acl and it worked too. > > $ sudo setcifsacl -S > "ACL:S-1-5-21-609720889-20222076-3464071719-1001:ALLOWED/0x0/FULL,ACL:S-1-5-21-609720889-20222076-3464071719-513:ALLOWED/0x0/0x120088,ACL:\Everyone:ALLOWED/0x0/0x120088" > /mnt/file1.txt > > $ sudo ls -l /mnt/ > total 1 > -rwx------ 1 root root 32 Jan 26 2015 file1.txt > -rwx------ 1 root root 0 Nov 30 2014 file2.txt > drwx------ 2 root root 0 Nov 30 2014 folder1 > drwx------ 2 root root 0 Dec 22 2014 folder2 > > What type of Windows server is this? These requests succeed but I do > see extra bytes > at the end of the security descriptor in both the requests, trying to > figure out why is it allocated in the first place... > > > On Tue, Jun 27, 2017 at 5:43 AM, Paul van Schayck wrote: >> Hi Shirish, >> >> Thank you for your answer. >> >> Attached you can find three pcap files. >> >> 1. linux-fail.pcap: Using the setcifsacl 6.7 version. >> 2. linux-succes.pcap: Using the modified setcifsacl with the patch >> above (note, I had to change to len + 4 in trim_request()) >> 3. windows-succes-trimmed.pcap: Using the dialog in Windows. Note that >> this is over smbv2. The file was trimmed down, because windows was >> noisy. >> >> The command I used was >> >> setcifsacl -S "ACL:S-1-5-21-1572361299-1184395705-1606240830-247332:ALLOWED/OI|CI/CHANGE,ACL:S-1-5-21-1572361299-1184395705-1606240830-544559:ALLOWED/I/FULL,ACL:S-1-3-0:ALLOWED/OI|CI|IO|I/FULL,ACL:S-1-5-21-1572361299-1184395705-1606240830-512:ALLOWED/OI|CI|I/FULL,ACL:S-1-5-21-1572361299-1184395705-1606240830-549266:ALLOWED/OI|CI|I/FULL,ACL:S-1-5-21-1572361299-1184395705-1606240830-544559:ALLOWED/OI|CI|IO|I/FULL,ACL:S-1-5-18:ALLOWED/OI|CI|I/FULL" >> cifs-test/ >> >> In Windows I used the dialog to set the same ACEs. Between the >> linux-success and linux-fail file you can see the difference in the >> number of zero bytes after the DACL. >> >> If this test case is still a bit noisy for you, I can try to build a >> more simple ACE list. But I have to setup a new share for this. >> >> Thanks, >> >> Paul van Schayck >> >> On Tue, Jun 27, 2017 at 6:40 AM, Shirish Pargaonkar >> wrote: >>> if you can provide tcpdumpas well as the command you are using of/for >>> both Windows and Linux way, that would be helpful >>> >>> On Mon, Jun 26, 2017 at 8:04 AM, Paul van Schayck via samba-technical >>> wrote: >>>> Dear samba-technical, >>>> >>>> If this list is inappropriate for this question then please redirect >>>> me to a better one. >>>> >>>> I'm working with a HDS HNAS (Hitachi NAS) which has its own >>>> implementation of the CIFS protocol. Mounting, and interactions all go >>>> fine using linux-cifs and related tools. >>>> >>>> However, when sending a `setcifsacl -a ACE /dir` the response is >>>> "main: setxattr error: Input/output error". Upon further digging using >>>> tcpdump/wireshark it became clear that the HNAS is responding with >>>> >>>> NT Status: STATUS_INVALID_SECURITY_DESCR (0xc0000079) >>>> >>>> Setting the same ACE using a Windows client did work. So further >>>> examining of the request being sent showed that the only difference >>>> between the Windows client and setcifsacl was the addition of a lot of >>>> padding zero bytes at the end of the security descriptor. >>>> >>>> Examining the code of setcifsacl.c and the request more it seems like >>>> the request was not properly trimmed down to the number of aces being >>>> sent. More buffer was allocated that the number of aces being sent. So >>>> I made the attached patch to trim down the request. This fixes some of >>>> the problems with setcifsacl, but for example breaks deleting aces. I >>>> also think it's most likely an ugly fix, and the problem needs to be >>>> solved elsewhere most likely. >>>> >>>> So my question is if someone can point me at the correct way to fix >>>> this in setcifsacl, or help me fix it properly. If necessary I can >>>> provide tcpdump's of Windows and Linux clients performing the request. >>>> >>>> Thanks, >>>> >>>> Paul van Schayck >>>> >>>> diff --git a/setcifsacl.c b/setcifsacl.c >>>> index 7eeeaa6..4ada3c8 100644 >>>> --- a/setcifsacl.c >>>> +++ b/setcifsacl.c >>>> @@ -761,6 +761,20 @@ setacl_action(struct cifs_ntsd *pntsd, struct >>>> cifs_ntsd **npntsd, >>>> return rc; >>>> } >>>> >>>> +ssize_t >>>> +trim_request(ssize_t bufsize, struct cifs_ntsd *npntsd) { >>>> + int i, len = 0; >>>> + char *a = (char *) npntsd; >>>> + >>>> + for( i = 0; i < bufsize; i++) { >>>> + if ( a[i] != 0 ) >>>> + len = i; >>>> + } >>>> + >>>> + return (ssize_t) len + 2; >>>> +} >>>> + >>>> + >>>> static void >>>> setcifsacl_usage(const char *prog) >>>> { >>>> @@ -902,7 +916,7 @@ cifsacl: >>>> if (rc) >>>> goto setcifsacl_action_ret; >>>> >>>> - attrlen = setxattr(filename, ATTRNAME, ntsdptr, bufsize, 0); >>>> + attrlen = setxattr(filename, ATTRNAME, ntsdptr, >>>> trim_request(bufsize, ntsdptr), 0); >>>> if (attrlen == -1) { >>>> printf("%s: setxattr error: %s\n", __func__, strerror(errno)); >>>> goto setcifsacl_facenum_ret; >>>> diff --git a/setcifsacl.c b/setcifsacl.c index 7eeeaa6..0d4b15f 100644 --- a/setcifsacl.c +++ b/setcifsacl.c @@ -50,24 +50,33 @@ enum setcifsacl_actions { static void *plugin_handle; static bool plugin_loaded; -static void +static int copy_cifs_sid(struct cifs_sid *dst, const struct cifs_sid *src) { - int i; + int i, size = 0; dst->revision = src->revision; + size += sizeof(uint8_t); + dst->num_subauth = src->num_subauth; + size += sizeof(uint8_t); + for (i = 0; i < NUM_AUTHS; i++) dst->authority[i] = src->authority[i]; + size += (sizeof(uint8_t) * NUM_AUTHS); + for (i = 0; i < src->num_subauth; i++) dst->sub_auth[i] = src->sub_auth[i]; + size += (sizeof(uint32_t) * src->num_subauth); + + return size; } static void copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd, - int numaces, int acessize) + int numaces, int acessize, ssize_t *bufsize) { - int osidsoffset, gsidsoffset, dacloffset; + int size, osidsoffset, gsidsoffset, dacloffset; struct cifs_sid *owner_sid_ptr, *group_sid_ptr; struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr; struct cifs_ctrl_acl *dacl_ptr, *ndacl_ptr; @@ -77,28 +86,34 @@ copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd, gsidsoffset = le32toh(pntsd->gsidoffset); dacloffset = le32toh(pntsd->dacloffset); + size = sizeof(struct cifs_ntsd); pnntsd->revision = pntsd->revision; pnntsd->type = pntsd->type; pnntsd->osidoffset = pntsd->osidoffset; pnntsd->gsidoffset = pntsd->gsidoffset; pnntsd->dacloffset = pntsd->dacloffset; + *bufsize = *bufsize + size; dacl_ptr = (struct cifs_ctrl_acl *)((char *)pntsd + dacloffset); ndacl_ptr = (struct cifs_ctrl_acl *)((char *)pnntsd + dacloffset); + size = acessize + sizeof(struct cifs_ctrl_acl); ndacl_ptr->revision = dacl_ptr->revision; - ndacl_ptr->size = htole16(acessize + sizeof(struct cifs_ctrl_acl)); + ndacl_ptr->size = htole16(size); ndacl_ptr->num_aces = htole32(numaces); + *bufsize = *bufsize + size; /* copy owner sid */ owner_sid_ptr = (struct cifs_sid *)((char *)pntsd + osidsoffset); nowner_sid_ptr = (struct cifs_sid *)((char *)pnntsd + osidsoffset); - copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr); + size = copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr); + *bufsize = *bufsize + size; /* copy group sid */ group_sid_ptr = (struct cifs_sid *)((char *)pntsd + gsidsoffset); ngroup_sid_ptr = (struct cifs_sid *)((char *)pnntsd + gsidsoffset); - copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr); + size = copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr); + *bufsize = *bufsize + size; return; } @@ -156,10 +171,10 @@ compare_aces(struct cifs_ace *sace, struct cifs_ace *dace, int compflags) } static int -get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, - int aces, ssize_t *bufsize, size_t *acesoffset) +alloc_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, + int aces, size_t *acesoffset) { - unsigned int size, acessize, dacloffset; + unsigned int size, acessize, bufsize, dacloffset; size = sizeof(struct cifs_ntsd) + 2 * sizeof(struct cifs_sid) + @@ -169,9 +184,9 @@ get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, *acesoffset = dacloffset + sizeof(struct cifs_ctrl_acl); acessize = aces * sizeof(struct cifs_ace); - *bufsize = size + acessize; + bufsize = size + acessize; - *npntsd = malloc(*bufsize); + *npntsd = malloc(bufsize); if (!*npntsd) { printf("%s: Memory allocation failure", __func__); return errno; @@ -188,7 +203,7 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize, size_t acesoffset; char *acesptr; - rc = get_sec_desc_size(pntsd, npntsd, numcaces, bufsize, &acesoffset); + rc = alloc_sec_desc(pntsd, npntsd, numcaces, &acesoffset); if (rc) return rc; @@ -198,9 +213,8 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize, acessize += size; acesptr += size; } - copy_sec_desc(pntsd, *npntsd, numcaces, acessize); - acesptr = (char *)*npntsd + acesoffset; + copy_sec_desc(pntsd, *npntsd, numcaces, acessize, bufsize); return 0; } @@ -215,7 +229,7 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize, char *acesptr; numaces = numfaces + numcaces; - rc = get_sec_desc_size(pntsd, npntsd, numaces, bufsize, &acesoffset); + rc = alloc_sec_desc(pntsd, npntsd, numaces, &acesoffset); if (rc) return rc; @@ -230,7 +244,8 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize, acesptr += size; acessize += size; } - copy_sec_desc(pntsd, *npntsd, numaces, acessize); + + copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize); return 0; } @@ -249,7 +264,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize, return -1; } - rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset); + rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset); if (rc) return rc; @@ -270,7 +285,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize, acessize += size; } - copy_sec_desc(pntsd, *npntsd, numfaces, acessize); + copy_sec_desc(pntsd, *npntsd, numfaces, acessize, bufsize); return 0; } @@ -294,7 +309,7 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize, return -1; } - rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset); + rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset); if (rc) return rc; @@ -317,7 +332,8 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize, printf("%s: Nothing to delete\n", __func__); return 1; } - copy_sec_desc(pntsd, *npntsd, numaces, acessize); + + copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize); return 0; }