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

Qubes: maliciously creafted PDF could limit the number of pages converted #565

Closed
deeplow opened this issue Oct 2, 2023 · 0 comments · Fixed by #566
Closed

Qubes: maliciously creafted PDF could limit the number of pages converted #565

deeplow opened this issue Oct 2, 2023 · 0 comments · Fixed by #566
Assignees
Labels
P:Qubes QubesOS integration timeout Dangerzone Times Out
Milestone

Comments

@deeplow
Copy link
Contributor

deeplow commented Oct 2, 2023

7 months ago in commit aeeed4

  • search = re.search(r"^Pages: (\d+)", line) got replaced with
  • search = re.search(r"Pages:\s*(\d+)\s*\n", stdout.decode())
    This code parsed the output of pdfinfo and this meant that if the PDF had metadata called Pages: 1, the end "safe" pdf would be 1 page long, regardless of the original length.

This document is a proof of concept. It has 4 pages but when converted (in Qubes) it results in 1 page. This is not security-critical since the Qubes port is in alpha and with no security guarantees and in the containers version (Windows, macOS and Linux) it only affects the timeout.

A fix is pretty simple:

diff --git a/dangerzone/conversion/doc_to_pixels.py b/dangerzone/conversion/doc_to_pixels.py
index 38f16150..48f301dc 100644
--- a/dangerzone/conversion/doc_to_pixels.py
+++ b/dangerzone/conversion/doc_to_pixels.py
@@ -254,7 +254,7 @@ class DocumentToPixels(DangerzoneConverter):
             timeout=timeout,
         )
 
-        search = re.search(r"Pages:\s*(\d+)\s*\n", stdout.decode())
+        search = re.search(r"^Pages:\s*(\d+)\s*\n", stdout.decode(), re.MULTILINE)
         if search is not None:
             num_pages: int = int(search.group(1))
         else:

However, a more long-term fix is finding python bindings for pdfinfo or a similar tool with such bindings or python module.

@deeplow deeplow self-assigned this Oct 2, 2023
deeplow added a commit that referenced this issue Oct 2, 2023
This should only affect the alpha version of Qubes OS (in containers
it only allows the attacker to control the timeout). In short, an
attacker could have PDF metadata that would show before "Pages:" in
the `pdfinfo` command output and this would essentially override the
number of pages measured in the server. This could enable the attacker
to shorten the number of pages of a document for example.

Fixes #565
deeplow added a commit that referenced this issue Oct 2, 2023
This should only affect the alpha version of Qubes OS (in containers
it only allows the attacker to control the timeout). In short, an
attacker could have PDF metadata that would show before "Pages:" in
the `pdfinfo` command output and this would essentially override the
number of pages measured in the server. This could enable the attacker
to shorten the number of pages of a document for example.

Fixes #565
@deeplow deeplow added P:Qubes QubesOS integration timeout Dangerzone Times Out labels Oct 2, 2023
@deeplow deeplow added this to the 0.5.0 milestone Oct 2, 2023
deeplow added a commit that referenced this issue Oct 2, 2023
This should only affect the alpha version of Qubes OS (in containers
it only allows the attacker to control the timeout). In short, an
attacker could have PDF metadata that would show before "Pages:" in
the `pdfinfo` command output and this would essentially override the
number of pages measured in the server. This could enable the attacker
to shorten the number of pages of a document for example.

Fixes #565
@deeplow deeplow closed this as completed in 7daeccd Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P:Qubes QubesOS integration timeout Dangerzone Times Out
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant