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.

Sign up for GitHub

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

Jump to bottom

Experimental Landlock based sandboxing #597

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Experimental Landlock based sandboxing #597

wants to merge 3 commits into from

Conversation

vlaci
Copy link
Contributor

@vlaci vlaci commented Jun 6, 2023

Implementation of #594 together with onekey-sec/unblob-native#11

Having to first create the extraction directory complicates things a lot.
I am unsure what approach we should take here but it can be seen, that the first directory needs somewhat special treatment.

Alternative integration approaches:

  • set LANDLOCK_ACCESS_FS_MAKE_DIR on the parent of the extraction root as an escape hatch
  • introduce a workdir where extraction takes place one level deeper to achieve simpler code.

@qkaiser
Copy link
Contributor

qkaiser commented Jun 6, 2023

I would be in favor of option 1 since it's how unblob works. The extraction path provided with -e is where we create the extraction directory so we do need LANDLOCK_ACCESS_FS_MAKE_DIR.

As long as we're clear about the fact that unblob limits itself to the path provided with -e and not the directory within, it's acceptable to me.

qkaiser
qkaiser reviewed Jun 6, 2023
View reviewed changes
unblob/models.py Outdated Show resolved Hide resolved
unblob/processing.py Outdated Show resolved Hide resolved
unblob/processing.py Outdated Show resolved Hide resolved
@vlaci
Copy link
Contributor Author

vlaci commented Jun 6, 2023

I would be in favor of option 1 since it's how unblob works. The extraction path provided with -e is where we create the extraction directory so we do need LANDLOCK_ACCESS_FS_MAKE_DIR.

As long as we're clear about the fact that unblob limits itself to the path provided with -e and not the directory within, it's acceptable to me.

We need this for the parent of extraction directory:

LANDLOCK_ACCESS_FS_MAKE_DIR: Create (or rename) a directory.

And this for the parent directory of report file:

LANDLOCK_ACCESS_FS_MAKE_REG: Create (or rename or link) a regular file.

The latter seems a bit much to me

@vlaci vlaci force-pushed the landlock branch 4 times, most recently from d2138c8 to 3c51313 Compare June 7, 2023 21:01
@vlaci
Copy link
Contributor Author

vlaci commented Jun 7, 2023

Hehe, this change is incompatible with code coverage measurement :D

 INTERNALERROR>   File "/home/runner/.cache/pypoetry/virtualenvs/unblob-PkeEArhf-py3.8/lib/python3.8/site-packages/coverage/sqldata.py", line 1064, in _connect
INTERNALERROR>     raise DataError(f"Couldn't use data file {self.filename!r}: {exc}") from exc
INTERNALERROR> coverage.exceptions.DataError: Couldn't use data file '/home/runner/work/unblob/unblob/.coverage.fv-az626-878.3677.365646': unable to open database file

@qkaiser qkaiser mentioned this pull request Aug 3, 2023
Merged
@qkaiser qkaiser mentioned this pull request Jan 8, 2024
Merged
@qkaiser qkaiser force-pushed the landlock branch 6 times, most recently from e92d6e3 to 52e9322 Compare January 8, 2024 10:01
@qkaiser
Copy link
Contributor

qkaiser commented Jan 8, 2024 •
edited

Two things to do before tagging as ready for review:

  • solve the ModuleNotFoundError: No module named 'unblob_native.sandbox' error by properly exposing it
  • add unit testing (unsupported platforms, different access checks)

@qkaiser qkaiser assigned qkaiser and unassigned qkaiser Jan 9, 2024
@qkaiser qkaiser added enhancement New feature or request dependencies Pull requests that update a dependency file labels Jan 9, 2024
@qkaiser qkaiser force-pushed the landlock branch 3 times, most recently from 59659a3 to c71c40a Compare February 4, 2024 10:47
@qkaiser qkaiser marked this pull request as ready for review February 4, 2024 10:49
@qkaiser
Copy link
Contributor

qkaiser commented Feb 21, 2024

onekey-sec/unblob-native#11

@qkaiser
Copy link
Contributor

qkaiser commented Feb 21, 2024

Will rebase once version 0.1.2 of unblob-native is in.

vlaci
vlaci commented Feb 21, 2024
View reviewed changes
unblob/processing.py Outdated
Comment on lines 129 to 140
if report_file:
restrictions += [
AccessFS.read_write(report_file),
AccessFS.make_reg(report_file.parent),
]

if "pytest" in sys.modules:
restrictions += [
AccessFS.read_write("/tmp"), # noqa: S108
AccessFS.read_write("/build"),
AccessFS.read_write(Path(__file__).parent.parent.resolve().as_posix()),
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

restrictions could be part of ExtractionConfig we pass for process_file, so test and report specific values could be set-up in fixture and command line processing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like your thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it

Copy link
Contributor

Choose a reason for hiding this comment

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

gave it a try yesterday but failed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found an interesting thing: the only test, that actually calls the cli and does a full extraction is the tests/test_cli.py::test_skip_extension case, everything else works with a mocked process_file :)

vlaci
vlaci commented Feb 21, 2024
View reviewed changes
unblob/processing.py Outdated Show resolved Hide resolved
qkaiser
qkaiser reviewed Feb 22, 2024
View reviewed changes
unblob/cli.py Show resolved Hide resolved
qkaiser
qkaiser reviewed Feb 26, 2024
View reviewed changes
unblob/processing.py
restrict_access(*config.sandbox_access_restrictions)
except SandboxError:
logger.warning("Sandboxing FS access is unavailable on this system, skipping.")
restrict_access(*config.sandbox_access_restrictions)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will lead to an unhandled SandboxError on non supported platforms.

@qkaiser
Copy link
Contributor

qkaiser commented Feb 29, 2024

rebased and solved conflicts

@qkaiser
Copy link
Contributor

qkaiser commented Mar 11, 2024

rebased and solved conflicts

vlaci and others added 3 commits April 15, 2024 12:03
@vlaci @qkaiser
feat: introduce landlock based sandboxing
c4392da 
Co-authored-by: Quentin Kaiser <quentin.kaiser@onekey.com>
@vlaci @qkaiser
tests: skip extensions cli test shouldn't call process file either
c530d3a 
all other tests in this file assert on `process_file` being called
with correct arguments. We need specific tests which test that the
configuration is interpreted correctly
@vlaci @qkaiser
wip: move sandbox config to cli
4ef1bad 
we should(?) have tests where sandboxing is enabled.
@qkaiser
Copy link
Contributor

qkaiser commented Apr 15, 2024

rebased and solved conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qkaiser qkaiser qkaiser left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vlaci @qkaiser