Bug 15195 - Permission denied calling SMBC_getatr when file not exists
Summary: Permission denied calling SMBC_getatr when file not exists
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 4.17.0
Hardware: x64 Linux
: P5 major (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-09 00:45 UTC by Ví­ctor Daniel Martínez O.
Modified: 2022-12-15 16:32 UTC (History)
4 users (show)

See Also:


Attachments
"Raw" patch for master (2.33 KB, patch)
2022-10-09 04:24 UTC, Jeremy Allison
no flags Details
patch for 4.17 (6.88 KB, patch)
2022-10-19 09:13 UTC, Pavel Filipenský
asn: review+
Details
git-am fix for 4.16.next. (6.95 KB, patch)
2022-10-19 16:30 UTC, Jeremy Allison
jra: review? (pfilipensky)
Details
git-am fix for 4.16.next (7.36 KB, patch)
2022-10-25 22:25 UTC, Jeremy Allison
no flags Details
git-am fix for 4.16.next. (7.30 KB, patch)
2022-10-26 17:19 UTC, Jeremy Allison
pfilipensky: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ví­ctor Daniel Martínez O. 2022-10-09 00:45:33 UTC
Description of problem:

gvfs return permission denied calling `SMBC_getatr` when file not exists.

Investigating, apparently `SMBC_getatr` no longer uses `errno` but instead returns `NTSTATUS`:

https://gitlab.com/samba-team/samba/-/commit/d4f8fdd69a1278d8473d541dc6b32014a369bcd0
https://github.com/samba-team/samba/commit/d4f8fdd69a1278d8473d541dc6b32014a369bcd0

Reverting the previous commit fixes the bug. I rebuilt the Fedora 37 Samba SRPM and added the (reverse) patch to SPEC and everything works perfect:

https://koji.fedoraproject.org/koji/buildinfo?buildID=2062333

$ git format-patch -1 --stdout d4f8fdd69a1278d8473d541dc6b32014a369bcd0 > 0001-libsmb-Convert-SMBC_getatr-to-NTSTATUS.patch
0001-libsmb-Convert-SMBC_getatr-to-NTSTATUS.patch

...
Patch100:       0001-libsmb-Convert-SMBC_getatr-to-NTSTATUS.patch
...
%prep
xzcat %{SOURCE0} | gpgv2 --quiet --keyring %{SOURCE2} %{SOURCE1} -
%autosetup -n samba-%{version}%{pre_release} -N
%autopatch -M 99
%patch100 -p1 -R
...

Version-Release number of selected component (if applicable):

$ rpm -qa gvfs-*
gvfs-client-1.50.2-2.fc37.x86_64
gvfs-afc-1.50.2-2.fc37.x86_64
gvfs-fuse-1.50.2-2.fc37.x86_64
gvfs-goa-1.50.2-2.fc37.x86_64
gvfs-afp-1.50.2-2.fc37.x86_64
gvfs-archive-1.50.2-2.fc37.x86_64
gvfs-gphoto2-1.50.2-2.fc37.x86_64
gvfs-mtp-1.50.2-2.fc37.x86_64
gvfs-smb-1.50.2-2.fc37.x86_64

$ rpm -qa gvfs-smb samba-* libsmbclient*
gvfs-smb-1.50.2-2.fc37.x86_64
samba-common-4.17.0-1.fc37.noarch
samba-client-libs-4.17.0-1.fc37.x86_64
samba-common-libs-4.17.0-1.fc37.x86_64
libsmbclient-4.17.0-1.fc37.x86_64
samba-client-4.17.0-1.fc37.x86_64

$ rpm -q glib2
glib2-2.73.3-3.fc37.x86_64

Steps to Reproduce:

$ gio mount -u "smb://bar@SERVER/foo/"
$ gio mount "smb://bar@SERVER/foo/"
$ date | tee /run/user/1000/gvfs/smb-share\:server\=SERVER\,share\=foo\,user\=bar/folder/another-test.txt
tee: '/run/user/1000/gvfs/smb-share:server=192.168.44.1,share=backups,user=bar/folder/another-test.txt': Permission denied
Thu Sep 15 03:55:20 PM -05 2022
$ echo $?
1

Additional info:

Log messages in `GVFS_SMB_DEBUG=10 GVFS_DEBUG=1 /usr/libexec/gvfsd --replace`:

smb: backend_dbus_handler org.gtk.vfs.Mount:QueryInfo (pid=4762)
smb: Queued new job 0x7f0b10006cb0 (GVfsJobQueryInfo)
smbc_stat(smb://SERVER/foo/folder)
smb: backend_dbus_handler org.gtk.vfs.Mount:QueryInfo (pid=4762)
smb: Queued new job 0x7f0b10006b70 (GVfsJobQueryInfo)
SMBC_getatr: sending qpathinfo
smb: send_reply(0x7f0b10006cb0), failed=0 ()
smbc_stat(smb://SERVER/foo/folder)
SMBC_getatr: sending qpathinfo
smb: send_reply(0x7f0b10006b70), failed=0 ()
smb: backend_dbus_handler org.gtk.vfs.Mount:QueryInfo (pid=4762)
smb: Queued new job 0x7f0b10002400 (GVfsJobQueryInfo)
smbc_stat(smb://SERVER/foo/folder/another-test.txt)
SMBC_getatr: sending qpathinfo
map_errno_from_nt_status: 32 bit codes: code=c0000022
cli_status_to_errno: 0xc0000022 -> 13
smb: send_reply(0x7f0b10002400), failed=1 (Permission denied)

Red Hat Bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=2127301

Related gvfs issues:
https://gitlab.gnome.org/GNOME/gvfs/-/issues/649
https://gitlab.gnome.org/GNOME/gvfs/-/issues/651
Comment 1 Jeremy Allison 2022-10-09 04:08:18 UTC
I think I see the problem. Reverting the patch isn't the correct thing to do.

In SMBC_getatr() the code does:

        status = cli_resolve_path(frame, "",
                                  creds,
                                  srv->cli, fixedpath,
                                  &targetcli, &targetpath);
        if (!NT_STATUS_IS_OK(status)) {
                d_printf("Couldn't resolve %s\n", path);
                TALLOC_FREE(frame);
                return NT_STATUS_OBJECT_NAME_NOT_FOUND;
        }

        if (!srv->no_pathinfo2) {
                status = cli_qpathinfo2(targetcli,
                                        targetpath,
                                        &create_time_ts,
                                        &access_time_ts,
                                        &write_time_ts,
                                        &change_time_ts,
                                        &size,
                                        &attr,
                                        &ino);
                if (NT_STATUS_IS_OK(status)) {
                        goto setup_stat;
                }
        }

        if (!srv->no_pathinfo3) {
                status = cli_qpathinfo3(targetcli,
                                        targetpath,
                                        &create_time_ts,
                                        &access_time_ts,
                                        &write_time_ts,
                                        &change_time_ts,
                                        &size,
                                        &attr,
                                        &ino);
                if (NT_STATUS_IS_OK(status)) {
                        goto setup_stat;
                }
        }
...

all_failed:
        srv->no_pathinfo2 = False;
        srv->no_pathinfo3 = False;

        TALLOC_FREE(frame);
        return NT_STATUS_ACCESS_DENIED;

Internally to cli_resolve_path() if we get NT_STATUS_OBJECT_NAME_NOT_FOUND it maps to NT_STATUS_OK (we might be trying to check if a path is valid to create).

Currently we just goto success if either of these calls succeeds, and fall through to the next call on any error. But we should only fall through to the next attempt if we get NT_STATUS_INVALID_LEVEL or NT_STATUS_NOT_SUPPORTED. For any other case we should just return the error value returned by the call.

Currently as both cli_qpathinfo2() and cli_qpathinfo3() are returning NT_STATUS_OBJECT_NOT_FOUND we're falling through to the 'all_failed:' exit, which arbitarily sets NT_STATUS_ACCESS_DENIED as the error code.
Comment 2 Jeremy Allison 2022-10-09 04:24:51 UTC
Created attachment 17546 [details]
"Raw" patch for master

I think this is the correct fix. I'll add a test later and produce an MR for this. If you can test it in the meantime I'd appreciate it !

Thanks,

Jeremy.
Comment 3 Ví­ctor Daniel Martínez O. 2022-10-09 17:20:10 UTC
(In reply to Jeremy Allison from comment #2)
Hi Jeremy. I have rebuilt the Fedora Samba SRPM version 4.17.0 by applying your patch and it fixes the issue. Thank you very much.
Comment 4 Pavel Filipenský 2022-10-14 08:44:09 UTC
Hi Jeremy,

can you please create a MR for this? I would like to get the fix to Fedora 37.

Thanks you,
Pavel
Comment 5 Jeremy Allison 2022-10-14 16:50:47 UTC
I'm still out with COVID. I'm planning to create an MR for this on Monday. It needs a test adding to one of our libsmbclient tests (shouldn't be too hard to do, I'm just not in a good shape to do it yet).

When does Fedora37 freeze ?
Comment 6 Jeremy Allison 2022-10-17 20:28:16 UTC
Running ci on MR with test at:

https://gitlab.com/samba-team/devel/samba/-/pipelines/669177604
Comment 7 Jeremy Allison 2022-10-17 21:54:09 UTC
Pavel, here is the MR for your review.

https://gitlab.com/samba-team/samba/-/merge_requests/2756
Comment 8 Samba QA Contact 2022-10-19 00:14:03 UTC
This bug was referenced in samba master:

9eda432836bfff3d3d4a365a08a5ecb54f0f2e34
fd0c01da1c744ae6fd9d8675616d8b6d3531e469
Comment 9 Pavel Filipenský 2022-10-19 09:13:46 UTC
Created attachment 17583 [details]
patch for 4.17
Comment 10 Andreas Schneider 2022-10-19 09:51:12 UTC
Comment on attachment 17583 [details]
patch for 4.17

lgtm
Comment 11 Andreas Schneider 2022-10-19 09:51:40 UTC
Jule, please apply the patch to the relevant branch. Thanks!
Comment 12 Jule Anger 2022-10-19 10:50:21 UTC
Pushed to autobuild-v4-17-test.
Comment 13 Samba QA Contact 2022-10-19 11:53:11 UTC
This bug was referenced in samba v4-17-test:

09ec2b13e7cccb0beeac6c87a9acd6ea5537d8ed
142a771d85463216075913695d84530c6cb4ff9e
Comment 14 Jule Anger 2022-10-19 12:06:30 UTC
Closing out bug report.

Thanks!
Comment 15 Samba QA Contact 2022-10-19 12:26:18 UTC
This bug was referenced in samba v4-17-stable (Release samba-4.17.1):

09ec2b13e7cccb0beeac6c87a9acd6ea5537d8ed
142a771d85463216075913695d84530c6cb4ff9e
Comment 16 Jeremy Allison 2022-10-19 16:30:15 UTC
Created attachment 17585 [details]
git-am fix for 4.16.next.

Back-port for 4.16.next.
Comment 17 Jeremy Allison 2022-10-25 22:25:00 UTC
Created attachment 17603 [details]
git-am fix for 4.16.next

Backported from master. Fixed version - actually removes knownfail.
Comment 18 Pavel Filipenský 2022-10-26 08:14:56 UTC
Comment on attachment 17603 [details]
git-am fix for 4.16.next

Hi Jeremy, 

the patch looks good, just two tiny comments:


1) According to https://wiki.samba.org/index.php/Samba_Release_Planning#Patch_Process

since it is not a clean cherry pick, this should be changed
(cherry picked from commit fd0c01da1c744ae6fd9d8675616d8b6d3531e469)
to
(backported from commit fd0c01da1c744ae6fd9d8675616d8b6d3531e469)
and add a terse oneliner that explains the conflict on the next line using the following format: [your@mail: reason].

2) there is a mixture of 'false' and 'False' in SMBC_getatr(), should we keep it consistent?
Comment 19 Jeremy Allison 2022-10-26 17:18:05 UTC
(In reply to Pavel Filipenský from comment #18)

Fixed the "Cherry-picked" -> "Back-ported".

No, we shouldn't change "false" -> "False". As we change/add new code we should always move towards standards based types. Consistency isn't so important. I'll update a new version. Thanks !
Comment 20 Jeremy Allison 2022-10-26 17:19:26 UTC
Created attachment 17604 [details]
git-am fix for 4.16.next.

Fixed version.
Comment 21 Pavel Filipenský 2022-10-27 08:42:24 UTC
Comment on attachment 17604 [details]
git-am fix for 4.16.next.

LGTM
Comment 22 Jeremy Allison 2022-10-27 16:04:19 UTC
Hi Jule, if you could merge this for 4.16.next I'd appreciate it - thanks !
Comment 23 Jule Anger 2022-10-31 09:03:04 UTC
Pushed to autobuild-v4-16-test.
Comment 24 Samba QA Contact 2022-10-31 15:32:04 UTC
This bug was referenced in samba v4-16-test:

efa48817d3c6fd3c64051bdf29648dff1702cf5d
618395a7eafbb6224047610659c2d343318a1d33
Comment 25 Jule Anger 2022-10-31 21:04:45 UTC
Closing out bug report.

Thanks!
Comment 26 RC Bosworth 2022-11-17 16:10:13 UTC
I'm still running into problems with an issue I believe is related to this bug. I believe many android file managers are connecting to Samba via GVFS.  They are no longer able to delete multiple files (or dirs) and it appears the problem started with 4.17.0.

Deleting single files works, but returns an access denied message.

No change in 4.17.2.

I'm not sure how to capture client side communications but here's the server side log:

Nov 16 02:43:00 Hippo  smbd[1668]: [2022/11/16 02:43:00.249990,  0] ../../source3/smbd/server.c:1741(main)
Nov 16 02:43:00 Hippo  smbd[1668]:   smbd version 4.17.2 started.
Nov 16 02:43:00 Hippo  smbd[1668]:   Copyright Andrew Tridgell and the Samba Team 1992-2022
Nov 16 02:43:00 Hippo  nmbd[1673]: [2022/11/16 02:43:00.283325,  0] ../../source3/nmbd/nmbd.c:901(main)
Nov 16 02:43:00 Hippo  nmbd[1673]:   nmbd version 4.17.2 started.
Nov 16 02:43:00 Hippo  nmbd[1673]:   Copyright Andrew Tridgell and the Samba Team 1992-2022
...
Nov 17 00:15:38 Hippo  smbd[22786]: [2022/11/17 00:15:38.152578,  2] ../../source3/smbd/open.c:1678(open_file)
Nov 17 00:15:38 Hippo  smbd[22786]:   admin opened file SERVER/FOO/file.mkv read=No write=No (numopen=2)
Nov 17 00:15:38 Hippo  smbd[22786]: [2022/11/17 00:15:38.167282,  3] ../../source3/smbd/smb2_trans2.c:6834(smbd_do_setfilepathinfo)
Nov 17 00:15:38 Hippo  smbd[22786]:   smbd_do_setfilepathinfo: file SERVER/FOO/file.mkv (fnum 1321203594) info_level=1013 totdata=1
Nov 17 00:15:38 Hippo  smbd[22786]: [2022/11/17 00:15:38.175453,  2] ../../source3/smbd/close.c:830(close_normal_file)
Nov 17 00:15:38 Hippo  smbd[22786]:   admin closed file file SERVER/FOO/file.mkv (numopen=0) NT_STATUS_ACCESS_DENIED
Nov 17 00:15:38 Hippo  smbd[22786]:   smbd_smb2_request_error_ex: smbd_smb2_request_error_ex: idx[1] status[NT_STATUS_ACCESS_DENIED] || at ../../source3/smbd/smb2_close.c:111
status[NT_STATUS_ACCESS_DENIED]Nov 17 00:15:38 Hippo  smbd[22786]: [2022/11/17 00:15:38.175530,  3] ../../source3/smbd/smb2_server.c:3955(smbd_smb2_request_error_ex)
Comment 27 Jeremy Allison 2022-11-17 18:20:08 UTC
Can you get me a wireshark trace of this failing please ? Also, open a new bug and upload there as this one is resolved.
Comment 28 Samba QA Contact 2022-12-15 16:32:48 UTC
This bug was referenced in samba v4-16-stable (Release samba-4.16.8):

efa48817d3c6fd3c64051bdf29648dff1702cf5d
618395a7eafbb6224047610659c2d343318a1d33