CVE-2023-50446: Set permissions on log directory by Jontified · Pull Request #5398 · mullvad/mullvadvpn-app

Reviewable status: 9 of 14 files reviewed, 3 unresolved discussions (waiting on @Jontified)

mullvad-paths/src/windows.rs line 146 at r3 (raw file):

// Authenticated users SID https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers
let (authenticated\_users\_ea, authenticated\_users\_psid) =
    match create\_explicit\_access("S-1-5-11", GENERIC\_READ) {

This is fine, but it seems like we could have used CreateWellKnownSid(WinAuthenticatedUserSid, …) and CreateWellKnownSid(WinBuiltinAdministratorsSid, …) here.

A nice thing about it is that we allocate the memory ourselves, so most of the LocalFrees go away, and we only have to use plain arrays or Vecs. Cleaner, probably?

Though I vaguely recall that this did not work for some reason.

mullvad-paths/src/windows.rs line 217 at r3 (raw file):

let sid\_wide\_str = get\_wide\_str(sid\_wide\_str);
let mut psid = ptr::null\_mut();
if 0 == unsafe { ConvertStringSidToSidW(sid\_wide\_str.as\_ptr(), &mut psid) } {

I think we should avoid Yoda conditions, as they’re inconsistent with our general code style. The point of these is mainly to prevent accidental assignment, which isn’t relevant for if statements in Rust.