Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect ace_count while unpacking ACEs #143

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

MWedl
Copy link
Contributor

@MWedl MWedl commented Oct 7, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

MWedl commented Oct 11, 2021

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

@codecov
Copy link

codecov bot commented Oct 11, 2021

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
Copy link
Owner

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

@jborean93 jborean93 merged commit d2dab92 into jborean93:master Oct 14, 2021
adiroiban added a commit to chevah/smbprotocol that referenced this pull request Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants