Block traffic before the network is up on Linux

[pinkisemils]

There have been reported leaks that happen due to the daemon starting too late. These changes address this leak window in two ways:

• the daemon may now be started before the networking daemons start
• a new systemd unit that is started before basic.target is added that will setup blocking rules is added.

To support the daemon starting sooner, it must now wait for the network managmenet services to be started to manage DNS, so some new DBus code has been added to do that via RPCs and signals with systemd.

To support blocking traffic during early boot, the daemon now has a new argument for this exact purpose - the daemon will start, initialize the firewall to a blocked state and exit.

In addition to the changes above, the daemon's unit file is changed so that the resource directory is set explicitly via an environment variable, the /opt/ mountpoint is listed as a dependency, and the daemon unit is listed as a dependency of some known network management services. The early boot blocking unit is listed as a dependency of the daemon and the basic.target - this ensures it's ran before any of the networking services have started. To ensure that the dameon binary and the unit files are accessible during early boot, they have been moved to /usr/....

---

This change is 

[faern]

Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

---

dist-assets/linux/mullvad-early-boot-blocking.service line 1 at r2 (raw file):

# Systemd service unit file to block all traffic during early boot.

This file is very similar to the one above. I suppose only one of them should be in the PR?

[faern]

Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

---

mullvad-daemon/src/early_boot_firewall.rs line 30 at r2 (raw file):

            endpoint: talpid_types::net::Endpoint {
                address: (std::net::Ipv4Addr::LOCALHOST, 0).into(),
                protocol: talpid_types::net::TransportProtocol::Tcp,

Seems a bit hackish to allow 127.0.0.1:0/TCP. At the very least this needs a comment explaining itself. And the best would of course be to restructure stuff a bit to not need it at all.

[dlon]

Reviewed 7 of 18 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @pinkisemils)

---

dist-assets/linux/before-install.sh line 9 at r2 (raw file):

        systemctl stop mullvad-daemon.service
        systemctl disable mullvad-daemon.service
        systemctl stop mullvad-early-boot-blocking.service || true

Should this not be unconditional (on mullvad-daemon running)? systemctl status above will only return 0 if mullvad-daemon is running.

---

dist-assets/linux/mullvad-early-boot-block.service line 1 at r2 (raw file):

# Systemd service unit file to block all traffic during early boot.

This is never used.

---

talpid-dbus/src/systemd/mod.rs line 122 at r2 (raw file):

                move |job_finished: JobRemovedSignal, _connection, _message| {
                    if job_finished.primary_name == unit_name {
                        println!(

Use log or remove this.

---

talpid-dbus/src/systemd/mod.rs line 167 at r2 (raw file):

                .map(Unit::is_active)
                .map(|is_active| {
                    println!("Only checking if unit is active after not receiving a signal about it - {is_active}");

Use log or remove this.

[dlon]

Reviewed 4 of 18 files at r1.
Reviewable status: 12 of 19 files reviewed, 8 unresolved discussions (waiting on @faern and @pinkisemils)

---

dist-assets/linux/post-transaction.sh line 8 at r2 (raw file):

# Repeated enablement of the daemon service will result in the early-boot unit
# being executed when the daemon is already running, which results in the
# firewall rules being applied.

Would it make sense to check whether the daemon is running in early-boot instead? That way, it never hurts if the blocking unit is somehow started after the daemon.

---

mullvad-daemon/src/early_boot_firewall.rs line 1 at r2 (raw file):

use mullvad_daemon::settings::{self, SettingsPersister};

I wonder if this might belong in mullvad-setup instead (where we have reset-firewall, etc.). But I don't think it matters too much.

---

mullvad-daemon/src/early_boot_firewall.rs line 30 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Seems a bit hackish to allow 127.0.0.1:0/TCP. At the very least this needs a comment explaining itself. And the best would of course be to restructure stuff a bit to not need it at all.

The allowed endpoint should probably be optional.

---

talpid-core/src/dns/linux/network_manager.rs line 32 at r2 (raw file):

        let sd = systemd::Systemd::new()?;

        if sd.network_manager_will_run()? {

Echoing what was said elsewhere: Can this not just rely on systemd to ensure that NetworkManager is running here?

[dlon]

Reviewed 4 of 18 files at r1, 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @faern and @pinkisemils)

---

CHANGELOG.md line 39 at r3 (raw file):

Add

I also think it should be slightly more descriptive. "Block network traffic on boot before the daemon starts"?

---

dist-assets/linux/mullvad-daemon.service line 7 at r3 (raw file):

Description=Mullvad VPN daemon
Before=network-online.target
After=mullvad-early-boot-blocking.service

Should these not be added back now?

After=network-online.target
After=NetworkManager.service
After=systemd-resolved.service

[dlon]

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @faern and @pinkisemils)

[pinkisemils]

Reviewable status: 21 of 25 files reviewed, 6 unresolved discussions (waiting on @dlon, @faern, and @pinkisemils)

---

dist-assets/linux/before-install.sh line 9 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should this not be unconditional (on mullvad-daemon running)? systemctl status above will only return 0 if mullvad-daemon is running.

Really, due to the short-lived nature of the blocking unit, this line should be removed in it's entirety - the unit runs in less than a second and must only run once during bootup.

---

dist-assets/linux/mullvad-daemon.service line 7 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should these not be added back now?

After=network-online.target
After=NetworkManager.service
After=systemd-resolved.service

Yep.

---

dist-assets/linux/mullvad-early-boot-blocking.service line 1 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This file is very similar to the one above. I suppose only one of them should be in the PR?

Done.

---

dist-assets/linux/post-transaction.sh line 8 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Would it make sense to check whether the daemon is running in early-boot instead? That way, it never hurts if the blocking unit is somehow started after the daemon.

That way lies race conditions, unfortunately. The daemon only needs to be enabled if it isn't enabled.

---

mullvad-daemon/src/early_boot_firewall.rs line 30 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

The allowed endpoint should probably be optional.

Making it optional seems to be the correct thing to do here.

[dlon]

Reviewed 4 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon and @faern)

[faern]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon and @pinkisemils)

---

CHANGELOG.md line 39 at r6 (raw file):

- GUI: Add electron flags to run Wayland native if in a compositor/desktop known to work well
- Add support for Linux ARM64.
- Added traffic blocking during early boot, before the daemon starts.

This should definitely be under ### Security. And mention it stops potential leaks happening when systemd starts our daemon late.

[pinkisemils]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon and @pinkisemils)

---

talpid-core/src/dns/linux/network_manager.rs line 32 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Echoing what was said elsewhere: Can this not just rely on systemd to ensure that NetworkManager is running here?

Just because it's running doesn't necessarily mean it's working in a way that's usable by our daemon. Just because the service is running doesn't imply it's managing DNS too, unfortunately.

[dlon]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon and @pinkisemils)

---

dist-assets/linux/before-install.sh line 9 at r2 (raw file):
If I understand this description of oneshot from the man page correctly, the service is considered to be up forever once started:

Behavior of oneshot is similar to simple; however, the service manager will consider the unit up after the main process exits.

---

dist-assets/linux/post-transaction.sh line 8 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

That way lies race conditions, unfortunately. The daemon only needs to be enabled if it isn't enabled.

Couldn't mullvad-daemon --initialize-early-boot-firewall check if the UDS exists, like the daemon in general does if you try to launch another instance of it?

But it's not important since it only matters if the services are launched in the wrong order.

---

talpid-core/src/dns/linux/network_manager.rs line 32 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Just because it's running doesn't necessarily mean it's working in a way that's usable by our daemon. Just because the service is running doesn't imply it's managing DNS too, unfortunately.

This is good. I meant to submit the comment before the systemd module was removed, so it's moot.

[faern]

Reviewed 1 of 2 files at r7.
Reviewable status: 24 of 25 files reviewed, 3 unresolved discussions (waiting on @dlon and @pinkisemils)

[dlon]

Reviewed 1 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

[pinkisemils]

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

---

dist-assets/linux/before-install.sh line 9 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

If I understand this description of oneshot from the man page correctly, the service is considered to be up forever once started:

Behavior of oneshot is similar to simple; however, the service manager will consider the unit up after the main process exits.

The unit will be in an active state, but it only has to be activated once, but it doesn't imply that the service will continue running.

---

dist-assets/linux/post-transaction.sh line 8 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Couldn't mullvad-daemon --initialize-early-boot-firewall check if the UDS exists, like the daemon in general does if you try to launch another instance of it?

But it's not important since it only matters if the services are launched in the wrong order.

The daemon could well be starting up and not yet created the UDS when the early boot unit would be executing. It'd work out if the early boot unit was to create the UDS to force all subsequent daemon instances to wait on it, but that would be messy since currently the daemon (rightfully IMO) shuts down if the control UDS already exists.

[dlon]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

---

CHANGELOG.md line 94 at r8 (raw file):

### Security
#### Linux
- Added traffic blocking during early boot, before the daemon starts, to prevent leaks in the case

This belongs under Unreleased.

---

dist-assets/linux/before-install.sh line 9 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

The unit will be in an active state, but it only has to be activated once, but it doesn't imply that the service will continue running.

Got it. I suppose you could remove this, unless systemctl enable ... will fail if you do.

---

dist-assets/linux/post-transaction.sh line 8 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

The daemon could well be starting up and not yet created the UDS when the early boot unit would be executing. It'd work out if the early boot unit was to create the UDS to force all subsequent daemon instances to wait on it, but that would be messy since currently the daemon (rightfully IMO) shuts down if the control UDS already exists.

That's fair.

[pinkisemils]

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

---

CHANGELOG.md line 94 at r8 (raw file):

Previously, dlon (David Lönnhager) wrote…

This belongs under Unreleased.

Done.

---

dist-assets/linux/before-install.sh line 9 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Got it. I suppose you could remove this, unless systemctl enable ... will fail if you do.

It shouldn't fail, so I have removed this.

[dlon]

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