Bug 14983 - NT_STATUS_ACCESS_DENIED translates into EPERM instead of EACCES in SMBC_server_internal
Summary: NT_STATUS_ACCESS_DENIED translates into EPERM instead of EACCES in SMBC_serve...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 4.15.5
Hardware: All Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-18 19:13 UTC by Elia Geretto
Modified: 2022-05-07 10:04 UTC (History)
7 users (show)

See Also:


Attachments
git-am fix for 4.16.rcNext, 4.15.next, 4.14.next. (1.50 KB, patch)
2022-03-16 21:32 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Elia Geretto 2022-02-18 19:13:32 UTC
In the code snippet at:

https://gitlab.com/samba-team/samba/-/blob/master/source3/libsmb/libsmb_server.c#L575

the value of the NT_STATUS is discarded and EPERM is always returned, even on NT_STATUS_ACCESS_DENIED. Given the error paths around it, it seems that it should instead be translated using map_errno_from_nt_status.

I have reached that code location while debugging the addition of a printer in an authenticated SMB share in GNOME Settings 41, which expects EACCES when credentials are required.
Comment 1 Jeremy Allison 2022-02-18 20:56:40 UTC
Just clarifying, you think that the errno return should be from the NTSTATUS returned from:

564	status = cli_session_setup_creds(c, creds);
565	if (!NT_STATUS_IS_OK(status)) {

not from the (unsaved) status returned in:

572		    !NT_STATUS_IS_OK(cli_session_setup_anon(c))) {

Correct ?
Comment 2 Elia Geretto 2022-02-18 21:01:04 UTC
To be honest, I do not know what would make sense in general case. In the case of an authenticated server, I think there would be no difference because both are ACCESS_DENIED, right?.
Comment 3 Elia Geretto 2022-02-22 09:07:15 UTC
Just to clarify, does that look like the cause of the wrong errno value to you, or should I debug some more?
Comment 4 Elia Geretto 2022-03-08 15:19:50 UTC
Ping.
Comment 5 Elia Geretto 2022-03-12 20:25:58 UTC
I have created an MR [1] with the change discussed before. I have tested it with GNOME 41.3 on Fedora 35 and it fixes the bug I was experiencing. I can now properly list SMB print servers protected by authentication.

[1]: https://gitlab.com/samba-team/samba/-/merge_requests/2436
Comment 6 Samba QA Contact 2022-03-16 19:45:03 UTC
This bug was referenced in samba master:

70b9977a46e5242174b4461a7f49d5f640c1db62
Comment 7 Jeremy Allison 2022-03-16 21:32:10 UTC
Created attachment 17220 [details]
git-am fix for 4.16.rcNext, 4.15.next, 4.14.next.

Cherry-picks cleanly from master for 4.16.rcNext, 4.15.next, 4.14.next.
Comment 8 Jule Anger 2022-03-17 08:29:26 UTC
Pushed to autobuild-v4-{16,15,14}-test.
Comment 9 Samba QA Contact 2022-03-17 09:46:04 UTC
This bug was referenced in samba v4-14-test:

3ae7ead5fd53e5ca590cb6bee82afc92b35264f6
Comment 10 Samba QA Contact 2022-03-17 10:13:03 UTC
This bug was referenced in samba v4-16-test:

34771e1931587807d0395c7ac7f4be18654997f4
Comment 11 Samba QA Contact 2022-03-17 10:55:30 UTC
This bug was referenced in samba v4-15-test:

9b6e8ae65e24788c82022e761bfb2c78b59260d9
Comment 12 Jule Anger 2022-03-17 11:03:59 UTC
Closing out bug report.

Thanks!
Comment 13 Samba QA Contact 2022-03-21 12:17:29 UTC
This bug was referenced in samba v4-16-stable (Release samba-4.16.0):

34771e1931587807d0395c7ac7f4be18654997f4
Comment 14 Andreas Schneider 2022-04-04 07:46:47 UTC
This creates a regression in Samba 4.16 that users are not able to connect to Samba shares with Nautilus anymore!

See
https://bugzilla.redhat.com/show_bug.cgi?id=2068976
https://gitlab.gnome.org/GNOME/gvfs/-/issues/611
Comment 15 Samba QA Contact 2022-04-04 12:49:55 UTC
This bug was referenced in samba v4-14-stable (Release samba-4.14.13):

3ae7ead5fd53e5ca590cb6bee82afc92b35264f6
Comment 16 Jeremy Allison 2022-04-04 16:01:06 UTC
Andreas, can you get me a packet trace showing what is happening here ?

We obviously don't have a test for what nautilus is doing here so we have no guarentees that we won't keep breaking this. We need to have a test that does *exactly* what nautilus wants so we can fix this.
Comment 17 Jeremy Allison 2022-04-04 16:02:09 UTC
Why is nautilus depending on exact error codes here ? Can you give me a link to the nautilus code that is using this ?
Comment 18 Guenther Deschner 2022-04-04 17:38:58 UTC
Hi Jeremy,

I believe this is the relevant gnome code:

https://gitlab.gnome.org/GNOME/gvfs/-/blob/master/daemon/gvfsbackendsmb.c#L516
Comment 19 Jeremy Allison 2022-04-04 17:47:33 UTC
OK, so the change was to map the error code returned from the server to errno rather than hard-coding it to EACCESS. Which (IMHO) is the correct thing to do.

errno = map_errno_from_nt_status(status);

can you get a packet trace from a failed anonymous logon, so we can see exactly what NTSTATUS we're getting here and work out what it's being mapped to in errno.

The gnome code is doing:

if (.. || (errsv != EACCES && errsv != EPERM))

so if it really is getting NT_STATUS_ACCESS_DENIED then it should work. We need to see what exactly we got back here. I think hard coding these error codes in gnome isn't the right thing to do to fall-back to anonymous logon.

Why is it checking them ? Why not just always try anonymous logon if the user doesn't give a username or password ?
Comment 20 Jeremy Allison 2022-04-04 17:50:08 UTC
Here is the error map we are using to map NT_STATUS -> errno.

        {NT_STATUS_ACCESS_VIOLATION, EACCES},
..
        {NT_STATUS_ACCESS_DENIED, EACCES},
..
        {NT_STATUS_WRONG_PASSWORD, EACCES},
        {NT_STATUS_LOGON_FAILURE, EACCES},

Looks like it should work to me. We really need to see what the server is sending back.
Comment 21 Guenther Deschner 2022-04-04 17:53:31 UTC
Can't check right now but from the redhat bug it is clearly NT_STATUS_INVALID_PARAMETER:

...
Failed to start GENSEC client mech gse_krb5: NT_STATUS_INTERNAL_ERROR
gensec_spnego_client_negTokenInit_step: Could not find a suitable mechtype in NEG_TOKEN_INIT
gensec_update_send: spnego[0x7f7ed801c150]: subreq: 0x7f7ed80230c0
gensec_update_done: spnego[0x7f7ed801c150]: NT_STATUS_INVALID_PARAMETER tevent_req[0x7f7ed80230c0/../../auth/gensec/spnego.c:1631]: state[3] error[-7963671676338569203 (0x917B5ACDC000000D)]  state[struct gensec_spnego_update_state (0x7f7ed8023280)] timer[(nil)] finish[../../auth/gensec/spnego.c:1947]
SPNEGO login failed: An invalid parameter was passed to a service or function.
map_errno_from_nt_status: 32 bit codes: code=c000000d
smb: do_mount - [smb://fnuc.local/finance; 0] res = -1, cancelled = 0, errno = [22] 'Invalid argument' 
smb: do_mount - (errno != EPERM && errno != EACCES), cancelled = 0, breaking
...
Comment 22 Jeremy Allison 2022-04-04 18:01:07 UTC
So that looks like an internal return from libsmbclient when krb5 isn't selected or something. But returning this back to the caller is clearly the right fix instead of hiding all error returns as EACESS.

The client should want to know that the library couldn't use krb5 due to a mis-configure IMHO.

Can we get the GNOME code fixed here instead ? I think Samba is doing the right thing.
Comment 23 Jeremy Allison 2022-04-07 01:01:35 UTC
Ping. Gunther or Andreas, can we get an update on this ? libsmbclient returning the correct error code to the caller, rather than squashing everything into EACESS seems to be a no-brainer in terms of being the right thing to do. If the caller wants to fall-back to unauthenticated connections they should always do so unless we return ECONNREFUSED.

Hard-coding an allow list or error returns in Gnome seems to be the wrong thing to do.
Comment 24 Jeremy Allison 2022-04-07 01:02:37 UTC
Adding Pavel to this in the hope he has some bandwidth :-).
Comment 25 Andreas Schneider 2022-04-13 08:46:42 UTC
So this means comsumers of libsmbclient need to handle more return codes, including EINVAL.
Comment 26 Jeremy Allison 2022-04-13 16:54:54 UTC
It means consumers of libsmbclient have to be able to handle any POSIX error returned. Having a hard-coded list of "only EACCES" means retry with anonymous credentials is poor programming practice.

The correct logic here is "if you get anything other than ECONNREFUSED then retry with anonymous".
Comment 27 Ralph Gauer 2022-04-19 18:53:05 UTC
Is it possible that this change is breaking the ability to browse the list of shares on a samba server?
Since upgrading to 4.16 both smbclient and Windows are not able to list the shares anymore - smbclient returns an empty list and Windows returns "access denied" for anonymous and authenticated users.
Access to shares directly via UNC is working normally.
"//sambaserver" fails.
"//sambaserver/share" is working.
Comment 28 Guenther Deschner 2022-04-20 09:24:52 UTC
Upstream gnome has that issue now addressed via https://gitlab.gnome.org/GNOME/gvfs/-/issues/611. The freshly released Fedora 36 brings that update already as part of gvfs in gvfs-1.50.0-4.fc36.x86_64.

Closing this bug again.
Comment 29 Andreas Schneider 2022-04-20 10:21:26 UTC
FYI: KDE is not affected!
Comment 30 Jeremy Allison 2022-04-25 16:12:02 UTC
(In reply to Guenther Deschner from comment #28)

Thanks a lot Guenther for taking care of this !
Comment 31 Samba QA Contact 2022-04-26 14:44:50 UTC
This bug was referenced in samba v4-15-stable (Release samba-4.15.7):

9b6e8ae65e24788c82022e761bfb2c78b59260d9
Comment 32 Harald Sitter 2022-04-27 22:04:19 UTC
Seems KIO is affected after all https://bugs.kde.org/show_bug.cgi?id=453090

Should this change maybe get refined to not break with the two biggest consumers of libsmbclient?
Comment 33 Jeremy Allison 2022-04-27 22:53:11 UTC
(In reply to Harald Sitter from comment #32)

Nope. The 2 biggest users were depending on very undocumented behavior. It's much better to feel the pain now and not depend on specific error codes for this sort of thing.

Make the calling libraries flexible in coping with the error returns to do what they need.

Any other way creates complete ossification of the underlying code, where no improvements are allowed due to fear of breaking some unknown caller dependency.

We can't live like that !