Set permissions on log directory

[Jontified]

Move a bit of the C++ code to rust and make sure that log directory is protected with admin permissions.

Fixes privilege escalation on Windows (CVE-2023-50446). Insufficient permissions on directory in Mullvad VPN Windows app in version 2023.5 and older allows any local unprivileged user to escalate privileges to SYSTEM.

---

This change is 

[dlon]

Reviewed 7 of 14 files at r1, 2 of 3 files at r3, all commit messages.
Reviewable status: 9 of 14 files reviewed, 2 unresolved discussions (waiting on @Jontified)

---

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

/// only directories that do not already exist and the leaf directory will have their permissions set.
pub fn create_dir_recursive(path: &Path, set_security_permissions: bool) -> Result<()> {
    if set_security_permissions {

Are the branches flipped? set_security_permissions == true seems to imply that permissions should be set.

---

windows/windows-libraries line 1 at r3 (raw file):

Subproject commit 6df03881c5bbde740ba601906594be61b99319a7

In my opinion, wrapping create_privileged_directory in libcommon creates a strange dependency for libcommon on the app repository. That's supposed to be an independent library. Moreover, the point of mullvad-nsis it to depend less on it, if anything.

I suggest either (a) use create_privileged_directory directly in the projects, or (b) put the wrapper in libshared (not a submodule).

[dlon]

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.

[faern]

with the given security attributes

This function does not take any security attributes.

[Jontified]

Reviewable status: 8 of 14 files reviewed, 4 unresolved discussions (waiting on @dlon and @faern)

---

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

Previously, faern (Linus Färnstrand) wrote…

with the given security attributes

This function does not take any security attributes.

Done.

---

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

Previously, dlon (David Lönnhager) wrote…

Are the branches flipped? set_security_permissions == true seems to imply that permissions should be set.

Done.

---

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

Previously, dlon (David Lönnhager) wrote…

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.

Done.

---

windows/windows-libraries line 1 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

In my opinion, wrapping create_privileged_directory in libcommon creates a strange dependency for libcommon on the app repository. That's supposed to be an independent library. Moreover, the point of mullvad-nsis it to depend less on it, if anything.

I suggest either (a) use create_privileged_directory directly in the projects, or (b) put the wrapper in libshared (not a submodule).

Done. Since it's only called in log I think it's cleaner to just have the call there.

[dlon]

Reviewed 2 of 14 files at r1, 1 of 1 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faern and @Jontified)

---

mullvad-nsis/Cargo.toml line 18 at r6 (raw file):

[target.i686-pc-windows-msvc.build-dependencies]
cbindgen = { version = "0.24.3", default-features = false }

Nit: Diff just removes a newline.

---

mullvad-paths/src/windows.rs line 245 at r6 (raw file):

    unsafe { LocalFree(new_dacl as isize) };

    if ERROR_SUCCESS != result {

Still one over here, but it's not a big deal.

---

windows/nsis-plugins/src/log/log.cpp line 301 at r6 (raw file):

        if (Status::Ok != create_privileged_directory(reinterpret_cast<const uint16_t*>(w_path)))
        {
            THROW_ERROR("Failed to create privileged directory");

Nit: Fine, but why a needlessly vague "privileged directory" rather than "log directory"?

---

windows/nsis-plugins/src/log/log.vcxproj line 103 at r6 (raw file):

      <ModuleDefinitionFile>log.def</ModuleDefinitionFile>
    </Link>
  </ItemDefinitionGroup>

Looks like this file is missing a pre build event (see cleanup.vcxproj).

---

windows/nsis-plugins/src/tray/tray.vcxproj line 72 at r6 (raw file):

      <GenerateDebugInformation>true</GenerateDebugInformation>
      <ImageHasSafeExceptionHandlers>false</ImageHasSafeExceptionHandlers>
      <AdditionalLibraryDirectories>$(ProjectDir)../../../../target/i686-pc-windows-msvc/release;$(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/nsis/;$(SolutionDir)bin\$(Platform)-$(Configuration)\</AdditionalLibraryDirectories>

Can the changes be reverted for this project?

---

windows/windows-libraries line 1 at r3 (raw file):

Previously, Jontified wrote…

Done. Since it's only called in log I think it's cleaner to just have the call there.

Awesome!

[dlon]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @faern and @Jontified)

---

windows/nsis-plugins/src/log/log.cpp line 297 at r6 (raw file):

		logpath.append(L"Mullvad VPN");

        const wchar_t* w_path = logpath.wstring().c_str();

Should be tabs.

[Jontified]

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

---

windows/nsis-plugins/src/log/log.cpp line 297 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should be tabs.

Done.

---

windows/nsis-plugins/src/log/log.vcxproj line 103 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Looks like this file is missing a pre build event (see cleanup.vcxproj).

Done.

[dlon]

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

[dlon]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jontified)

---

windows/nsis-plugins/src/log/log.cpp line 297 at r7 (raw file):

		logpath.append(L"Mullvad VPN");

		const wchar_t* w_path = logpath.wstring().c_str();

Use after free -- the std::wstring is destroyed here.

[Jontified]

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

---

windows/nsis-plugins/src/log/log.cpp line 297 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Use after free -- the std::wstring is destroyed here.

Good catch, done.

[dlon]

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