The following is not a final patch but at least shows the direction.

When changing in source3/libsmb/clidfs.c (marking the new lines with *)

        status = cli_tree_connect_creds(c, sharename, "?????", creds);
*        if (!NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER)) {
                if (!NT_STATUS_IS_OK(status)) {
                        d_printf("tree connect failed: %s\n", nt_errstr(status));
                        cli_shutdown(c);
                        return status;
                }
*        }

I get when running:

sudo ./smbclient -L filer -U domain/user

Enter DOMAIN\users's password:

        Sharename       Type      Comment
        ---------       ----      -------
SMB1 disabled -- no workgroup available

I guess that when querying the list, there are more checks of the same and therefore the list is not populated...

With debugging enabled:

sudo ./smbclient -L filer -U domain/user -d10

Enter DOMAIN\users's password:

        Sharename       Type      Comment
        ---------       ----      -------
signed SMB2 message
Could not connect to srvsvc pipe: NT_STATUS_NETWORK_NAME_DELETED
SMB1 disabled -- no workgroup available

The problem here is that the NetApp server is returning NT_STATUS_INVALID_PARAMETER on a signed request.

Our signing checking code can also return this on an invalid packet.

NTSTATUS smb2_signing_check_pdu(struct smb2_signing_key *signing_key,
                                enum protocol_types protocol,
                                const struct iovec *vector,
                                int count)
{
        const uint8_t *hdr;
        const uint8_t *sig;
        uint64_t session_id;
        uint8_t res[16];
        static const uint8_t zero_sig[16] = { 0, };
        int i;

        if (count < 2) {
                return NT_STATUS_INVALID_PARAMETER;
        }

        if (vector[0].iov_len != SMB2_HDR_BODY) {
                return NT_STATUS_INVALID_PARAMETER;
        }
...

According to:

https://docs.microsoft.com/en-us/archive/blogs/openspecification/smb3-secure-dialect-negotiation

NT_STATUS_INVALID_PARAMETER shouldn't be returned if the server doesn't support the FSCTL_VALIDATE_NEGOTIATE_INFO request which is what we're trying to do here to see if a man in the middle downgraded the connection. According to the MS docs the only valid returns here are:

------------------------------------------------------------------
Down-level servers (pre-Windows 2012) will return STATUS_NOT_SUPPORTED or STATUS_INVALID_DEVICE_REQUEST since they do not allow or implement FSCTL_VALIDATE_NEGOTIATE_INFO. The client should accept the response provided it’s properly signed. For SMB3-capable server, it is recommended that the server implements FSCTL_VALIDATE_NEGOTIATE_INFO. On the other-hand, when a client establishes an SMB 3.x connection, it MUST go through the FSCTL_VALIDATE_NEGOTIATE_INFO phase, provided RequireSecureNegotiate allows it.

The protocol documents that a non-SMB3-capable (2.002 or 2.1) should respond to VALIDATE_NEGOTIATE_INFO request with a status error of STATUS_NOT_SUPPORTED or STATUS_INVALID_DEVICE_REQUEST, the same error as for any unsupported/non-allowed FSCTL. Windows Server 2008 (SMB 2.002) and Windows Server 2008 R2 (SMB 2.1) return STATUS_FILE_CLOSED, instead.
------------------------------------------------------------------

We check for all these in smb2cli_validate_negotiate_info_done()
in the following code:

       status = smb2cli_ioctl_recv(subreq, state,
                                    &state->out_input_buffer,
                                    &state->out_output_buffer);
        TALLOC_FREE(subreq);
        if (NT_STATUS_EQUAL(status, NT_STATUS_FILE_CLOSED)) {
                /*
                 * The response was signed, but not supported
                 *
                 * Older Windows and Samba releases return
                 * NT_STATUS_FILE_CLOSED.
                 */
                tevent_req_done(req);
                return;
        }
        if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_DEVICE_REQUEST)) {
                /*
                 * The response was signed, but not supported
                 *
                 * This is returned by the NTVFS based Samba 4.x file server
                 * for file shares.
                 */
                tevent_req_done(req);
                return;
        }
        if (NT_STATUS_EQUAL(status, NT_STATUS_FS_DRIVER_REQUIRED)) {
                /*
                 * The response was signed, but not supported
                 *
                 * This is returned by the NTVFS based Samba 4.x file server
                 * for ipc shares.
                 */
                tevent_req_done(req);
                return;
        }
        if (NT_STATUS_EQUAL(status, NT_STATUS_NOT_SUPPORTED)) {
                /*
                 * The response was signed, but not supported
                 *
                                        * This might be returned by older Windows versions or by
                 * NetApp SMB server implementations.
                 *
                 * See
                 *
                 * https://blogs.msdn.microsoft.com/openspecification/2012/06/28/smb3-secure-dialect-negotiation/
                 *
                 */
                tevent_req_done(req);
                return;
        }
        if (tevent_req_nterror(req, status)) {
                return;
        }

Because NT_STATUS_INVALID_PARAMETER isn't listed as a proper protocol response
here we're treating it as an error.

We could modify smb2_signing_check_pdu() to avoid returning NT_STATUS_INVALID_PARAMETER
and make those errors NT_STATUS_INVALID_NETWORK_RESPONSE instead, thus allowing us
to catch NT_STATUS_INVALID_PARAMETER in smb2cli_validate_negotiate_info_done()
and treat it as signing success, but I hate having to change our code for yet
another NetApp protocol violation.

Hello Jeremy,

thanks for responding and explaining.

Yes, it is not nice, but I please you to do the change, because Microsoft itself can handle it - as you see in the provided trace. Samba would then be inline with how Microsoft is doing it.

Martin

Created attachment 16376 [details]
git-am fix for master.

Can you test this patch to see if it solves your problems ? If so I'll run a ci-build and see if Metze agrees this is safe to use.

Hello Jeremy,

I cloned the master repo, applied the patches + checked them for presence, compiled and installed samba. smb.conf is default, no changes made. This is the output of the tests made:

smbclient --version
Version 4.14.0pre1-GIT-8b4c6fb7c84

smbclient -L filer -U domain/user
Enter DOMAIN\user's password:

        Sharename       Type      Comment
        ---------       ----      -------
cli_rpc_pipe_open_noauth: rpc_pipe_bind for pipe srvsvc failed with error NT_STATUS_CONNECTION_DISCONNECTED
SMB1 disabled -- no workgroup available

(Note: there are shares available for this user, they are listed when browing via W10)

Just FYI: A customer of mine ran into that exact same issue a few weeks ago. I have patches for both issues, I just need to coordinate with Metze about the security implications for the first one, and I need to create an automated test for the second issue you just ran into: Hint: NetApp aligns the blob in the SMB2_IOCTL differently than others, and Samba is too strict in this.

Please give me a day or two.

Thanks for your patience.

Great ! That sounds like a plan. If you have time can you upload your patches here so I can take a look ?

Created attachment 16377 [details]
Additional patch from Volker.

Fix from Volker and Metze for the ioctl strange padding issue from NetApp filers.

Note, the fix here is in patch [PATCH 2/2]. The first patch is a modification to our server code to reproduce the problem and won't be in the final fix.

Hello Volker,

I did a test applying your patch #2 on a fresh git master clone and compiled it.

Following note: In can define on the NetApp fileserver if smb signing is required for smb2 or not. This is done by setting an option on the NetApp:

options cifs.smb2.signing.required on|off

Here are my results (no dependence if sudo is used or not):

sudo smbclient -L filer -U domain/user

cifs.smb2.signing.required off --> tree connect failed: NT_STATUS_ACCESS_DENIED
cifs.smb2.signing.required on --> tree connect failed: NT_STATUS_INVALID_PARAMETER

Doing the connectivity test on Windows 10 against the Netapp

cifs.smb2.signing.required off --> no connection to filer
cifs.smb2.signing.required on --> connection shows tree

Result as far I can tell:
When using cifs.smb2.signing.required off, samba reacts inline with Microsoft and denies access. Patch 2/2 is not fixing
tree connect failed: NT_STATUS_INVALID_PARAMETER

Hope this helps,
Martin

(In reply to Martin from comment #10)

Martin, you need both the git-am fix for master (fixes the NT_STATUS_INVALID_PARAMETER issue) *AND* part #2 of Volker's patch which fixes the padding issues (the cli_rpc_pipe_open_noauth: rpc_pipe_bind for pipe srvsvc failed with error NT_STATUS_CONNECTION_DISCONNECTED problem).

I'm planning to update this with a combination patch but have other work to do first.

If you want to try this yourself, apply:

https://attachments.samba.org/attachment.cgi?id=16376

first, then:

https://attachments.samba.org/attachment.cgi?id=16377

after. That should get you connected.

Hello Jeremy,

I did as you requested and can tell that I now get the tree :smile:

sudo smbclient -L filer -U domain/user
Enter DOMAIN\user's password:

        Sharename       Type      Comment
        ---------       ----      -------
        IPC$            IPC       Remote IPC
        ETC$            Disk      Remote Administration
        HOME            Disk      Default Share
        C$              Disk      Remote Administration
        ...

Please add the patches!
When you do that, what is the timeframe to expect for a stable release that can be downloaded (means not master, but from the download page).

Many thanks,
Martin

Dear Jeremy, dear Volker,

just a minor thing regarding documentation,
the NetApp OS release was not 7.4 it was 7.3.7
Sorry for the typo...

Metze has a complete fix in gitlab right now. Once this passes we can get this fixed upstream and in the next 4.12 and 4.13 releases.

This bug was referenced in samba master:

fdcdfceefdd3186ef0b70bb6e83dddc8f4c073db
0abb5ca6b96c843909dea56d5594e334547ae90f
508ed5b42c23f8b3d9730d838bd921cb73c61358
3db566026bcc0bff87acae762211e1c49220dc82
39c0d2b666a6ddac7cd3b29fe76be7375690b27b
4c6c71e1378401d66bf2ed230544a75f7b04376f

Created attachment 16398 [details]
git-am fix for 4.13.next.

Cherry-picked from master -> 4.13.next. Doesn't apply to 4.12.next as that doesn't have the smb2_ioctl_smbtorture infrastructure.

I'll create a separate back-port for 4.12.next without the test changes.

Created attachment 16399 [details]
git-am fix for 4.12.next.

Cherry-pick for 4.12.next without test changes.

This is great news Jeremy - many thanks for it.

Q1: What is the timline for a public available stable 4.12.x and 4.13.x containing this patch?

Q2: Is it possible to reproduce that behaviour using samba only? Means no NetApp running 7.3.7 as host available but samba. In case, what is necessary to do so (patching, compiling ect)? Like a kind of switch that tells: behave like...

It'll be in the next official Samba 4.13 and 4.12 releases. Karolin knows what the schedules are.

Metze added test code into the server using a specific ioctl that causes Samba to behave like a NetApp in padding. We won't behave like a NetApp in returning the NT_STATUS_INVALID_PARAMETER though.

Hi Martin,

(In reply to Martin from comment #18)

Samba 4.12.12 is scheduled for March 11 2021 and Samba 4.13.4 is scheduled for January 26 2021. If there are no security release in between or there are any issues with the autobuild, this patchset will be included (so it's very likely ;-).

Pushed to autobuild-v4-{13,12}-test.

This bug was referenced in samba v4-12-test:

2f8b1fb8aec7274b33969922e2a329c2c26aca1f
a5efe54435337463bca6a6eccabf4792745ffdca
c0a7b8c7bd23f02129839aae5541669c86ee5506
552548ff00e08ba4639b2a9b1ef978259cdb7c1a

This bug was referenced in samba v4-13-test:

bb951cd05c247d24adeb1d929a2b63333771e8d6
5e64e53fe2f8b7e8c734e89abdabc85cfa6d4e95
26e762a42e2191009c7f42bfe7b1131e8af33409
6ae3c220a93c2128d6522d02004310ca30380a63
efb811f6e4390f9f210decb2da9c59b1ca63cfee
810b019db9ed9f5e1ea49db2b1c3e4e5fcae7f5c

Closing out bug report.

Thanks!

This is an additional info for readers who use PHP accessing shares on affected NetApp systems.

Having this patch, you can now use smbclient and mount cifs successfully accessing a NetApp filer running an old version of ONTAP :applause:

Because of an incomplete implementation of the SMB2 protocol for those older
NetApp systems, an access via SMB2 via PHP is NOT possible when using `stat`.

Cause:
NetAPP systems running ONTAB 8.3 or older do not have a full implementation of SMBv2.
 
In this particular case, the NetAPP server incorrectly returns error STATUS_NOT_SUPPORTED in response to a SMB2 QUERY_INFO request of InfoType SMB2_0_INFO_FILE and FileInfoClass (a.k.a. InformationLevel) FILE_ALL_INFORMATION.

See also:

https://kb.netapp.com/?title=Advice_and_Troubleshooting%2FData_Storage_Software%2FONTAP_OS%2FLinux_smbclient_fails_to_download_file_with_error_%2522getattrib%3A_NT_STATUS_NOT_SUPPORTED%2522

Solution:
The only solution for accessing those systems via PHP is to set the `client max protocol = NT1`

Systems running ONTAP +9.x are not longer affectd.

This bug was referenced in samba v4-13-stable (Release samba-4.13.4):

bb951cd05c247d24adeb1d929a2b63333771e8d6
5e64e53fe2f8b7e8c734e89abdabc85cfa6d4e95
26e762a42e2191009c7f42bfe7b1131e8af33409
6ae3c220a93c2128d6522d02004310ca30380a63
efb811f6e4390f9f210decb2da9c59b1ca63cfee
810b019db9ed9f5e1ea49db2b1c3e4e5fcae7f5c

This bug was referenced in samba v4-12-stable (Release samba-4.12.12):

2f8b1fb8aec7274b33969922e2a329c2c26aca1f
a5efe54435337463bca6a6eccabf4792745ffdca
c0a7b8c7bd23f02129839aae5541669c86ee5506
552548ff00e08ba4639b2a9b1ef978259cdb7c1a

This bug was referenced in samba master:

b3c9823d907b91632679e6f0ffce1b7192e4b9b6
5ecac656fde4e81aa6e51e7b3134ea3fb75f564a
ef57fba5dbf359b204ba952451e1e33ed68f1c91
1faf15b3d0f41fa8a94b76d1616a4460ce0c6fa4
155348cda65b441a6c4db1ed84dbf1682d02973c

Created attachment 16683 [details]
Additional patches for v4-13-test

Created attachment 16684 [details]
Additional patches for v4-14-test

Created attachment 16685 [details]
Additional patches for v4-15-test

Created attachment 16686 [details]
Additional patches for v4-12-test (just as reference)

Karolin this is good to be pushed to 4.13.next, 4.14.next, 4.15.rc2.

Thanks !

This bug was referenced in samba v4-13-test:

e38295a091ee3e57b6ae68ab36442de04714350d
5d98e2f29608cf9c6c810f07ca136f375b4dba8e
d4d9bc847c5a12075a7e7b7c1eb23a637f8670b8
f47e9965c77ad4f1936dde71b79dec5e12add12a
5b58f6637241dfee4249d89fab3220fef59c9cfb

This bug was referenced in samba v4-15-test:

570b3ced84ae14a5e3a0f4b89bc8f2944683d6e1
0be68189ffcc746c67dd1ae0610f4b33973c8eee
b644b297bf83e49d81c97593f5e33b4dc57686dc
170b81955078c5cb9620516cfd31fe02db6f11f6
8380f21aadde1b5433b0770e8a2d9ed53b61101a

This bug was referenced in samba v4-15-stable:

570b3ced84ae14a5e3a0f4b89bc8f2944683d6e1
0be68189ffcc746c67dd1ae0610f4b33973c8eee
b644b297bf83e49d81c97593f5e33b4dc57686dc
170b81955078c5cb9620516cfd31fe02db6f11f6
8380f21aadde1b5433b0770e8a2d9ed53b61101a

This still needs to be applied to v4-14-test too...

Pushed to autobuild-v4-14-test.

This bug was referenced in samba v4-14-test:

d84d0c1095cf8dcb883bbe173bbfce11b713ae12
0d89ce25acd1768f54b216450fce754c3af2918c
cee1b839a1fd3cb164396b12576e99fcaefdb64d
a095a2d960af1730342a9fdd71c411a85efc5a67
25f3cb8c9733fb4b667ca2eb59ce3aa70b2b9c86

Closing out bug report.

Thanks!

This bug was referenced in samba v4-14-stable (Release samba-4.14.7):

d84d0c1095cf8dcb883bbe173bbfce11b713ae12
0d89ce25acd1768f54b216450fce754c3af2918c
cee1b839a1fd3cb164396b12576e99fcaefdb64d
a095a2d960af1730342a9fdd71c411a85efc5a67
25f3cb8c9733fb4b667ca2eb59ce3aa70b2b9c86

This bug was referenced in samba v4-13-stable (Release samba-4.13.11):

e38295a091ee3e57b6ae68ab36442de04714350d
5d98e2f29608cf9c6c810f07ca136f375b4dba8e
d4d9bc847c5a12075a7e7b7c1eb23a637f8670b8
f47e9965c77ad4f1936dde71b79dec5e12add12a
5b58f6637241dfee4249d89fab3220fef59c9cfb