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

Remove (most) attacker-controlled error messages #537

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

deeplow
Copy link
Contributor

@deeplow deeplow commented Aug 31, 2023

Creates exceptions in the server code to be shared with the client via an identifying exit code. These exceptions are then reconstructed in the client.

Refs #456 but does not completely fix it. Unexpected exceptions and progress descriptions are still passed in Containers.

This was addressed in the process of fixing #430, since in Qubes we'll have server errors that we'll need to communicate.

@deeplow deeplow added this to the 0.5.0 milestone Aug 31, 2023
@deeplow deeplow requested a review from apyrgio August 31, 2023 11:42
@deeplow deeplow changed the title Remove attacker-controlled error messages Remove (most) attacker-controlled error messages Aug 31, 2023
@deeplow deeplow force-pushed the 456-remove-attacker-errors branch 2 times, most recently from 57795d3 to bc4993d Compare August 31, 2023 12:24
Copy link
Contributor

@apyrgio apyrgio left a comment

Choose a reason for hiding this comment

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

I have added some comments inline, but I also have one last general comment:

In order to remove all reliance on attacker-controlled messages, we should also mark our run_command invocations with the stage that the conversion takes place, probably with a Python constant, so that we can return an exit code that reflects it.

dangerzone/conversion/doc_to_pixels.py Outdated Show resolved Hide resolved
dangerzone/conversion/errors.py Outdated Show resolved Hide resolved
dangerzone/conversion/errors.py Outdated Show resolved Hide resolved
dangerzone/conversion/errors.py Outdated Show resolved Hide resolved
dangerzone/conversion/doc_to_pixels.py Outdated Show resolved Hide resolved
dangerzone/conversion/doc_to_pixels_qubes_wrapper.py Outdated Show resolved Hide resolved
dangerzone/isolation_provider/qubes.py Show resolved Hide resolved
@apyrgio
Copy link
Contributor

apyrgio commented Sep 14, 2023

I have resolved most of our discussions, and I've commented on some very small issues remaining. Other than that, feel free to merge the PR.

BTW, regarding this:

In order to remove all reliance on attacker-controlled messages, we should also mark our run_command invocations with the stage that the conversion takes place, probably with a Python constant, so that we can return an exit code that reflects it.

Let's just have it in mind for the general error handling PR.

Creates exceptions in the server code to be shared with the client via an
identifying exit code. These exceptions are then reconstructed in the
client.

Refs #456 but does not completely fix it. Unexpected exceptions and
progress descriptions are still passed in Containers.
Distinguish from podman or other errors in called binaries by shifting
the error codes by 128.
@deeplow deeplow force-pushed the 456-remove-attacker-errors branch 2 times, most recently from d81f895 to ee886ec Compare September 19, 2023 14:45
@deeplow deeplow merged commit 94f569c into main Sep 19, 2023
11 checks passed
@apyrgio apyrgio deleted the 456-remove-attacker-errors branch October 2, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants