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

mssqlshell: switch back to current db after enum_impersonate #1609

Merged
merged 1 commit into from
Sep 13, 2023
Merged

mssqlshell: switch back to current db after enum_impersonate #1609

merged 1 commit into from
Sep 13, 2023

Conversation

exploide
Copy link
Contributor

@exploide exploide commented Sep 7, 2023

When running enum_impersonate in the SQL shell, it will cycle through all available databases and remains in the last one. This is unexpected because it silently changes the current database. The following screenshot illustrates the issue.

enum_impersonate-users

This change ensures the current database after running enum_impersonate is the same as before.

@gabrielg5 gabrielg5 self-assigned this Sep 12, 2023
@gabrielg5
Copy link
Collaborator

Hi @exploide , nice catch! thanks for the PR
I just made a couple suggestions in case an exception is triggered in the enum_impersonate.

Let me know what you think, and we can move forward with the merge

@gabrielg5 gabrielg5 added the in review This issue or pull request is being analyzed label Sep 12, 2023
@exploide
Copy link
Contributor Author

Hi @gabrielg5 thank you very much for your review!

That the function might raise an exception and the db change is not performed is a valid concern. Unfortunately the change you committed did not solve the problem because when it's done in the except clause, the db change will only be performed when an exception occurred, but not when everything went well. So it needs to go into a finally clause instead. I changed that now.

(I did not wrap the db change in another try/catch again because I assume it is simple enough and some other functions like do_enum_users don't do it as well. But yeah, it is a bit inconsistent. On the other hand, catching a bare exception might be problematic anyway, because it could silently hide errors. But this is a code style question and might be out of scope of this PR. I can change it though if you like.)

Feel free to move forward with the merge or request changes. :)

@gabrielg5
Copy link
Collaborator

hey, yes! my intention was to put it in the finally clause, but ended up putting it in the except :/
testing now, if all good will merge it
thanks!!

@gabrielg5 gabrielg5 merged commit 0017927 into fortra:master Sep 13, 2023
9 checks passed
@exploide exploide deleted the mssqlshell-enum_impersonate-use_db branch September 14, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review This issue or pull request is being analyzed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants