Ksmbd-review: Difference between revisions

From SambaWiki
mNo edit summary
mNo edit summary
Line 74: Line 74:
|checks for .. escaping the share
|checks for .. escaping the share
| patch submitted, reviewed, tested multiple ways (libsmb2, smbclient)
| patch submitted, reviewed, tested multiple ways (libsmb2, smbclient)
| "ksmbd: prevent out of share access"
| "ksmbd: prevent out of share access" (see ksmbd_conv_path_to_unix function in misc.c and its use in smb2pdu.c)
|
|
|-
|-
|checks for symlinks in path components
|checks for symlinks in path components
| patch submitted, partially reviewed
| patch submitted, partially reviewed
| "ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup" (see ksmbd_conv_path_to_unix function in misc.c and its use in smb2pdu.c)
| "ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup"
|
|
|-
|-

Revision as of 19:26, 20 September 2021

There are various checks that are especially important for SMB3 servers. They fall into multiple categories:

  • packet processing checks to make sure buffers do not overflow, allowing access to data outside the SMB request
  • path processing checks to make sure access to files outside the share is not permitted, these include checks for ".." or "../.." to make sure they do not go outside the share, and checks for symlinks in paths and checks to ensure paths do not allow a processed path e.g. with "\" or "/" to go outside the share. These primarily impact three operations (open, query directory and rename) as unlike SMB1 most SMB2/SMB3 operations are handle based, not path based.
  • denial of service and resource checks (e.g. ensure that no single client can exhaust the server by opening millions of files, or use so many credits that they submit thousands of simultaneous requests starving other clients)

In addition, it is important that servers only implement reasonably secure dialects and authentication mechanisms by default. For example SMB2.1 or later supports protection against "man in the middle" attacks, and should be the minimum version supported by default. In addition, NTLMv1 authentication should be disabled (NTLMv2 can be allowed, although 8 character and longer passwords may be a good minimum to require by default). Kerberos authentication should also be supported. In the longer run, especially to support Macs (which allow at least two additional peer-to-peer authentication mechanisms which look promising) we can consider adding additional mechanisms, but currently NTLMv2 (encapsulated in NTLMSSP during negotiation) is for "peer to peer" use cases, and Kerberos for domain joined cases.

SMB3.1.1 supports very strong signing and encryption options (e.g. GCM256 encryption), and it may be helpful to ensure that users know about the advantages of using current SMB3.1.1 so they don't choose less secure options.

Packet processing checks in ksmbd
Type of check Status (submitted, reviewed) Function(s) including the check (and patch name if not included yet) Additional work if any
SMB3 header validation Already implemented See ksmbd_check_message function in smb2misc.c and ksmbd_verify_smb_message
StructureSize field check for all 19 SMB3 commands Already implemented See smb2_get_data_area_len function and smb2_req_struct_sizes in smb2misc.c
check for overflow of SET_INFO command patches submitted, partially reviewed "ksmbd: add request buffer validation in smb2_set_info"
check for overflow of FSCTL command patch submitted, partially reviewed "ksmbd: add validation in smb2_ioctl", smb2pdu.c
Generic check for packet overflow Already implmented See ksmb2_check_message and smb2_calc_size functions
Packet Signing Check (for packet corruption and man-in-the-middle attacks) Already implemented check_sign_req function in smb2pdu.c
Check for open context overflow patch submitted, partially reviewed See "ksmbd: add buffer validation for SMB2_CREATE_CONTEXT", oplock.c smb2pdu.c
Check for overflow of ACL patch submitted, partially reviewed See "ksmbd: add buffer validation for SMB2_CREATE_CONTEXT", smbacl.c (parse_dacl and parse_sec_desc functions)
Check for negotiate context overflow Already implemented See deassemble_neg_contexts function (in smb2pdu.c) and also decode_encrypt_context and decode_sign_cap_ctxt Some contexts (such as compression context) are ignored (set to none) and may need additional checks in future when implemented
Path processing checks in ksmbd
Type of check Status (submitted, reviewed) Function(s) including the check (and patch name if not included yet) Additional work if any
checks for .. escaping the share patch submitted, reviewed, tested multiple ways (libsmb2, smbclient) "ksmbd: prevent out of share access" (see ksmbd_conv_path_to_unix function in misc.c and its use in smb2pdu.c)
checks for symlinks in path components patch submitted, partially reviewed "ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup"
checks for resolved path never being / or \ (outside the share) (also see additional restrictions made possible by ksmbd_share_veto_filename function in mgmt/share_config.c, and ksmbd_validate_filename in misc.c)


Denial of Service checks
Type of check Status (submitted, reviewed) Function(s) including the check (and patch name if not included yet) Additional work if any
checks for opening too many files Would additional checks be helpful e.g. see smb2_open in smb2pdu.c and ksmbd_open_fd in vfs_cache.c
checks for allowing too many requests (crediting) See smb2_set_rsp_credits and conn->max_credits field, and conn->total_credits in smb2_consume_credit_charge to investigate this