Bug 14176 - NT_STATUS_ACCESS_DENIED becomes EINVAL when using SMB2 in SMBC_opendir_ctx
NT_STATUS_ACCESS_DENIED becomes EINVAL when using SMB2 in SMBC_opendir_ctx
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient
4.10.9
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on: 14174
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-30 09:02 UTC by Elia Geretto
Modified: 2019-11-26 12:23 UTC (History)
2 users (show)

See Also:


Attachments
Call stack of the functions that do not set NT_STATUS_ACCESS_DENIED (1.56 KB, text/plain)
2019-10-30 09:02 UTC, Elia Geretto
no flags Details
Log generated when triggering the problem (4.27 KB, text/plain)
2019-10-30 09:19 UTC, Elia Geretto
no flags Details
git-am fix for master. (3.49 KB, patch)
2019-10-31 23:09 UTC, Jeremy Allison
no flags Details
git-am fix for 4.11.next, 4.10.next. (3.73 KB, patch)
2019-11-18 17:45 UTC, Jeremy Allison
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Elia Geretto 2019-10-30 09:02:05 UTC
Created attachment 15585 [details]
Call stack of the functions that do not set NT_STATUS_ACCESS_DENIED

As stated in the title, the symptom of this bug is the following: if an SMB2 share, protected with username and password, is accessed with the wrong credentials using SMBC_opendir_ctx, errno is set to EINVAL instead of EACCES.

The problem is in the following chunk of code: https://gitlab.com/samba-team/samba/blob/master/source3/libsmb/libsmb_dir.c#L855-872

In particular, it appears that net_share_enum_rpc is not setting correctly the NTSTATUS in srv->cli, which is then used to select the correct return value by cli_errno.

Looking at net_share_enum_rpc (https://gitlab.com/samba-team/samba/blob/master/source3/libsmb/libsmb_dir.c#L376-381), even if the return value of cli_rpc_pipe_open_noauth is NT_STATUS_ACCESS_DENIED (I verified this with a debugger), the NTSTATUS inside cli is not set to NT_STATUS_ACCESS_DENIED and the error is dropped, returning -1.

Looking then at the call stack (attached) to check where the NT_STATUS_ACCESS_DENIED comes from, none of the functions in it is setting the NTSTATUS inside the cli_state.

This bug appears to be similar to bug 11276, so the fix is probably to set the error in cli_state in one of the functions in the call stack. It is probably worth verifying if this happens in other places as well: functions that return an NTSTATUS without setting it in cli_state, assuming that their caller will do it.

One last note, this bug is probably masked by bug 14174, which is likely to make the SMB1 function correct the missing NTSTATUS.
Comment 1 Elia Geretto 2019-10-30 09:19:07 UTC
Created attachment 15586 [details]
Log generated when triggering the problem

The log has been generated setting "client min protocol = SMB2" to work around bug 14174. The call to SMBC_opendir_ctx comes from GNOME Control Center, in the Printers panel (https://gitlab.gnome.org/GNOME/gnome-control-center/blob/master/panels/printers/pp-samba.c#L243), which is currently broken with SMB2-only servers.
Comment 2 Jeremy Allison 2019-10-30 18:01:39 UTC
Got it. Error handling inside libsmbclient is messed up in a few places.

Most SMB2 code paths set cli->raw_status, almost none of the SMB1 code paths do.

I'll fix this one by creating a patch to get net_share_enum_rpc() to set cli->raw_status on error, then log a new bug to add setting cli->raw_status inside all the synchronous SMB1 cli_XXXX() calls.
Comment 3 Elia Geretto 2019-10-30 19:52:51 UTC
Thank you very much for you reply! I am looking forward to the fix.
Comment 4 Jeremy Allison 2019-10-31 22:08:06 UTC
(In reply to Jeremy Allison from comment #2)

> Most SMB2 code paths set cli->raw_status, almost none of the SMB1 code paths do.

On close inspection this is incorrect. I'm just going to fix this specific bug for this bug report.
Comment 5 Jeremy Allison 2019-10-31 23:09:07 UTC
Created attachment 15594 [details]
git-am fix for master.

Can you confirm this fixes the problem. It's in gitlab-CI right now.
Comment 6 Elia Geretto 2019-11-01 12:24:21 UTC
I tested the patch on top of master on Fedora 30. I can confirm it fixes the bug in this report. It is not enough to fix the problem in GNOME Control Center, but the subsequent issues do not appear to be related to this project.
Comment 7 Jeremy Allison 2019-11-18 17:45:56 UTC
Created attachment 15624 [details]
git-am fix for 4.11.next, 4.10.next.

Cherry-pick from master. Applies cleanly to 4.11.next, 4.10.next.
Comment 8 Andreas Schneider 2019-11-18 18:40:05 UTC
Karolin, could you please apply the patches to the relevant branches? Thanks!
Comment 9 Karolin Seeger 2019-11-19 08:50:21 UTC
(In reply to Andreas Schneider from comment #8)
Pushed to autobuild-v4-{11,10}-test.
Comment 10 Karolin Seeger 2019-11-26 12:23:52 UTC
(In reply to Karolin Seeger from comment #9)
Pushed to both branches.
Closing out bug report.

Thanks!