Skip to content

[SMB] Massive Fixes, Features and Refactoring - #1894

Merged
anadrianmanrique merged 42 commits into
fortra:masterfrom
covertivy:master
Aug 7, 2025
Merged

[SMB] Massive Fixes, Features and Refactoring#1894
anadrianmanrique merged 42 commits into
fortra:masterfrom
covertivy:master

Conversation

@covertivy

@covertivy covertivy commented Feb 8, 2025

Copy link
Copy Markdown
Contributor

Hello!

I did some research regarding some annoying STATUS_SHARING_VIOLATION and STATUS_ACCESS_DENIED errors.
I know for a fact that some files that cannot be read with smbclient can be copied aside with a command / the file explorer.
This means that theoretically it should be possible to do so over SMB!

I opened up Wireshark and played around a bit - it seems this is caused by over-restrictive share access permissions on impacket's side.
I then dug deeper and saw some mismatching flag usage in the SMBv1 implementation of the protocol so I fixed those too.

To sum up, I added the ability to READ FILES WITH OPEN HANDLES WITH (ALMOST) NO RESTRICTION!!!
The only restriction is of course for some system files (eg. SAM, SECURITY, SYSTEM and basically all files that require a ShadowCopy to allow reading them).
This means that files with "weak handles" can be read remotely WITH ABSOLUTELY NO LIMITATION!

The list contains:

  • Event Logs - *.evt / *.evtx
  • Files open by Local Users
  • Browser Files - Chrome Logins & History
  • And much much more!

Glad to suffer for all y'alls pleasure!

…, write and delete.

I found it out when capturing network traffic and seeing I can manage to read files with open handles.
This fixes SHARE_ACCESS_DENIED errors when trying to read files with handles on them.
This is the logical thing to do when we open files for reading.
We do not want to block other processes from interacting with the file.
This may raise problems when reading files that are being written into / being deleted.
I still think this is the right move since we usually want to read the files no matter what.
This is related to my older commits on the subject.
@covertivy covertivy changed the title Add Ability to Read Files with Handles Add Ability to Read Files with Open Handles over SMB Feb 8, 2025
@covertivy covertivy changed the title Add Ability to Read Files with Open Handles over SMB Fix: Add Ability to Read Files with Open Handles over SMB Feb 10, 2025
@yuri2o1o

Copy link
Copy Markdown

Really important fix that makes SMB more intuitive! Please merge!

@laxa

laxa commented Feb 26, 2025

Copy link
Copy Markdown
Contributor

Hello, interesting PR, however, it does not seem to be working on my side, am I missing something?

$ smbclient.py 'User:Password'@192.168.122.111
# use C$
# cd users/user/appdata/local/microsoft/edge/user data/default/network
# get cookies
[-] SMB SessionError: code: 0xc0000043 - STATUS_SHARING_VIOLATION - A file cannot be opened because the share access flags are incompatible.

@covertivy

Copy link
Copy Markdown
Contributor Author

The problem is with your test case.
On chromium based browsers the Cookies file is locked with a "strong handle" which restricts read access to other processes, therefore in this case the file cannot be read by us.
However, the other sensitive chrome files such as Local State, Login Data, History and others can be read with no issue!

I am happy to help with any other questions you may have 😊

covertivy and others added 12 commits March 1, 2025 04:37
This is done to assist future development of SetInfo operations on files and directories.

```
Date Conversion Example - Year Component:
-----------------------------------------

    2009 - 1980 = 29

            | (convert to binary)
            V

    0001 1101

            | (position data correctly by shifting)
            V

                    0001 1101
    <<                     9
        ---------------------
    =  0 0011 1010 0000 0000

            | (trim to correct size with bitwise AND of correct mask)
            V

        0 0011 1010 0000 0000
    &   1111 1110 0000 0000
        ---------------------
    =   0011 1010 0000 0000
```
By doing so I also fixed a bad structure definition in the SMBSetFileBasicInfo structure.
Now we can modify file information remotely ;)
Also converted the SMB DATETIME methods to use my SMB_DATE and SMB_TIME implementations.
Also implemented setInfo method to use for setting file information.
fileInformationClasses other than the default one.
…s using SMB.

for now only implemented query.
@covertivy

Copy link
Copy Markdown
Contributor Author

@anadrianmanrique Done :)

@anadrianmanrique

Copy link
Copy Markdown
Collaborator

@covertivy I'm currently reviewing this PR. Testing has been great so far. Really nice features has been implemented. I'll post some minor changes for the code review. I'm the meanwhile I would like to request, if possible, to revert changes related to annotations and cosmetic fixes as well ( that doesn't include comments/documentation etc ), as we want to keep the code as consistent as possible. Thanks!

@covertivy

Copy link
Copy Markdown
Contributor Author

Pity, I really hoped for better type hinting, but oh well 😅
@anadrianmanrique do you believe the addition of annotations be accepted if all of the codebase was annotated as well?
I honestly believe it would drastically improve impacket as a whole...
Anyways, I will get to work removing all annotations & type hints from the project.

@anadrianmanrique

Copy link
Copy Markdown
Collaborator

@covertivy yes I understand what you mean, and I agree. But at the moment we want to minimize the amount of changes in the code base as we are approaching some stabilization phase prior to the 0.13 release. We want to have this kind of changes controlled in a very well defined scoped task, and also done in a different phase of the release process. Hope you can understand, and apologies for any inconvenience.

@covertivy

Copy link
Copy Markdown
Contributor Author

No worries all good, thanks for the amazing work!

@covertivy

Copy link
Copy Markdown
Contributor Author

I believe I corrected all of the undesired annotations & design changes.
Let me know if any more work is required @anadrianmanrique.

Comment thread impacket/examples/smbclient.py Outdated
Comment thread impacket/smb.py Outdated
Comment thread impacket/smb.py Outdated
Comment thread impacket/smb.py
Comment thread impacket/smb.py
Comment thread impacket/smb.py
Comment thread impacket/smb3.py
Comment thread impacket/smb3.py
@anadrianmanrique

Copy link
Copy Markdown
Collaborator

added some request changes. Additionally, changes in test_smb.py are run in the context of this PR, so they can be uncommented

Comment thread examples/attrib.py
Comment thread examples/filetime.py
@nullsection

Copy link
Copy Markdown

So it appears that the cookies file still appears locked. History file works in this branch.

# pwd
/Users/user/AppData/local/Google/Chrome/User Data/Default/Network
# get Cookies
[-] SMB SessionError: code: 0xc0000043 - STATUS_SHARING_VIOLATION - A file cannot be opened because the share access flags are incompatible.

@covertivy

Copy link
Copy Markdown
Contributor Author

I explained all this in this comment.
@laxa asked the exact same question, read my reply and feel free to ask questions 😉

@nullsection

Copy link
Copy Markdown

@laxa asked the exact same question, read my reply and feel free to ask questions 😉

My bad! Probably should have seen that!

Thanks

@anadrianmanrique

Copy link
Copy Markdown
Collaborator

merging now, thanks for the PR

@anadrianmanrique
anadrianmanrique merged commit 20002f7 into fortra:master Aug 7, 2025
@covertivy

Copy link
Copy Markdown
Contributor Author

Thanks!
It's been a pleasure!

@1058274

1058274 commented Aug 10, 2025

Copy link
Copy Markdown
Contributor

Merging this PR seems to have broken ntlmrelayx indirectly:

(.venv) bob@debian:/tmp/tmp.AF3wd7q2VI/impacket$ python3 ./examples/ntlmrelayx.py -t 0

Traceback (most recent call last):
  File "/tmp/tmp.AF3wd7q2VI/impacket/./examples/ntlmrelayx.py", line 56, in <module>
    from impacket.examples.ntlmrelayx.servers import SMBRelayServer, HTTPRelayServer, WCFRelayServer, RAWRelayServer, RPCRelayServer
  File "/tmp/tmp.AF3wd7q2VI/impacket/.venv/lib/python3.11/site-packages/impacket/examples/ntlmrelayx/servers/__init__.py", line 12, in <module>
    from impacket.examples.ntlmrelayx.servers.smbrelayserver import SMBRelayServer
  File "/tmp/tmp.AF3wd7q2VI/impacket/.venv/lib/python3.11/site-packages/impacket/examples/ntlmrelayx/servers/smbrelayserver.py", line 46, in <module>
    from impacket.smbserver import getFileTime, decodeSMBString, encodeSMBString
ImportError: cannot import name 'getFileTime' from 'impacket.smbserver' (/tmp/tmp.AF3wd7q2VI/impacket/.venv/lib/python3.11/site-packages/impacket/smbserver.py)

@anadrianmanrique

Copy link
Copy Markdown
Collaborator

@1058274 thanks for reporting. fixed in #2019

@covertivy

Copy link
Copy Markdown
Contributor Author

Apparently also broke some of impacket's exec examples.
I fixed it in #2038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

medium Medium priority item waiting for response Further information is needed from people who opened the issue or pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants