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

Removed use of System.Environment.Exit #129

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anthemtotheego
Copy link

This prevents Rubeus from killing your process when running certain functions inline (or in a non sacrificial process). To reproduce, run current Rubeus through inline-ExecuteAssembly in monitor mode for X seconds. The process is killed and you lose your beacon. If you remove System.Environment.Exit and run the same command, it functions properly and you do not lose your beacon.

Removed System.Environment.Exit(0); and replaced with return;  This allows you to run monitor or harvest mode inline (non sacrificial process) without it killing your process.
Replaced System.Environment.Exit(1); with return null;  This will prevent the process from being killed if ran inline (in a non sacrificial process).
Copy link
Member

@HarmJ0y HarmJ0y left a comment

Choose a reason for hiding this comment

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

This shouldn't be an issue ya @0xe7 ?

@0xe7
Copy link
Contributor

0xe7 commented Apr 29, 2022

I'm not 100% because my experience with CS is limited, @CCob may be better to comment on this

@CCob
Copy link
Contributor

CCob commented May 19, 2022

The one for HarvestTicketGrantingTickets it fine, but StringToByteArray is used everywhere it looks like which will more likely lead to a confusing NullReferenceException. @anthemtotheego, can we change the PR so that it throws InvalidParameterException for the StringToByteArray function?

@anthemtotheego
Copy link
Author

Yeah, no issue at all with you changing it to avoid any possible issues.

@n3rada
Copy link

n3rada commented May 15, 2024

Is it still an issue?

@MexHigh
Copy link

MexHigh commented May 17, 2024

Is it still an issue?

Yes. I'm also having this issue when executing Rubeus in memory. Environment.Exit will also kill the executable Rubeus is running in.

@CCob
Copy link
Contributor

CCob commented May 18, 2024

If someone is willing to modify the PR to throw an ArgumentException for the StringToByteArray function instead of returning null I will happily merge.

@n3rada
Copy link

n3rada commented May 19, 2024

@CCob, has @anthemtotheego given you and other maintainers the right to edit the PR?

If this is not the case, it seems that PR #163 has applied the necessary changes.

@CCob
Copy link
Contributor

CCob commented May 20, 2024

Permissions is not the issue, was hoping for the original PR to be updated to save some time that's all.

@n3rada
Copy link

n3rada commented May 31, 2024

Since he's not responding, I think you should take a look at the other PR ( #163 ) that's doing what you asked, don't you?

@CCob
Copy link
Contributor

CCob commented Jun 1, 2024

Well it certainly won't get looked at when some entitled keyboard warrior demands changes via some passive aggressive commentary.

Myself and others involved in this project help maintain it in their own time as and when possible.

This PR is far smaller than the other one, which means it's far easier to be confident nothing will break.

If you are so keen for a feature that benefits you, why not take this PR, modify the requested line and submit a new one. Then I can review from my phone and merge the commit instead of needing to fire up my PC, review 50 lines of code, check nothing has broken and merge the other PR just for you.

@n3rada
Copy link

n3rada commented Jun 1, 2024

I couldn't agree with you more! Please accept my apologies if you found me aggressive when that was not at all the intention.

It's more about moving forward and wanting the project to have less open PR. The current version of Rubeus suits me perfectly. I wanted to help the author of #163, who has already done what you're asking here! I don't want to steal another PR or create any fight, that's not my war!

@CCob
Copy link
Contributor

CCob commented Jun 1, 2024

Appreciate the response.

I don't think it's about stealing PRs. At the end of the day we all have busy lives, so if a PR has stalled because people are off doing other things I don't think anyone would mind. If the modification is made from a fork of the original PR then all commiters should be included in the commit log anyway. You can also link to this PR if necessary too.

I'll see if I have time next week to fully review the other PR. From a quick look on my phone I can see error paths will output more human friendly errors than a stack trace so it certainly has its merits.

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.

6 participants