Bug 9585 - Samba 3.6.x not correctly signing any but the last response in a compound request/response
Samba 3.6.x not correctly signing any but the last response in a compound req...
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: SMB2
3.6.6
All FreeBSD
: P5 critical
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-23 10:24 UTC by Hemanth
Modified: 2013-05-07 08:57 UTC (History)
2 users (show)

See Also:


Attachments
packet capture during the problem & screenshot (487.69 KB, application/x-zip-compressed)
2013-01-23 10:24 UTC, Hemanth
no flags Details
Shows that the signature in the response is copied from the request (977 bytes, application/octet-stream)
2013-01-24 02:02 UTC, Richard Sharpe
no flags Details
A potential fix (1.21 KB, patch)
2013-01-26 23:38 UTC, Richard Sharpe
no flags Details
A better fix ... (1.56 KB, patch)
2013-01-27 12:36 UTC, Richard Sharpe
no flags Details
Alternate fix for 3.6.x (661 bytes, patch)
2013-01-28 19:48 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.x. (1.39 KB, patch)
2013-01-29 00:02 UTC, Jeremy Allison
rsharpe: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hemanth 2013-01-23 10:24:50 UTC
Created attachment 8470 [details]
packet capture during the problem & screenshot

Setup & Configuration:

FreeNAS(FreeBSD8.3) server with Samba 3.6.7 stack. Max protocol set to SMB2.
Win 7: Manually set the RequireSecuritySignatiure to 1.

Issue:

When the Client(win 7) side signing set to required(RequireSecuritySignatiure=1) we are unable to perform certain fie operations like unable to open file from browser, unable to list the files from command line using dir, etc. Where as we are able to create file, rename, delete, etc.

For file open it says "The specified server can not perform the requested operation". (Attached the screenshot along this bug)

Though I have files "dir" says File not found.

Z:\>dir
 Volume in drive Z is testshare
 Volume Serial Number is 51EE-A039

 Directory of Z:\

01/23/2013  02:56 PM    <DIR>          .
File Not Found

Z:\>

Steps to reproduce:
- Export a share on FreeNAS server where Samba 3.6.7 running with SMB2 enabled.
- In a windows 7 client enable the RequiredSecutiySignature bit and reboot the client.
- After reboot map to the share crated in step 1. 
- Open an existing file(can be simple text file) or create a new file and open the same. You will an error saying "The specified server can not perform the requested operation".
- From command line try listing the files/folders in the mapped drive. It says "File not found" thuogh you have some data in it.


Expectation:

Ideally File operations should be working fine even when Signing is mandated(either from Server or Client).

Did not any patches post 3.6.7 addressing this issue. Please let me know if any of the latest patches address this.

Observation on SMB1 & SMB2 signing:

Unlike SMB1 in SMB2 When singing is set to "Not Required"(Enabled:1, Required:0) on both server and client, we could see the sessions not using the signatures. Packets are getting signed only on mandating(Required:1) either or both server and client sides.

Attachments:

- Screen shot of file open error.
- Packet capture during this problem.
Comment 1 Richard Sharpe 2013-01-23 17:38:55 UTC
Further info.

We believe that a GetInfo GetFSInfo shows this problem. We are going to write a small Windows program that demos the problem because we have isolated, using ProcMon, what Win32 request the client was making.

I will try to get that done today.
Comment 2 Richard Sharpe 2013-01-23 21:33:01 UTC
It looks like one of the problems we are seeing are with GetINFO SMB2_FS_INFO_01.

The client issues a GetInfo FS_INFO_01;GetInfo FS_INFO_05 and then reissues the GetInfo FS_INFO_01. After that the test program gets an error from

Will try to get the test code dropped into the bug.
Comment 3 Richard Sharpe 2013-01-23 22:03:14 UTC
Fundamentally, we are calling CreateFile on a file passed in on the command line and then calling GetFileInformationByHandle:

http://msdn.microsoft.com/en-us/library/windows/desktop/aa364952(v=vs.85).aspx
Comment 4 Richard Sharpe 2013-01-24 00:43:43 UTC
This is the code we use to test with:

int _tmain(int argc, _TCHAR* argv[])
{
	OFSTRUCT of;
	BY_HANDLE_FILE_INFORMATION fileInformation;
	HFILE hFile = OpenFile(argv[1],&of, OF_READ);
	if(!GetFileInformationByHandle((HANDLE)hFile, &fileInformation))
	{
		printf("Cannot get file information...\n");
		return 1;
	}
	CloseHandle((HANDLE)hFile);
	return 0;
}

This is from Windows.

We have found that by removing the code that fills in the qfsinfo and things start working.

static void call_trans2qfsinfo(connection_struct *conn,
                  struct smb_request *req,
                   char **pparams, int total_params,
                   char **ppdata, int total_data,
                   unsigned int max_data_bytes) { ...
...
...
DEBUG(3,("call_trans2qfsinfo: level = %d\n", info_level));

#if 0 
     status = smbd_do_qfsinfo(conn, req,
                  info_level,
                  req->flags2,
                  max_data_bytes,
                  ppdata, &data_len);
     if (!NT_STATUS_IS_OK(status)) {
         reply_nterror(req, status);
         return;
     }
#endif
...
...
...
}
Comment 5 Jeremy Allison 2013-01-24 00:46:45 UTC
Ok, that's amazingly strange...

I'll take a look.

Jeremy.
Comment 6 Richard Sharpe 2013-01-24 01:08:52 UTC
OK, wrong code was listed. It was this code that was hacked out:

        case 0x02:/* SMB2_GETINFO_FS */
        {
                uint16_t file_info_level;
                char *data = NULL;
                int data_size = 0;

                /* the levels directly map to the passthru levels */
                file_info_level = in_file_info_class + 1000;

#if 0
                status = smbd_do_qfsinfo(conn, state,
                                         file_info_level,
                                         STR_UNICODE,
                                         in_output_buffer_length,
                                         &data,
                                         &data_size);
                if (!NT_STATUS_IS_OK(status)) {
                        SAFE_FREE(data);
                        if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_LEVEL)) {
                                status = NT_STATUS_INVALID_INFO_CLASS;
                        }
                        tevent_req_nterror(req, status);
                        return tevent_req_post(req, ev);
                }
#endif
Comment 7 Jeremy Allison 2013-01-24 01:11:34 UTC
Hmmm. So what you're saying is returning any actual data (i.e. a non-zero length) for the FS_INFO/SMB2_FS_INFO_01 response is causing the client to drop the connection ?

Can you post a packet capture of the same SMB2 signed request running against a Windows server please ?

I'm wondering if we have the response wrong in some way which is only triggering an error path in the signed case.

Jeremy.
Comment 8 Richard Sharpe 2013-01-24 01:18:41 UTC
More info. Looks like this might be related to compound requests. There is a GetInfo FS_INFO_01 and GetInfo FS_INFO_05 pair there.
Comment 9 Jeremy Allison 2013-01-24 01:24:18 UTC
What is the packet number in the trace you sent that causes the client to give an error ?

The last request/response pair I see is in 1607/1609 which *isn't* a compound request.

Jeremy.
Comment 10 Richard Sharpe 2013-01-24 01:35:15 UTC
Is it possible that in static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2_request *req) we should sign the PDU before scheduling the immediate request to process the rest of the compound request?

Otherwise it seems like we might not correctly sign all but the last request in a compound request.
Comment 11 Richard Sharpe 2013-01-24 01:58:29 UTC
OK, yes, I believe that the problem is in: smbd_smb2_request_reply.

We need to hoist the code that does signing:

        if (req->do_signing) {
                NTSTATUS status;
                status = smb2_signing_sign_pdu(req->session->session_key,
                                               &req->out.vector[i], 3);
                if (!NT_STATUS_IS_OK(status)) {
                        return status;
                }
        }

to above the check for a compound request. We might also need to hoist the code for calculating credits above the check for a compound request as well.
Comment 12 Richard Sharpe 2013-01-24 02:02:36 UTC
Created attachment 8474 [details]
Shows that the signature in the response is copied from the request

This capture shows that the signature in the first GetInfo request in this compound request is the same as the signature in the first GetInfo response.

The correct signature in the first response did not get calculated.
Comment 13 Richard Sharpe 2013-01-24 03:05:22 UTC
OK, it looks like hoisting the signing will not work because smb2_calculate_credits goes through and calculates credits for each response in the compound response.

Essentially we will need to do the same. That is, go through an calculate the signature for each response. However, there are several places where signing is called. So, it might be better to calculate credits only for the current PDU.

I think something like:

diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index a0e390e..199fbce 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -1822,10 +1822,15 @@ static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2
 
        if (req->do_signing) {
                NTSTATUS status;
-               status = smb2_signing_sign_pdu(req->session->session_key,
-                                              &req->out.vector[i], 3);
-               if (!NT_STATUS_IS_OK(status)) {
-                       return status;
+               int idx = 0;
+               int count = outreq->out.vector_count;
+
+               for (idx=1; idx < count; idx += 3) {
+                       status = smb2_signing_sign_pdu(req->session->session_key
+                                              &req->out.vector[idx], 3);
+                       if (!NT_STATUS_IS_OK(status)) {
+                               return status;
+                       }
                }
        }

However, I think it might be better to rework calculate credits and have credits calculated for each request as the reply is generated rather than doing them all in one fell swoop.
Comment 14 Jeremy Allison 2013-01-24 17:00:46 UTC
Quick question - is this already fixed in 4.0.x and master ? If not we also need a patch there.

Jeremy.
Comment 15 Richard Sharpe 2013-01-24 17:07:11 UTC
(In reply to comment #14)
> Quick question - is this already fixed in 4.0.x and master ? If not we also
> need a patch there.

OK, looking at master it seems that it does the signing for all but the last response before it makes the decision to handle the remaining requests in the compound request, and then calculates the credits and signs the last response after the check for whether or not this is the last response.

This seems a bit clunky, but will surely work.
Comment 16 Richard Sharpe 2013-01-24 17:34:39 UTC
Just a note. 3.3.4.1.3 of [MS-SMB2].pdf says:

"The server MAY<171> grant credits separately on each response in the compounded chain. Then the entire response chain MUST be sent to the client as a single submission to the underlying transport."

However, footnote 171 says:

"<171> Section 3.3.4.1.3: Windows-based servers grant all credits in the final response of the compounded chain, and grant 0 credits in all responses other than the final response."

This means that we are not bound to do it the way Windows does.
Comment 17 Richard Sharpe 2013-01-24 21:09:01 UTC
This patch works, but I am unhappy with it:

-bash-4.0$ diff -up build/cloudcc/build_x86_64/samba-3.6.6/source3/smbd/smb2_server.c.orig build/cloudcc/build_x86_64/samba-3.6.6/source3/smbd/smb2_server.c
--- build/cloudcc/build_x86_64/samba-3.6.6/source3/smbd/smb2_server.c.orig     2013-01-24 20:36:20.842019947 -0800
+++ build/cloudcc/build_x86_64/samba-3.6.6/source3/smbd/smb2_server.c   2013-01-24 20:36:51.865124838 -0800
@@ -1724,6 +1724,19 @@ static NTSTATUS smbd_smb2_request_reply(

        req->current_idx += 3;

+       /* Set credit for these operations (zero credits if this
+          is a final reply for an async operation). */
+       smb2_calculate_credits(req, req);
+
+       if (req->do_signing) {
+               NTSTATUS status;
+               status = smb2_signing_sign_pdu(req->session->session_key,
+                                              &req->out.vector[i], 3);
+               if (!NT_STATUS_IS_OK(status)) {
+                       return status;
+               }
+       }
+
        if (req->current_idx < req->out.vector_count) {
                /*
                 * We must process the remaining compound
@@ -1750,19 +1763,6 @@ static NTSTATUS smbd_smb2_request_reply(

        smb2_setup_nbt_length(req->out.vector, req->out.vector_count);

-       /* Set credit for these operations (zero credits if this
-          is a final reply for an async operation). */
-       smb2_calculate_credits(req, req);
-
-       if (req->do_signing) {
-               NTSTATUS status;
-               status = smb2_signing_sign_pdu(req->session->session_key,
-                                              &req->out.vector[i], 3);
-               if (!NT_STATUS_IS_OK(status)) {
-                       return status;
-               }
-       }
-
        if (DEBUGLEVEL >= 10) {
                dbgtext("smbd_smb2_request_reply: sending...\n");
                print_req_vectors(req);
Comment 18 Jeremy Allison 2013-01-26 01:14:39 UTC
I don't think your patch is correct.

Calling smb2_calculate_credits(req, req) multiple times is not idempotent, as the code inside smb2_set_operation_credit() changes:

       sconn->smb2.credits_granted += credits_granted;
       sconn->smb2.seqnum_range += credits_granted;

sconn->smb2.credits_granted and sconn->smb2.seqnum_range are global variables, so you're updating them on every smb2_calculate_credits() call.

Jeremy.
Comment 19 Richard Sharpe 2013-01-26 23:38:08 UTC
Created attachment 8495 [details]
A potential fix

This is a potential fix that looks better than the previous ones I suggested.
Comment 20 Richard Sharpe 2013-01-27 00:13:53 UTC
I notice that there are tests for compound requests in the master branch and one of them contains the following interesting comment:

        status = smb2_create_recv(req[0], tree, &cr);
        /* TODO: check why this fails with --signing=required */
        CHECK_STATUS(status, NT_STATUS_INVALID_PARAMETER);
        status = smb2_close_recv(req[1], &cl);
        CHECK_STATUS(status, NT_STATUS_INVALID_PARAMETER);
        status = smb2_close_recv(req[2], &cl);
        CHECK_STATUS(status, NT_STATUS_FILE_CLOSED);

        smb2_util_unlink(tree, fname);

We should be able test that it is correct now, anyway.
Comment 21 Jeremy Allison 2013-01-27 05:00:31 UTC
Ok, I'll check this more fully on Monday, but I think there's a more elegant fix. I think we can do it the same way for 3.6.x that we do it in master/4.0.x.

Give me Monday morning to get this evaluated more fully. I think your fix works for the ordinary case, but at first glance I think it may miss the case where the last element in a compound goes async (which does happen).

Jeremy.
Comment 22 Richard Sharpe 2013-01-27 05:15:49 UTC
(In reply to comment #21)
> Ok, I'll check this more fully on Monday, but I think there's a more elegant
> fix. I think we can do it the same way for 3.6.x that we do it in master/4.0.x.
> 
> Give me Monday morning to get this evaluated more fully. I think your fix works
> for the ordinary case, but at first glance I think it may miss the case where
> the last element in a compound goes async (which does happen).

Yeah, I forgot about that.
Comment 23 Richard Sharpe 2013-01-27 12:36:44 UTC
Created attachment 8496 [details]
A better fix ...

OK, this is a better patch that overcomes your objection, I think.

However, I suspect that there is still one more change to make, and that is to use the same signing function in the remaining place where signing is done.
Comment 24 Jeremy Allison 2013-01-28 19:48:42 UTC
Created attachment 8507 [details]
Alternate fix for 3.6.x

Ok, your fix is going to work, but it changes the way we do the signing w.r.t. the way it's done in 4.0.x and master.

I think this change is smaller and achieves the same effect in the same way that master and 4.0.x does it.

But right now it's a toss-up between the two.

What do you think ?

Jeremy.
Comment 25 Richard Sharpe 2013-01-28 19:57:58 UTC
(In reply to comment #24)
> Created attachment 8507 [details]
> Alternate fix for 3.6.x
> 
> Ok, your fix is going to work, but it changes the way we do the signing w.r.t.
> the way it's done in 4.0.x and master.
> 
> I think this change is smaller and achieves the same effect in the same way
> that master and 4.0.x does it.
> 
> But right now it's a toss-up between the two.
> 
> What do you think ?

OK, that will work, I think, but you might need to add comments explaining what is going on :-)
Comment 26 Jeremy Allison 2013-01-29 00:02:24 UTC
Created attachment 8510 [details]
git-am fix for 3.6.x.

Ok, +1 this one (with additional comment :-) and I'll re-assign to Karolin for inclusion in 3.6.next.

Cheers,

Jeremy.
Comment 27 Jeremy Allison 2013-01-29 00:18:51 UTC
Re-assigning to Karolin for inclusion in 3.6.next.
Jeremy.
Comment 28 Karolin Seeger 2013-01-29 11:42:05 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!
Comment 29 Richard Sharpe 2013-01-29 18:31:52 UTC
We have tested this patch and it seems to work.