Skip to content

Add Assured's security audit report of the installer downloader#7866

Merged
faern merged 1 commit intomainfrom
security-audit-of-installer-des-1574
Mar 26, 2025
Merged

Add Assured's security audit report of the installer downloader#7866
faern merged 1 commit intomainfrom
security-audit-of-installer-des-1574

Conversation

@faern
Copy link
Member

@faern faern commented Mar 21, 2025

This PR adds the audit report from the audit of the desktop installer downloader for macOS and Windows. It also updates the relevant markdown files to link to it, and add our comments and responses about the audit and the findings.


This change is Reviewable

@faern faern requested a review from dlon March 21, 2025 14:07
@linear
Copy link

linear bot commented Mar 21, 2025

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions


audits/2025-03-20-assured-installer-downloader.md line 87 at r1 (raw file):

a result of a pre-audit meeting between Mullvad and Assured where we probably missunderstood
some of their early feedback on the metadata verification best practices. We will change
the implementation back to only use the verified data, as recommended.

It was reverted here: #7859. So the text could be updated to reflect that. :)


audits/2025-03-20-assured-installer-downloader.md line 110 at r1 (raw file):

protects against the attack. It can make the attack slightly harder to carry out, but nothing more.

We will update the documentation to not claim there are measures to protect against

Fixed here: #7858

If you're happy with those changes, this part could also updated too.

@faern faern force-pushed the security-audit-of-installer-des-1574 branch from db03b04 to ab48a7a Compare March 21, 2025 14:47
Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @dlon)


audits/2025-03-20-assured-installer-downloader.md line 87 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

It was reverted here: #7859. So the text could be updated to reflect that. :)

Nice! Added a link to the commit and reworded the sentence.


audits/2025-03-20-assured-installer-downloader.md line 110 at r1 (raw file):

//! This is potentially vulnerable to TOCTOU, ie replacing the file after its hash has been
//! verified, but only by the current user. However, this would require asking the user to approve
//! privilege escalation, just like the actual installer does.

I do not understand the formulation of this comment. The "However, this" part seem to refer to some fix, that the comment does not clearly talk about. The sentence seems to be missing something like "In order to mitigate this we would need to ...." or similar.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @faern)


audits/2025-03-20-assured-installer-downloader.md line 110 at r1 (raw file):

The "However, this" part seem to refer to some fix

It is just poorly worded. 😄 It should probably say "Exploiting this...", or something like that.

@faern faern force-pushed the security-audit-of-installer-des-1574 branch from ab48a7a to fa2ecc1 Compare March 25, 2025 15:02
Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @dlon)


audits/2025-03-20-assured-installer-downloader.md line 110 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

The "However, this" part seem to refer to some fix

It is just poorly worded. 😄 It should probably say "Exploiting this...", or something like that.

Fixed

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@faern faern force-pushed the security-audit-of-installer-des-1574 branch from fa2ecc1 to 0a0aaca Compare March 26, 2025 08:50
@faern faern merged commit 0a0aaca into main Mar 26, 2025
8 checks passed
@faern faern deleted the security-audit-of-installer-des-1574 branch March 26, 2025 08:53
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.

2 participants