People: |
|
---|
11665 SMB2 NEGOTIATE Security Mode handling is wrong
Review Request #2287 — Created Sept. 9, 2019 and submitted
Information | |
---|---|
andy_js | |
illumos-gate | |
11659, 11665, 11670 | |
Reviewers | |
general | |
gdamore, gwr |
11665 SMB2 NEGOTIATE Security Mode handling is wrong
11659 SMB2 protocol version negotiation needs work
11670 SMB2_FLAGS_SIGNED is not valid during NEGOTIATE
Before this change, running the Windows Protocol Test Suite with only the SMB2/3 Negotiate tests enable yields a result of:
59 / 107 Passed, 41 Failed, 7 Inconclusive
After this fix the result is:
100 / 107 Passed, 0 Failed, 7 Inconclusive
The inconclusive tests are inconclusive because the require SMB 3.1 support.
-
There was one weird smbtorture test that this code was dealing with. Let me also check on that and get back to you.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 1) How about we remove the return(-1) but leave the rest of the if (...status != 0) block?
I believe the spec. still calls out some "invalid parameter" cases for which we might need to do an smb2sr_put_error call (at some point in the future).
e.g. no matching protocol, I think. I know I've seen Windows return invalid protocol to negotiate, so I know it's possible.
I'd rather not have smb2_status ignored here.
-
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 2) Oh, sorry. I advised you incorrectly.
If we want to handle (e.g.) NT_STATUS_INVALID_PARAMETER here we need to do the put error here AND return zero.Perhaps also add a comment here that there are no protocol failures after this point.
The rc!=0 test is just for the (probably impossible) case of running out of message space.
Description: |
|
---|
-
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 2) OK, I think I'd recommend adding the put error call here, just before the send reply call:
if (sr->smb2_status != 0) smb2sr_put_error(sr, sr->smb2_status);
-
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 4) Also, if failure in smb2_negotiate_common is not going to cause connection drop, it might be better to change the return to a ulong NT_STATUS and let callers do the smb2sr_put_error like most other places.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Bugs: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+49 -45) |
Testing Done: |
|
---|
Testing Done: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
People: |
|
-
This is a good start, but I'd like to ask for some restructuring.
We've tried to always place the START probes after everythign has been decoded (unless that's impractical for some reason) so that the START probe can examine the fully decoded state.
See smb2_create for a complex example of the expected code structure.In general, every SMB2 dispatch handler should be structured like this:
- Variable inializations and allocations
- Decode fixed parts.
- Decode variable parts. (generally, decode errors are a protocol violation and should cause a disconnect, as "all bets are off". Generally disconnect happens by returning a non-zero "dispatch code" to the common dispatch function.)
- Call the dtrace START probe
- Do error checking on what we decoded (errors after the START probe should goto a label just before the DONE probe)
- Execute the "action" for the request, usually via some sub-function (possibly shared with SMB1)
- Call the dtrace DONE probe
- Encode the results (either success or failure). In general, encoding should not fail.
- Common cleanup of locals (may be a label here for goto from decode section errors)
- Return to common dispatch (where the reply is sent, either immediately or at the end of a compound).
Note that Negotiate is unusual in that it's not called by the common dispatch function.
As written smb2_negotiate_common was trying to be like a normal dispatch handler function,
and smb2_newrq_negotiate and smb1_negotiate_smb2 were intended to contain the actions that
would normally be expected in the caller of a normal dispatch handler function.Sorry that wasn't more clear. I thought about putting that in a big comment somewhere, but I'm not sure where it would go that people would actually see it. Reach out to me (privately) if you'd like to talk about any of this. Thanks! -GWR
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 5) See later comment about doing more of this in smb2_negotiate_common() if possible.
Also a nit: why insert this one assignment here vs after the assignment of smb2_cmd_code?
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 5) See my comment in smb2_negotiate_common, relevant to these additions.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 5) This move is breaking some rules (OK, unwritten rules) about the dtrace probes. See my longer comment about this in the general part of the review.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 5) Would it be difficult to keep all the "build an SMB2 reply" work here in smb2_negotiate_common()?
That was the intent of this funciton.
Change Summary:
Take care not to mark the session as NEGOTIATED when smb2_negotiate_common fails.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Bugs: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 7 (+65 -50) |
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Bugs: |
|
-
Thanks for simplifying this change. I have just a few bits of feedback remaining.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 9) I think we want the final (overwrite=true) call to smb2_encode_header() after smb2_status is set and after any calls to smb2sr_put_error(sr, status);
(Right before smb2_send_reply)At least that's how it happens elsewhere.
Same for the later call at line 317 -
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 9) Did you forget to limit the version_cnt value that's passed to smb_mbc_decodef?
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 9) If you change this to
sr->smb2_hdr_flags |= SMB2_FLAGS_SERVER_TO_REDIR;
you could get rid of saved_hdr_flags.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 9) Same comment as on the earlier smb2_encode_header call. (line 165)
-
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 9) One other thing I just noticed. We really should be checking SMB2_FLAGS_SERVER_TO_REDIR here too (like smb2_dispatch does). This is also a good place to do the "reserve header space" too (also like dispatch)
+ if ((sr->smb2_hdr_flags & SMB2_FLAGS_SERVER_TO_REDIR) != 0) + return (-1); + sr->smb2_hdr_flags |= SMB2_FLAGS_SERVER_TO_REDIR; + + /* Reserve space for the reply header. */ + (void) smb2_encode_header(sr, B_FALSE);