-
-
Notifications
You must be signed in to change notification settings - Fork 237
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
Add module S28_python_run #1277
Conversation
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.
Thank you for contributing a pull request!
Welcome to the EMBA firmware analysis community!
We are glad you are here and appreciate your contribution. Please keep in mind our contributing guidelines here and here.
Also, please check existing open issues and consider to open a discussion in the dedicated discussion area.
Additionally, we have collected a lot of details around EMBA, the installation and the usage of EMBA in our Wiki.
If you like EMBA you have the chance to support us by becoming a Sponsor or buying some beer here.
To show your love for EMBA with nice shirts or other merch you can check our Spreadshop.
This is an automatic message. Allow for time for the EMBA community to be able to read the pull request and comment on it.
I am currently on vacation and will come back to this PR soon. |
Understood. |
If someone can start with some initial rewiew and testing it would be highly appreciate. |
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.
For testing: where/how do I install pip packages?( does the venv work with the scripts)
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.
I did a quick code review of the bash code. Please check my notes.
If we are going to include python code into EMBA we will use the same actions as for EMBArk (pylint/bandit/pycodestyle). This means, we need to setup further github actions and include them also in the checker script.
I am not completely sure how we should handle the logging and we need to link to the results from the python modules.
Finally, we need a way to setup a virtual environment dynamically for the container to run the python scripts. Probably we can create a virtual environment in the external directory and mount it via docker-compose into the container.
As there are currently some questions, I would recomment to draft this PR for now to work on these topics.
Good work and I think this will results in cool stuff for EMBA. Keep on going :)
modules/S28_python_run.sh
Outdated
local lRESULTS=() | ||
local lCOUNT_FINDINGS=0 | ||
|
||
lPYTHON_BIN="$( find . -name python3 | head -n 1 )" |
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.
We need to check this in the dependency checker. I also think this find is not needed as the installer should have already installed python3 in path. So, at the end we can do something like this here
emba/helpers/helpers_emba_dependency_check.sh
Line 709 in d829e18
check_dep_tool "ssdeep" |
modules/S28_python_run.sh
Outdated
if [[ ${lPYTHON_SCRIPT_COUNT} -gt 0 ]]; then | ||
print_output "[*] ${lPYTHON_SCRIPT_COUNT} Python script/s queued for execution." | ||
|
||
for lSCRIPT in "${PYTHON_SCRIPTS[@]}"; do |
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.
I think I was missing somthing like the following:
local lSCRIPT=""
Why ist the PYTHON_SCRIPTS
array not local?
Please rename it to PYTHON_SCRIPTS_ARR
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.
Considering the PYTHON_SCRIPTS array works essentially the same as the SELECT_MODULES array in an EMBA profile, should I still rename it?
modules/S28_python_run.sh
Outdated
sub_module_title "${lSCRIPT}" | ||
print_output "[*] Executing: ${lPYTHON_BIN} ${lSCRIPT_DIR}/${lSCRIPT}.py" | ||
mapfile -t lRESULTS < <(${lPYTHON_BIN} "${lSCRIPT_DIR}/${lSCRIPT}.py") | ||
lCOUNT_FINDINGS=$(("${lCOUNT_FINDINGS}" + "${#lRESULTS[@]}")) |
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.
we can do at least something like the following (unsure on the lRESULTS counting):
lCOUNT_FINDINGS=$((lCOUNT_FINDINGS + "${#lRESULTS[@]}"))
lRESULTS is an array, please rename it the following lRESULTS_ARR
I will begin work on the requested changes. I will also update the copyright notices accordingly, I did not change them due to a misunderstanding on my end. Please feel free to contact me for any additional questions or requirements. To clarify some things:
As it is currently, the module uses EMBAs venv. PIP packages should work when installed into this environment.
The
Right now, the Bash module reads the results from STDOUT of the Python script (each line = 1 results).
Instead of using EMBAs venv, this might be possible. I will look into it. |
No hurry with this. I think this could be a very interesting addon for EMBA. Nevertheless, we need to design it in a way it will also fulfill the expectations into a python integration for EMBA. |
As the next big change I want to change the logging method around. As mentioned before, I plan on making the output from the python module grep-able, so it is easier I could either implement this by reading STDOUT from each Python module directly like I am doing at the moment, or Is there a preferred method to go about this? |
Probably it makes sense to log from the python modules as from the main EMBA modules. All relevant output into a text file that we can then link from the main EMBA s28 module. With this we should be able to easily generate the html report page. I think this could be around line 37 of your module. There we will need a write_link after your print_output. This links to the original log file from your python module. The grep-able thing is additionally a useful mechanism.
See my notes before. I would recommend to log all relevant details in a nice way to a dedicated log file and link to this log file via write_link from the main s28 module. |
That makes sense, I did not know about the write_link function. |
The web reporter automatically picks it up and is doing the html stuff for you ... hopefully ;) You should see a link with the emba/helpers/helpers_emba_print.sh Line 401 in cee27c5
|
817a37f
to
cba0296
Compare
I majorly screwed up my git history just now, which caused this pull request to close. :,) |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature.
What is the current behavior? (You can also link to an open issue here)
EMBA cannot run Python scripts natively as modules.
What is the new behavior (if this is a feature change)? If possible add a screenshot.
This PR adds a new module ("S28_python_run") with the capability of running user-supplied python scripts as
modules. Related to issue Python runner module #1264.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No. The module S28 is disabled in the default scan profile (
default-scan.emba
).In addition, when no Python scripts are manually specified, it will start and terminate without doing anything.
Other information:
-S
).check_project.sh
does not detect issues with the new code.shell_check
does not find errors with the new code.shell_check
exception has been added inS28_python_run.sh
to conform with EMBA coding standards. The exception allows the use of quotation marks in arithmetic operations. (See line 39 inS28_python_run.sh
)pyright
.