-
-
usr/src/man/man4/smb.4 (Diff revision 1) I'd like this to be updated to reflect the fact that selecting aes128-gcm enables both aes128-gcm and aes128-ccm, preferring gcm, and selecting aes128-ccm only enables CCM.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 1) You might consider using smb_mbc_put_align(&sr->reply, 8), which makes sure that there's enough space in the MBC (this is what smb uses for other alignment cases)
-
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 1) A short comment (or #define?) indicating where the 64 comes from would be nice
-
usr/src/uts/common/fs/smbsrv/smb31_preauth.c (Diff revision 1) Is there a reason all of this function's callers ignore the return code?
-
-
usr/src/uts/common/fs/smbsrv/smb3_kdf.c (Diff revision 1) A comment (or #define) indicating where '89' comes from would be helpful
SMB 3.1.1 support for server
Review Request #2536 — Created April 15, 2020 and updated
Information | |
---|---|
ams | |
illumos-gate | |
12513 | |
Reviewers | |
general | |
Added these features:
- Preauthentication integrity
- AES-128-GCM encryption cipher
Tested using Win10 with Wireshark on client side and omnios with different combination of properties of smb/server smf service:
smbd/max_protocol ( "", "2.1", "3.02", "3.11" )
smbd/encrypt ("", "disabled", "enabled", "required")
smbd/encypt_cipher ("", "aes-128-ccm", "aes-128-gcm")
-
-
usr/src/uts/common/fs/smbsrv/smb3_encrypt_kcf.c (Diff revision 1) Does this imply a possible bug in KCF?
-
There are a few other 3.1.1 changes that appear to be mandatory that don't seem to be in this changeset. 3.3.4.4 Sending an Error Response The format for returning an error changed in 3.1.1. Error context is used by smb2_qinfo_sec (and maybe other query_info functions). See: 3.3.5.20.3 for an example. The current one is implemented by smb2sr_put_error_data(). 'NULL errors' also changed (i.e. 3.3.5.20.1 returns an error with bytecount set to 8, instead of 0) 3.3.5.4 Receiving an SMB2 NEGOTIATE Request store the entire ClientDialects array on the Connection (smb_session_t) object. 3.3.5.15.12 Handling a Validate Negotiate Info Request If 3.1.1 was negotiated, disconnect. Otherwise, compare the ClientDialects array in the request to the stored array (instead of re-calculating the best dialect and comparing to the negotiated dialect) Then there are a few other 3.1.1 changes that we might not want to implement faithfully, but are important to consider: 3.3.5.7 Receiving an SMB2 TREE_CONNECT Request "If Connection.Dialect is "3.1.1" and Session.IsAnonymous and Session.IsGuest are set to FALSE and the request is not signed or not encrypted, then the server MUST disconnect the connection." I'm not sure how strict we want this to be. You might consider enforcing this only if the server (or both client and server?) enables signing. (The spec no longer even conceptualizes a server or client that doesn't support signing, so it's not clear if any 3.1.1 clients are capable of disabling signing) 3.3.5.2.9 Verifying the Session The spec says that servers that support 3.1.1 are not supposed to allow 'optional' encryption: either it's required, or its disabled. I suppose that's not something we want to follow, though.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 1) These types are a bit odd. Several members (salt, _resrvd, data) are unused, and they're all only used in this one function. The encrypt_caps ctx is only one member. You might consider just using locals for these.
-
usr/src/uts/common/fs/smbsrv/smb2_session_setup.c (Diff revision 1) Unfortunately, Sessions and Connections are not one-to-one; Even without Multichannel, you can have more than one session per connection (and with multichannel, more than one connection per session). The Connection.preauth_hashval should only contain the negotiate request and response, and then Session.preauth_hashval should be initialized to Connection.preauth_hashval, and then updated with the Session Setup request/responses.
-
-
-
usr/src/uts/common/fs/smbsrv/smb3_encrypt_kcf.c (Diff revision 1) Probably -- unfortunately, the almost complete lack of documentation on KCF (and the relevant ARC cases never made it out) makes it hard to know which one is wrong (this has caused me no end of frustration whenever I've touched tht code).
For reading of a
crypto_data_t
, I thinkcd_offset
is supposed to be the effective offset into the blob of data (which could be a single raw buffer, split acrossmblk_t
s oriovec_t
s) andcd_length
is the total length of all the data summed across all the buffers thecrypto_data_t
contains.I'm less sure how they should be interpreted for writing -- e.g. should
cd_offset
be the current write position andcd_length
the total length of all the buffers, or shouldcd_length
be the current amount written. Though for either case, I can't point at anything to support my guesses.
Change Summary:
Fixes for most review issues, also added changes to support smb2 dtrace probes.
Change Summary:
Fixed per-connection hashvalue.
Fixed handling of negotiation info request.
Added error contexts.
Diff: |
Revision 3 (+1338 -199)
|
---|
-
-
usr/src/uts/common/fs/smbsrv/smb2_dispatch.c (Diff revisions 2 - 3) Because not all errors are 'sticky' in a compound, should these be reset to 0 at some point during compound processing? I imagine we only want to include the error contexts for the particular request/command that created them. Similiarly, I suppose it's possible for multiple requests in a compound to need to use error contexts...
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revisions 2 - 3) This should not be restricted to 3.1.1; the requirement is for 'servers that implement' 3.1.1, rather than for connections that negotiate 3.1.1. Same for the changes at line 915.
This function shouldn't ever be called on 3.1.1 - its a protocol error (3.3.5.15.12 "If Connection.Dialect is "3.1.1", the server MUST terminate the transport connection and free the Connection object")
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revisions 2 - 3) Is there a reason this isn't just a simple compare i.e.
for (int i = 0 ; i < num_dialects; i++) if (dialects[i] != s->cli_dialects[i]) goto drop;
It's simpler, and has fewer loops.
I worry that the 'hash' approach could be manipulated to obtain a value of 0, even when the arrays differ. I.e. If the 'real' string is 0x300, 0x302, An attacker could provide the values 0x212, 0x210, and this particular check would 'pass'.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revisions 2 - 3) With the dialect array comparison check (without the >= 3.1.1 check), the 'best dialect' check is no longer needed.
-
-
usr/src/uts/common/fs/smbsrv/smb31_errctx.c (Diff revision 3) I don't think we're ever supposed to increase max_bytes this way. (Encoding more than max_bytes can lead to panics elsewhere.)
If we're at the limit of the space allowed for, we should error out.
Change Summary:
fixed smb2_nego_validate(), reset of num of error contexts
Diff: |
Revision 4 (+1340 -201)
|
---|
-
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 4) In general, we should verify that the offset actually exists within the current request. The way most places do that is by using smb_mbc_decodef(..., "#.", ...) to skip to the listed offset (See http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/smbsrv/smb2_tree_connect.c#51 for an example)
In cases where we already know the offset is valid (like the above NEG_CTX_INFO_OFFSET, where we've already decoded that area once before), it's not necessary, but if we haven't verified it yet, we should.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 4) smb_mbc_put_align() modifies the buffer (it zero's the skipped data), which we probably don't want to do for the command buffer.
You could either make a 'read' equivalent of smb_mbc_put_align() that uses smb_mbc_decodef(), or just use smb_mbc_decodef() directly.
You could calculate ctx_end_off to 'round up' to an 8-byte boundary, then use smb_mbc_decodef(..., "#.", ...) to get your desired alignment, and still make sure that we're within the boundary of the request.
-
usr/src/uts/common/fs/smbsrv/smb2_negotiate.c (Diff revision 4) neg_in_ctxs and neg_out_ctxs are no longer valid after this function returns; perhaps you should NULL nego2->neg_{in,out}_ctxs after the send_reply()?
-
usr/src/uts/common/fs/smbsrv/smb31_errctx.c (Diff revision 4) Since the "C" format calls MBC_SETUP() on the passed mbc, if errdata == &sr->raw_data (as it is for all of our calls), then chain_offset will be 0 here.
I don't believe we'd currently end up with more than one error context, so it's not a problem 'right now'. However, a comment to that effect, and maybe an ASSERT(errdata != &sr->raw_data) before this ASSERT, should help future devs keep that restriction in mind.
-
usr/src/uts/common/fs/smbsrv/smb31_preauth.c (Diff revision 4) If we don't check the return code at call sites, could we log errors in this function? It'd be really nice to know 'right away' that something is going wrong here, instead of having to diagnose it from a vague 'access_denied' or signature error later.
-
usr/src/uts/common/fs/smbsrv/smb3_encrypt.c (Diff revision 4) 'mech' needs to be kmem_free()'d here; You might consider just setting rc = -1 and break'ing.
Diff: |
Revision 5 (+1371 -201)
|
---|
Change Summary:
Simplification of error contexts. No support for multiple ones, because we don't need it currently.
Diff: |
Revision 6 (+1287 -197)
|
---|
-
-
usr/src/uts/common/fs/smbsrv/smb2_dispatch.c (Diff revisions 4 - 6) Shouldn't this be "wbblllC"?
(likewise the args...)
Change Summary:
error context ByteCount, Reserved fields encoding fix
Diff: |
Revision 7 (+1289 -197)
|
---|