-
-
Notifications
You must be signed in to change notification settings - Fork 586
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
Support Empire for system-wide deployment #757
Conversation
The reason the
Can you explain this in a bit more detail? You shouldn't need sudo to update the starkiller submodule. |
Hey. |
@vinnybod any clue about my last comment? |
The current assumption Empire makes is that it is:
I'd approve a PR that adds your admin install use case as long as it still supports the above assumptions too. |
I need to do with you a deeper analysis to properly work on the code. Problem statementEmpire cannot work at system-level, for example when on my host I have 10 user accounts (i.e., in a school), I cannot install one single Empire instance for everyone but I need to install N instances for each user by consuming a lot of resources. Your requirements
not installed as admin
run as admin for opening portsSo, opening ports on the local network, and admin is needed.
doesn't write files as admin
run_as_user() scopeCurrently
So, currently, My proposalI don't see relevant risks to install Empire at system-level (so as root) and to pull updates and submodules. If you want to be compliant with the minimum privilege principles, what we can do is to allow Empire to be installed as root at system-level (as occurs for any tools/packages in Linux), but we can identify all those files, resources, elements that must be dealt as current user instead of root, and manage them in the user HOME folder. In this context |
It's more of a convenience factor for us at maintainers. There have been a lot of issues opened in the past due to file permissions issues. So enforcing installs and the file writes to be user-level decreases that. For example:
We handle this automatically with the ps-empire command
Sort of got into this above, but ideally all the files in the application directory would have uniform access permissions. It is less about risks and more about alleviating inbound requests related to issues stemming from people installing in different ways. My intention is that for 90%+ of users, the standard install that we enforce "just works". |
I try to split the problem in small problems because involving all users could be complex but still possible. If we want to try to "unblock" the minority of users, and to not impact the current 90+% users, I would start to manage from the raising issues when Empire is installed system-wide BUT with non-root run. I will edit the PR code. 1. Issue: Permission denied on directories specified in config files
Solution: code will check This approach is compliant to Linux specs (XDG Base Directory Specification that standardizes where application files, including logs, should be stored within the user’s home directory.) Advantages:
2. Add check on fetch_submodules()Added: def fetch_submodules():
if not os.path.exists(Path(".git")):
log.info("No .git directory found. Skipping submodule fetch.")
return
command = ["git", "submodule", "update", "--init", "--recursive"]
run_as_user(command) to run this function only if 3. config.yaml must be copied in HOME user folderIn Linux/Windows, configuration files must be copied from the default one to HOME folder, in order to allow it to manage according XDG standard specification. It allows to cover both user-level and system-level approach. Furthermore, the definition of config file path must be managed centrally. 4. Manage dirs specified in config.yaml to fallback to $HOME directory - WIP
directories:
downloads: empire/server/downloads/
module_source: empire/server/data/module_source/
obfuscated_module_source: empire/server/data/obfuscated_module_source/
|
c8bf478
to
6b001e5
Compare
@D3vil0p3r thanks for iterating on this with me 😅 . I'm still learning the best practices around Linux. |
Yes I am aware but I cannot write directly it because yaml file cannot expand env variables in case of Linux or Windows systems. So my approach is "first, copy config.yaml to home dir |
1st Questionwhat is the difference between Why in These two folders are changed/written at runtime during the usage of Empire? If not, what is the reason to have directories:
module_source: empire/server/data/module_source/
obfuscated_module_source: empire/server/data/obfuscated_module_source/ in 2nd QuestionOne additional note: since you don't want users use sudo for running Empire, INVOKE_OBFS_DST_DIR_BASE = "/usr/local/share/powershell/Modules/Invoke-Obfuscation"
...
...
# invoke obfuscation
if os.path.exists(f"{INVOKE_OBFS_DST_DIR_BASE}"):
shutil.rmtree(INVOKE_OBFS_DST_DIR_BASE)
pathlib.Path(pathlib.Path(INVOKE_OBFS_SRC_DIR_BASE).parent).mkdir(
parents=True, exist_ok=True
)
shutil.copytree(
INVOKE_OBFS_SRC_DIR_BASE, INVOKE_OBFS_DST_DIR_BASE, dirs_exist_ok=True
) this copy will always fail due to missing permission. Still, here I would suggest to write in 3rd QuestionWhen Empire is run the first time (for example on a Linux system), why is it running building/compilation by MSBuild? Usually compilation must not run at runtime. Best practices suggest to do it at "building time". Is it possible you build them offline and upload the output files directly on the Empire repository so this build process code can be removed? |
aba6b0b
to
4361ca7
Compare
The
Not every module has a 1 to 1 mapping with source code in
I don't recall the exact reason that these are in the
I am fine moving
This is coming from compiling the csharp compiler https://github.com/BC-SECURITY/Empire/blob/main/empire/server/plugins/csharpserver/csharpserver.py#L98-L100 We have actually already addressed this point in the upcoming 6.0 release which is due in a few months, so you shouldn't bother changing that. The 6.0 release will download a binary in the install script instead of compiling it at runtime. |
A couple notable changes that need to be documented in some way. In general, I am in favor of these changes but we just need to make sure that these things are noted in the changelog (and wiki if necessary).
I think we are pretty close to landing this! |
@vinnybod the reason why all those things are moved to By these changes, even if Empire is installed at system level, by moving runtime writable files on In particular, StarKiller is cloned to that home folder because it periodically check for updates, so its directory needs to be writable at runtime. Note that, if I remember well, when you run Empire first time, in So just to summarize and according to what I reported above, what is missing before merging? |
At a minimum, needed for merge:
-- Just some other musings, don't feel obligated to change the implementation here. |
@vinnybod The problem is that
It is not clear to me how to solve this. Can you guide me how can I run those tests that fail? |
https://github.com/D3vil0p3r/Empire/blob/patch-2/empire/server/core/config.py#L140-L141 I think you could just change this loader to something like tmp_config_dict = EmpireConfig(config_dict).model_dump()
# Change this function to return back the updated dict after it writes out the file
config_dict = config_manager.check_config_permission(tmp_config_dict, "server")
empire_config = EmpireConfig(config_dict) |
7007733
to
564fd2e
Compare
@vinnybod I updated the wiki files and changelog in this PR. Can you give a look? I also fixed the test error you got. The reason was that the config dict was structured in an inconsistent manner. I fixed it and now that test is correct. By running now
|
It's possible you don't have the malleable profile submodule cloned down. |
Indeed. Now all test_config tests work well. |
* Added initial implementation of c# bof yamls * updated bof yamls to take in formatting types * added clipboard and secinject * added nanodump * added tgtdelegation * fixed formatting * update folders and changelog * added pytest * fixed pytest * fixed seatbelt * Update empire/server/modules/bof/secinject.py Co-authored-by: Vincent Rose <[email protected]> * trying to figure this out * Update empire/server/modules/bof/nanodump.py Co-authored-by: Vincent Rose <[email protected]> * Update empire/server/core/module_service.py Co-authored-by: Vincent Rose <[email protected]> * Update empire/server/core/module_service.py Co-authored-by: Vincent Rose <[email protected]> * fixed dashes * fixed vinnybod comments and nanodump.yaml * fixed formatting * fixed formatting * simplified redundant functions * formatting * Update empire/server/core/module_service.py Co-authored-by: Vincent Rose <[email protected]> * Update empire/server/core/module_models.py Co-authored-by: Vincent Rose <[email protected]> * Update empire/server/modules/bof/tgtdelegation.py Co-authored-by: Vincent Rose <[email protected]> * sim108 fixes * fixed nanodump and removed pytest * second round of SA modules * added 2nd round of bofs * fixed errors * formatting * updated changelog * added docs for bof-modules * Update docs/module-development/bof-modules.md Co-authored-by: Vincent Rose <[email protected]> * made fixes --------- Co-authored-by: Vincent Rose <[email protected]>
@vinnybod any news on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments left. Also please check that linting and formatting pass before pushing. It will help speed up our feedback loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unintentional. cd
into this directory and set it back to the previous commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was unintentional and no idea why that change was up. Btw @vinnybod I fixed everything you asked above and lint test is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the tests are still failing. They are supposed to load the config from the test dir.
empire/test/test_download_service.py:3: in <module>
from empire.server.core.download_service import DownloadService
empire/server/core/download_service.py:10: in <module>
from empire.server.api.v2.download.download_dto import (
empire/server/api/v2/download/download_dto.py:6: in <module>
from empire.server.api.v2.tag.tag_dto import Tag, domain_to_dto_tag
empire/server/api/v2/tag/tag_dto.py:6: in <module>
from empire.server.core.db import models
empire/server/core/db/models.py:27: in <module>
from empire.server.core.config import empire_config
empire/server/core/config.py:153: in <module>
config_dict = config_manager.check_config_permission(config_dict, "server")
empire/config_manager.py:103: in check_config_permission
with open(config_path, "w") as config_file:
E FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/.empire/server/config.yaml'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinnybod it happens because you need to run empire at least one time (also just by empire
). In this manner, it copies the default empire/server/config.yaml
file to $HOME/.empire/server/
directory. After that, run the test again.
You can also edit the test file in order to make it to first run empire
command in order to initialize the stuff and so that it copies the config.yaml
to the $HOME
directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so then you need to add a config_init
call to the conftest.py fixture?
I think it is also important that the tests will run isolated without overwriting the ~/.empire
config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinnybod there was an issue on the code, not on the test. Now I fixed. Please check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinnybod the failed test below produces the following output:
> assert Path(invoke_obfs_dir / "Invoke-Obfuscation.ps1").exists()
E AssertionError: assert False
E + where False = <bound method Path.exists of PosixPath('/tmp/pytest-of-runner/pytest-0/test_reset_server0/powershell/Modules/Invoke-Obfuscation/Invoke-Obfuscation.ps1')>()
E + where <bound method Path.exists of PosixPath('/tmp/pytest-of-runner/pytest-0/test_reset_server0/powershell/Modules/Invoke-Obfuscation/Invoke-Obfuscation.ps1')> = PosixPath('/tmp/pytest-of-runner/pytest-0/test_reset_server0/powershell/Modules/Invoke-Obfuscation/Invoke-Obfuscation.ps1').exists
E + where PosixPath('/tmp/pytest-of-runner/pytest-0/test_reset_server0/powershell/Modules/Invoke-Obfuscation/Invoke-Obfuscation.ps1') = Path((PosixPath('/tmp/pytest-of-runner/pytest-0/test_reset_server0/powershell/Modules/Invoke-Obfuscation') / 'Invoke-Obfuscation.ps1'))
empire/test/test_zz_reset.py:129: AssertionError
It probably occurs because, as you know, we removed the code of Invoke-Obfuscation.ps1
file from server.py
:
-INVOKE_OBFS_SRC_DIR_BASE = os.path.join(
- os.path.dirname(__file__), "data/Invoke-Obfuscation"
-)
-INVOKE_OBFS_DST_DIR_BASE = "/usr/local/share/powershell/Modules/Invoke-Obfuscation"
and
- if os.path.exists(f"{INVOKE_OBFS_DST_DIR_BASE}"):
- shutil.rmtree(INVOKE_OBFS_DST_DIR_BASE)
- pathlib.Path(pathlib.Path(INVOKE_OBFS_SRC_DIR_BASE).parent).mkdir(
- parents=True, exist_ok=True
- )
- shutil.copytree(
- INVOKE_OBFS_SRC_DIR_BASE, INVOKE_OBFS_DST_DIR_BASE, dirs_exist_ok=True
- )
How can we fix this on test side? Does the test py file need to be edited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just remove the invoke obfuscation parts of that test, they no longer seem relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinnybod I applied the change on the test. Can you check please?
ec1cc16
to
59439c3
Compare
Thanks, all the tests are passing at this point. I will re-review and try to get it merged in the next few days. |
Thank you @vinnybod I hope it will be done soon so we can package Empire in the proper manner for Arch environment. |
Describe your changes
Refactored code to make Empire to be deployed at system-level (multi-user) with no impact on the current approach based on working only in a case Empire is cloned in a user-granted directory.
This PR makes Empire working also in case it is installed in a system directory without changing the current grants. It is possible by refactoring code by following XDG Base Dir specifications.
Issue ticket number and link (if there is one)
#756
Checklist before requesting a review
CHANGELOG.md
docs/
(if applicable)