jborean93 / smbprotocol

Python SMBv2 and v3 Client
MIT License
320 stars 74 forks source link

Respect ace_count while unpacking ACEs #143

Closed MWedl closed 3 years ago

MWedl commented 3 years ago

AclPacket._unpack_aces() does not respect the ace_count field. This method keeps parsing ACEs until there is no data left in the buffer. Most of the time this works fine, because normally there is no data after ace_count ACEs. However, while unpacking DACLs of IPC$ shares there is data left. This causes _unpack_aces to crash or sometimes to run into endless loops. I don't know what the exceeding data after the ACEs is, nor have I found any documentation about it, but they are certainly no ACEs.

Example:

AclPacket:
  acl_revision: 0x02
  sbz1: 0x00
  acl_size: 0x0408 (size of 3 ACEs + exceeding data at end)
  ace_count: 0x0003
  sbz2: 0x0000
  aces: 3 valid ACEs (total size of all 3 ACEs: 60 bytes)
  exceeding unknown data (964 bytes)
jborean93 commented 3 years ago

The change is fine but I'm wondering whether it's possible to add a test to unpack your data to https://github.com/jborean93/smbprotocol/blob/master/tests/test_security_descriptor.py. This will ensure that this mistake isn't made again in the future if the code is ever changed.

MWedl commented 3 years ago

Thanks for the feedback. I have added a test case.

codecov[bot] commented 3 years ago

Codecov Report

Merging #143 (fb2257c) into master (1f1b52a) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #143   +/-   ##
=======================================
  Coverage   98.91%   98.91%           
=======================================
  Files          23       23           
  Lines        4989     4989           
=======================================
  Hits         4935     4935           
  Misses         54       54           
Impacted Files Coverage Δ
smbprotocol/security_descriptor.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1f1b52a...fb2257c. Read the comment docs.

jborean93 commented 3 years ago

Thanks for adding the test, it's much appreciated.