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

Update mssqlclient.py #1381

Merged
merged 5 commits into from
Dec 5, 2024
Merged

Update mssqlclient.py #1381

merged 5 commits into from
Dec 5, 2024

Conversation

kiriknik
Copy link
Contributor

Add functionality to upload files

Add functionality to upload files
@gabrielg5 gabrielg5 requested a review from 0xdeaddood February 9, 2023 15:23
@0xdeaddood 0xdeaddood added the on hold Awaiting an action or decision to move forward label Mar 16, 2023
@anadrianmanrique anadrianmanrique added the in review This issue or pull request is being analyzed label Sep 26, 2024
@gabrielg5
Copy link
Collaborator

Hey @kiriknik, sorry coming so late to this one

Thanks for this PR!

This branch should be rebased with latest changes as we've modified this example to make it callable from ntlmrelayx as an interactive shell; so some classes have been moved to other files (now it is in impacket/examples/mssqlshell.py)

Despite that, I've been checking it and have a few comments:

  • Some error handling should be added to the command. I run it here and faced these, for example: file not found, remote file exists, permission denied... ie
  • It should be checked if xp_cmdshell is enabled. If not, we will need to fail and ask the user to do so (I wouldn't enable it automatically, just in case...)
  • tqdm should be added in the requirements and setup.py files - or removed and show progress in a different way -
  • Instead than windows and linux in this log line "[+] MD5 hashes match\n[*] In windows: {} in linux: {}".format(md5sum,md5sum_uploaded), I would say something like local / remote or even something more verbose Impacket Host / SQL Server host
  • A super tiny detail, alignment of commands description in the help output 🤓; and also the description of the new upload one can be changed to something that explains what the command does besides specifying what's required in those parameters
    image

Thank you!

@gabrielg5 gabrielg5 added waiting for response Further information is needed from people who opened the issue or pull request and removed on hold Awaiting an action or decision to move forward labels Oct 23, 2024
@gabrielg5
Copy link
Collaborator

Implemented requested changes.

  • Validating if xp_cmdshell is enabled - returning if not
  • Parsing command parameters with shlex
  • Uploading file and validating its there with xp_fileexist

Removed addition of tqdm dependency. It is a nice UX but rather more related to the examples than to the library itself.
Linking with #1810 to take into account when working on splitting examples from library

@anadrianmanrique anadrianmanrique added medium Medium priority item and removed waiting for response Further information is needed from people who opened the issue or pull request in review This issue or pull request is being analyzed labels Dec 5, 2024
else:
print("[-] ERROR! MD5 hashes do NOT match!")
print("[+] Uploaded file MD5: %s" % md5sum_uploaded)
except:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inform uknown exception conditions
avoid except: clauses

@gabrielg5
Copy link
Collaborator

gabrielg5 commented Dec 5, 2024

Showing unhandled exceptions in do_upload command (eventually something to do in all shells' commands...)

┌──(xxx㉿xxx)-[~/Desktop/impacket/examples]
└─$ python mssqlclient.py xxx/xxx@xxx -debug -windows-auth
Impacket v0.13.0.dev0+20241128.142905.20351f27 - Copyright Fortra, LLC and its affiliated companies

[+] Impacket Library Installation Path: /home/kali/.local/lib/python3.11/site-packages/impacket
Password:
[*] Encryption required, switching to TLS
[*] ENVCHANGE(DATABASE): Old Value: master, New Value: master
[*] ENVCHANGE(LANGUAGE): Old Value: , New Value: us_english
[*] ENVCHANGE(PACKETSIZE): Old Value: 4096, New Value: 16192
[*] INFO(xxxx): Line 1: Changed database context to 'master'.
[*] INFO(xxxx): Line 1: Changed language setting to us_english.
[*] ACK: Result: 1 - Microsoft SQL Server (110 2789)
[!] Press help for extra shell commands
SQL (xxx\xxx dbo@master)> upload a b
[-] Unhandled Exception: [Errno 2] No such file or directory: 'a'
SQL (xxx\xxx dbo@master)> exit

@anadrianmanrique anadrianmanrique removed the request for review from 0xdeaddood December 5, 2024 21:24
@anadrianmanrique anadrianmanrique merged commit 9ef36ac into fortra:master Dec 5, 2024
8 checks passed
@anadrianmanrique
Copy link
Contributor

Thanks for the PR!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium Medium priority item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants