Difference between revisions of "Ksmbd-review"

From SambaWiki
m
Line 2: Line 2:
   
 
*packet processing checks to make sure buffers do not overflow, allowing access to data outside the SMB request
 
*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
+
*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)
 
*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)
   
Line 14: Line 14:
 
|-
 
|-
 
|SMB3 header validation
 
|SMB3 header validation
  +
| Already implemented
|
 
  +
| See ksmbd_check_message function in smb2misc.c and ksmbd_verify_smb_message
|
 
 
|
 
|
 
|-
 
|-
|StructureSize field check for 19 SMB3 commands
+
|StructureSize field check for all 19 SMB3 commands
| already implemented
+
| Already implemented
| smb2_req_struct_sizes and smb2_get_data_area_len in smb2misc.c
+
| See smb2_get_data_area_len function and smb2_req_struct_sizes in smb2misc.c
 
|
 
|
 
|-
 
|-
Line 34: Line 34:
 
|-
 
|-
 
|Generic check for packet overflow
 
|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)
 
|Packet Signing Check (for packet corruption and man-in-the-middle attacks)
| already implemented
+
| Already implemented
 
| check_sign_req function in smb2pdu.c
 
| check_sign_req function in smb2pdu.c
 
|
 
|
Line 54: Line 54:
 
|-
 
|-
 
|Check for negotiate context overflow
 
|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
|
 
 
|-
 
|-
 
|}
 
|}
Line 80: Line 80:
 
|checks for resolved path never being / or \ (outside the share)
 
|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)
|
 
 
|
 
|
 
|-
 
|-
Line 96: Line 96:
 
|checks for opening too many files
 
|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)
 
|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
|
 
 
|
 
|
 
|-
 
|-

Revision as of 18:55, 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)
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 "ksmbd: prevent out of share access"
checks for symlinks in path components 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)
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