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.
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.
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.
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
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 ... ... ... }
Ok, that's amazingly strange... I'll take a look. Jeremy.
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
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.
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.
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.
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.
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.
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.
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.
Quick question - is this already fixed in 4.0.x and master ? If not we also need a patch there. Jeremy.
(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.
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.
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);
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.
Created attachment 8495 [details] A potential fix This is a potential fix that looks better than the previous ones I suggested.
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.
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.
(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.
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.
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.
(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 :-)
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.
Re-assigning to Karolin for inclusion in 3.6.next. Jeremy.
Pushed to v3-6-test. Closing out bug report. Thanks!
We have tested this patch and it seems to work.