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

Add an ability to install OFRAK from source #314

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

Conversation

ANogin
Copy link
Contributor

@ANogin ANogin commented May 23, 2023

One sentence summary of this PR (This should go in the CHANGELOG!)
Add an ability to install OFRAK from source

Link to Related Issue(s)
N/A

Please describe the changes in your request.
This is a source tree equivalent of pip install for OFRAK - works similar to build_image.py approach, but does not require docker.

  • Run make install_core/make install_tutorial/make install_develop to pip-install OFRAK from the current source tree. The core and tutorial versions do regular install (make install for packages listed in the corresponding .yml), while the install_develop does an "editable mode" install (make develop for packages).
  • Pass OFRAK_INSTALL_DEPS=brew/OFRAK_INSTALL_DEPS=apt to make to have the dependencies (including the npm/rollout prerequisites) automatically installed.
  • Pass OFRAK_INSTALL_PYTHON=python3.x (with or without the full path) to make to have OFRAK installed for the particular instance of python on your system.

(This also adds handling of binja as a potentially-missing dependency, as make install_develop easily triggers a situation where the ofrak binja modules are there, but binja is not - let me know if you'd rather have that as a separate PR).

Anyone you think should look at this, specifically?
@Edward-Larson probably? Maybe @rbs-jacob?

Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

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

I've made several suggestions for minor changes.

I wonder if we should also incorporate this such that it gets run inside of the Docker images during build? That would improve the consistency between Docker and non-Docker OFRAK setups. I could see it also making the code more maintainable. @whyitfor and @EdwardLarson will probably want to weigh in on that.

Makefile Outdated Show resolved Hide resolved
@@ -15,6 +21,21 @@
LOGGER = logging.getLogger(__file__)


class _BinjaExternalTool(ComponentExternalTool):
Copy link
Member

Choose a reason for hiding this comment

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

Haven't yet had a chance to test, but I wonder if making Binary Ninja an "external tool" creates the possibility of a confusing situation for a user: they explicitly set the Binary Ninja back end by either discovering it or setting a command-line argument, they set exclude_components_missing_dependencies=True (also settable by a command-line argument), and there is an issue with their Binary Ninja installation. In this case, when they go to analyze their file, it won't unpack code regions with Binary Ninja, but also won't give the user any feedback as to why.

This may not be an issue, whether because this situation is sufficiently unlikely, because we don't deem this to be a problem, or because this issue doesn't actually manifest this way, but it's worth considering this situation.

Copy link
Member

Choose a reason for hiding this comment

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

One other concern I have about making Binary Ninja an external tool like this is that it's not consistent with the other analysis back ends. We only show information about when Binary Ninja is missing, but don't show any of the respective information about which components won't be loaded when we don't have Ghidra installed, , for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is here because the binja backend is the one that actually prevents OFRAK from working at all if binja is not installed. Other backends do not have the issue.

install.py Outdated Show resolved Hide resolved
install.py Outdated Show resolved Hide resolved
ofrak_patch_maker/Makefile Outdated Show resolved Hide resolved
install.py Outdated Show resolved Hide resolved
install.py Show resolved Hide resolved
install.py Show resolved Hide resolved
install.py Outdated Show resolved Hide resolved

if not config.quiet:
print(f"*** Checking whether npm and rollup are installed")
check_executable(config, "npm")
Copy link
Member

@rbs-jacob rbs-jacob May 23, 2023

Choose a reason for hiding this comment

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

I don't think npm is included in the results of ofrak deps, so the error message of this function is incorrect when npm is not found.

Copy link
Contributor Author

@ANogin ANogin May 23, 2023

Choose a reason for hiding this comment

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

@jacob, the npm is checked before ofrak is installed. The error message (when dependency_mechanism is neither apt nor brew) is:

                print(f"** {executable} not found, please install manually,")
                print('** or use "--install_deps" / "OFRAK_INSTALL_DEPS"')
                print("** to have it be installed automatically for you:")
                print(f"**    apt:  sudo apt install -y {executable}")
                print(f"**    brew: brew install {executable}")

Do not see anything wrong with it - when --install_deps is apt or brew, then npm will be installed - --install_deps is not only for ofrak deps dependencies, but also for build dependencies as well.

@whyitfor
Copy link
Contributor

@ANogin, if I understand the intent behind this PR, you are looking for a way to install OFRAK from source natively.

Did you consider building a Makefile target to do this? What I'm envisioning is something like:

OFRAK_CONFIG=ofrak-dev.yml PYTHON_PATH=python3 make develop

The Makefile target could then:

  1. Parse the yaml to get list of packages to install
  2. Call make develop/install in the respective directory
  3. There could be an optional argument at the end to run apt/brew commands

I also wonder if we need an automated script to install from source -- this is going to increase the amount of things taht need to be tested and maintained.

ANogin and others added 3 commits May 23, 2023 14:40
Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>
@ANogin
Copy link
Contributor Author

ANogin commented May 23, 2023

Did you consider building a Makefile target to do this? What I'm envisioning is something like:

OFRAK_CONFIG=ofrak-dev.yml PYTHON_PATH=python3 make develop

@whyitfor , I can add OFRAK_CONFIG, but then would probably want to do the same for the build_image targets? Right now we have a few hardcoded targets with specific configs, rather than OFRAK_CONFIG, so I kept the same for consistency.

  1. Parse the yaml to get list of packages to install
  2. Call make develop/install in the respective directory

At least the above steps would presumably still require a python script?

  1. There could be an optional argument at the end to run apt/brew commands

Is there any benefit to handling the apt vs brew vs just print the suggestion logic in Makefile rather than in Python?

I also wonder if we need an automated script to install from source -- this is going to increase the amount of things taht need to be tested and maintained.

@ANogin ANogin requested a review from rbs-jacob July 27, 2023 03:23
@ANogin ANogin mentioned this pull request Jul 27, 2023
@ANogin ANogin marked this pull request as draft February 11, 2024 19:32
@ANogin ANogin marked this pull request as ready for review February 12, 2024 04:34
@ANogin ANogin requested a review from whyitfor February 12, 2024 04:34
ANogin and others added 21 commits May 30, 2024 14:01
* Use python3.8 in docker images

* Require pytest<8.0

This is needed becase of pytest-dev/pytest#11890
TvoroG/pytest-lazy-fixture#65

* Update changelog

* Revert "Update changelog"

This reverts commit 500ee9b.

Making changes before having coffee :(

* Add a note on recommending Python 3.8

* `ofrak_core` also needs `pytest<8.0`
…loonsecurity#414)

* Dropping the .altinstr_replacement section from the toolchain

* Updated CHANGELOG
* Set the fallback font to monospace

* Update CHANGELOG
…#423)

* Display strings with numbers primarily as strings

* Update CHANGELOG
* Add typing to ofrak_ghidra package

* Add changelog

---------

Co-authored-by: Paul Noalhyt <paul@redballoonsecurity.com>
* Increase time limit on `test_comment_content`

* Fix a spurious "no current event loop" test error
* Use python3.8 in docker images

* Require pytest<8.0

This is needed becase of pytest-dev/pytest#11890
TvoroG/pytest-lazy-fixture#65

* Update changelog

* Revert "Update changelog"

This reverts commit 500ee9b.

Making changes before having coffee :(

* Update to latest angr==9.2.89, which also necessitates Python >= 3.8 and capstone==5.0.0.post1

* Apply Edward's attempted fix to angr test failure

* Add a note on recommending Python 3.8

* Add a note on recommending Python 3.8

* Document the requirement of Python 3.8+

* Switch to angr 9.2.77

* `ofrak_core` also needs `pytest<8.0`

* ignore DataWord in test due to angr bug

* add another now missing block

* black linting

* Attempt to fix a capstone error

* Dropping the .altinstr_replacement section from the toolchain (redballoonsecurity#414)

* Dropping the .altinstr_replacement section from the toolchain

* Updated CHANGELOG

* Set the fallback font to monospace (redballoonsecurity#422)

* Set the fallback font to monospace

* Update CHANGELOG

* Display strings with numbers primarily as strings (redballoonsecurity#423)

* Display strings with numbers primarily as strings

* Update CHANGELOG

* Add typing support to ofrak_ghidra package (redballoonsecurity#421)

* Add typing to ofrak_ghidra package

* Add changelog

---------

Co-authored-by: Paul Noalhyt <paul@redballoonsecurity.com>

* Increase time limit on `test_comment_content`

* Fix a spurious "no current event loop" test error

* Explain 3.7 vs 3.8 better in the docs

* Cite specific versions of angr in comment

* Update docs/environment-setup.md

* Update docs/getting-started.md

---------

Co-authored-by: Edward Larson <edward@redballoonsecurity.com>
Co-authored-by: rbs-alexr <122491504+rbs-alexr@users.noreply.github.com>
Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>
Co-authored-by: Paul Noalhyt <paul.noalhyt@gmail.com>
Co-authored-by: Paul Noalhyt <paul@redballoonsecurity.com>
Co-authored-by: Wyatt <53830972+whyitfor@users.noreply.github.com>
…ds (redballoonsecurity#425)

- $PACKAGE_PATH generalizes build_image.py such that it works with any
yaml file containing a valid list of Python packages

Before this change, it was possible to use build_image.py a directory above
the OFRAK repository by passing in an OFRAK_DIR arugment in the yml file.
This change generalizes the approach so that build_image.py can be run in any ancestor directory.
---------

Co-authored-by: Dan Pesce <dan@redballoonsecurity.com>
Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>
* Remove SUBALIGN(0) from bss linker script

* Update changelog
---------

Co-authored-by: Dan Pesce <dan@redballoonsecurity.com>
* Add identify recursively to the GUI

* Add CHANGELOG entry

* Fix start page typo

* Add test

---------

Co-authored-by: Dan Pesce <dan@redballoonsecurity.com>
Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>
* add lief add/remove section modifier

* Changelog

* typo

---------

Co-authored-by: Dan Pesce <dan@redballoonsecurity.com>

---------

Co-authored-by: Dan Pesce <dan@redballoonsecurity.com>
Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>
redballoonsecurity#445)

* Fix Carve/Modify bug, handle an error in the server, vscode doesnt like the way we handle dragging

* lint

---------

Co-authored-by: Dan Pesce <dan@redballoonsecurity.com>
* Support `make -k test` in docker root

* Lint
* reduce chunking min limit from 64mb to 1mb

* Update frontend/src/hex/HexView.svelte

What is happening

---------

Co-authored-by: Dan Pesce <dan@redballoonsecurity.com>
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.

8 participants