Skip to content

nordvpn: harden sandboxing and fix cross-user permissions - #568291

Closed
novalkun wants to merge 6 commits into
NixOS:masterfrom
novalkun:nordvpn-security-fixes
Closed

novalkun wants to merge 6 commits into
NixOS:masterfrom
novalkun:nordvpn-security-fixes

Conversation

@novalkun

@novalkun novalkun commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Related: original hardening proposal #547437 and meshnet PR #564752.

Tighten nordvpnd's systemd sandboxing and fix startup and socket-mode issues that predate the meshnet work. The changes are adapted from different-error's patch, credited via Co-authored-by; they have been unable to update the original proposal directly due to a disputed account suspension.

Changes

  • Explicitly load the tun and wireguard kernel modules instead of relying on another service to load them.
  • Add ProtectSystem=strict, RestrictNamespaces, RestrictAddressFamilies, NoNewPrivileges, ProtectControlGroups, and an explicit TUN DeviceAllow. Convert capability settings to list form while retaining only CAP_NET_ADMIN.
  • Change nordvpnd.socket from 0770 to 0660: UNIX stream sockets need read/write access, not execute bits. Both modes already deny access to other users.
  • Start norduserd through default.target instead of graphical-session.target, so it also starts in non-graphical user sessions.
  • Run Go tests recursively with explicit exclusions for network access, privileged operations, and network state absent from the sandbox. Construct the skip regex as one quoted argument: indented shell string fragments previously left an empty alternative that skipped every daemon test.
  • Extend nixosTests.nordvpn to verify process ownership, exact capability bitmasks, polkit rules, loaded kernel modules, TUN device access through the service cgroup, and directory ownership/modes across all four user/group combinations. Add a headless scenario using a lingering user manager without a graphical session.

The documentation retains the reverse-path filtering configuration: systemd sandboxing does not replace that firewall setting. This PR remains independent of the meshnet PR.

How to test

Enable services.nordvpn.enable = true;, add the testing user to the nordvpn group, and set networking.firewall.checkReversePath = "loose"; when using the firewall. Build against this PR with:

nixos-rebuild build-vm \
  -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/refs/pull/568291/head.tar.gz \
  -I nixos-config=./configuration.nix

Check nordvpn status and inspect systemctl show nordvpnd.service. A real NordVPN account is required to exercise a VPN connection.

AI disclosure

Originally prepared with Claude Code (Claude Sonnet 5 / claude-sonnet-5), then reviewed and revised with OpenCode using GPT-6.1 Sol (openai/gpt-6.1-sol). AI assistance covered code and test changes, review, commit messages, and this description. Build and VM checks and live-test commands were executed through OpenCode; I performed the privileged activation and restoration of test configurations on my hardware. Relevant commits carry Assisted-by: trailers, and different-error's work retains Co-authored-by: attribution.

Things done

Testing performed locally

  • Built nordvpn.cli on x86_64-linux; the Go test suite passes. Confirmed the daemon tests now execute after fixing the skip regex, including daemon and daemon/dns. The only test in daemon/routes/ifgroup remains excluded because it requires privileged network changes.
  • Ran nixosTests.nordvpn: all six scenarios pass, covering the disabled module, four user/group combinations, and headless startup. In the headless scenario, norduserd is active while the user's graphical-session.target is inactive, and nordvpn status succeeds as that user.
  • Ran the repository formatter with --fail-on-change on the changed files: zero changes. git diff --check passes.
  • Live-tested on an x86_64-linux NixOS 26.05 host with only the NordVPN module/package replaced. Confirmed the sandbox settings, socket mode 0660, runtime/state directory mode 0750, and active daemons without restarts.
  • Inspected /proc/<pid>/status: effective, permitted, bounding, inheritable, and ambient capability sets are exactly CAP_NET_ADMIN (0x1000), with NoNewPrivs=1 and seccomp enabled.
  • Established a NordLynx VPN connection and retrieved an HTTPS page with HTTP 200. Disconnected afterward, restored the original system generation, and verified normal HTTPS access, active original daemons, and a byte-identical /etc/hosts.

The previous nixpkgs-review report built the earlier HEAD on x86_64-linux and aarch64-linux. This revision was tested locally on x86_64-linux; I have not run nixpkgs-review myself.

@nixpkgs-ci nixpkgs-ci Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for contributing to the documentation.

Follow the documentation style guide, especially:

  • Show, don’t tell. Start with a minimal working example. Explain after the code.
  • Avoid meta-commentary. Don’t write "This section explains how to…".
  • Use present tense, imperative mood, and active voice. For example: "Run the command," not "The user should run the following command."
  • Avoid vague references like "this" when the referent is unclear. Write "This command creates a folder," not "This will create a folder."
  • Be direct. Avoid "should", "might", "typically", and "usually".
  • Cut filler words like "simply", "just", "easily", and "basically". Use "to" instead of "in order to".

For larger changes, such as adding or removing whole sections, ask the NixOS documentation team for review.

novalkun added a commit to novalkun/nixpkgs that referenced this pull request Sep 29, 2026
Drop hedging language ("should", "some notes on") flagged by the
nixpkgs docs style guide bot on NixOS#568291.

Assisted-by: Claude Code (Sonnet 5, claude-sonnet-5)
@novalkun
novalkun force-pushed the nordvpn-security-fixes branch from 762bc76 to bee82b2 Compare September 29, 2026 14:46
@nixpkgs-ci nixpkgs-ci Bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 9.needs: reviewer This PR currently has no reviewers requested and needs attention. llm-assisted Contributions that include code generation with LLMs 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 29, 2026
@nixos-discourse

Copy link
Copy Markdown

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/7658

@novalkun

novalkun commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

cc @andersonjoseph, would appreciate a review.

@nixpkgs-review-gha

Copy link
Copy Markdown

✅ nixpkgs-review result

Generated using nixpkgs-review-gha (a0a3a8d)

Command: nixpkgs-review pr 568291
Commit: bee82b2c112ac44000b81ab71bfa6dd17e63669a (subsequent changes)
Merge: 9689d4e98eba1976d9d15af8a7f63ba882ca2310

Triggered by @novalkun (43009254)
Logs: https://github.com/novalkun/nixpkgs-review-gha/actions/runs/37017854380/attempts/1


x86_64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 1 package built:
  • nordvpn

aarch64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 1 package built:
  • nordvpn

@SuperSandro2000

Copy link
Copy Markdown
Member

This PR touches many things and should not be one big commit.

@novalkun
novalkun force-pushed the nordvpn-security-fixes branch from bee82b2 to 7ab6926 Compare October 3, 2026 23:35
@novalkun

novalkun commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

This PR touches many things and should not be one big commit.

thank you. I have splitted this into 6 commits per logical unit

@sheeeng

sheeeng commented Oct 12, 2026

Copy link
Copy Markdown
Member

✅ nixpkgs-review result

Generated using nixpkgs-review-gha (c28ca54)

Command: nixpkgs-review pr 568291
Commit: 7ab69267617cb0f2bcd16e6371838a46f8dc5122 (subsequent changes)
Merge: 52befd4932710c6e232e8b2007e034ed1710a451

Logs: https://github.com/sheeeng/defelo-nixpkgs-review-gha/actions/runs/38228936138/attempts/1


x86_64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 1 package built:
  • nordvpn

aarch64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 1 package built:
  • nordvpn

x86_64-darwin

✅ No rebuilds


aarch64-darwin

✅ No rebuilds

novalkun and others added 6 commits October 14, 2026 21:01
Run cli/fileshare/norduser tests recursively, and run the daemon test
suite with an explicit skip list for tests requiring network access,
root/CAP_NET_ADMIN, or network state the sandbox does not provide.

Join the skip patterns into one quoted regex. Splitting shell string
fragments across indented lines left a trailing empty alternative in
the first argument, unintentionally skipping all daemon tests.

Co-authored-by: different-error <[email protected]>
Assisted-by: Claude Code (Sonnet 5, claude-sonnet-5)
Assisted-by: OpenCode (GPT-6.1 Sol, openai/gpt-6.1-sol)
Previously relied on them being loaded some other way.

Co-authored-by: different-error <[email protected]>
Assisted-by: Claude Code (Sonnet 5, claude-sonnet-5)
UNIX stream sockets do not need execute bits. Use 0660 instead of 0770
to retain read/write access for the owner and group. Both modes already
deny access to other users.

Co-authored-by: different-error <[email protected]>
Assisted-by: Claude Code (Sonnet 5, claude-sonnet-5)
Assisted-by: OpenCode (GPT-6.1 Sol, openai/gpt-6.1-sol)
Headless/non-graphical sessions were failing to get it started.

Co-authored-by: different-error <[email protected]>
Assisted-by: Claude Code (Sonnet 5, claude-sonnet-5)
Add ProtectSystem = "strict", RestrictNamespaces,
RestrictAddressFamilies, NoNewPrivileges, ProtectControlGroups,
DeviceAllow to nordvpnd's unit, and convert AmbientCapabilities/
CapabilityBoundingSet to list form.

Keep the reverse-path filtering advice in the documentation: these
sandboxing changes do not replace the firewall configuration.

Co-authored-by: different-error <[email protected]>
Assisted-by: Claude Code (Sonnet 5, claude-sonnet-5)
Assisted-by: OpenCode (GPT-6.1 Sol, openai/gpt-6.1-sol)
Assert the exact capability bitmask on the running nordvpnd process
(/proc/<pid>/status), verify polkit rules, kernel modules, TUN device
access via the service's cgroup, and runtime/state directory
ownership+permissions across all four user/group config combinations.

Also cover norduserd startup through a lingering user manager without
a graphical session, and check that the user can query nordvpn status.

Co-authored-by: different-error <[email protected]>
Assisted-by: Claude Code (Sonnet 5, claude-sonnet-5)
Assisted-by: OpenCode (GPT-6.1 Sol, openai/gpt-6.1-sol)
@novalkun
novalkun force-pushed the nordvpn-security-fixes branch from 7ab6926 to 65b1b87 Compare October 14, 2026 14:04
@novalkun novalkun mentioned this pull request Oct 14, 2026
6 of 13 tasks
@sheeeng

sheeeng commented Oct 14, 2026

Copy link
Copy Markdown
Member

✅ nixpkgs-review result

Generated using nixpkgs-review-gha (c28ca54)

Command: nixpkgs-review pr 568291
Commit: 65b1b87b7a2bfd108e62fa6fde7da709aa3319a4 (subsequent changes)
Merge: 5ee776b339192fcfee81163d58798b6d3ccd6e55

Logs: https://github.com/sheeeng/defelo-nixpkgs-review-gha/actions/runs/38607829124/attempts/1


x86_64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 1 package built:
  • nordvpn

aarch64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 1 package built:
  • nordvpn

x86_64-darwin

✅ No rebuilds


aarch64-darwin

✅ No rebuilds

@ofborg ofborg Bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Oct 17, 2026
@novalkun

Copy link
Copy Markdown
Contributor Author

Closing in favor of #547437 — different-error's account is back and they've rebased/revived the original PR this was adapted from. Same change, original author, already has live QA from andersonjoseph.

@novalkun novalkun closed this Oct 17, 2026
@novalkun
novalkun deleted the nordvpn-security-fixes branch October 18, 2026 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 9.needs: reviewer This PR currently has no reviewers requested and needs attention. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. llm-assisted Contributions that include code generation with LLMs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants