diff --git a/daemon/jobs_test.go b/daemon/jobs_test.go index 275d34f3..bf85de32 100644 --- a/daemon/jobs_test.go +++ b/daemon/jobs_test.go @@ -36,6 +36,7 @@ func (failingLoginChecker) IsMFAEnabled() (bool, error) { return false, nil } func (failingLoginChecker) IsVPNExpired() (bool, error) { return true, errors.New("IsVPNExpired error") } + func (failingLoginChecker) GetDedicatedIPServices() ([]auth.DedicatedIPService, error) { return nil, fmt.Errorf("Not implemented") } @@ -128,6 +129,10 @@ func (n *meshNetworker) AllowFileshare(address meshnet.UniqueAddress) error { return nil } +func (n *meshNetworker) PermitFileshare() error { + return nil +} + func (n *meshNetworker) AllowIncoming(address meshnet.UniqueAddress, lanAllowed bool) error { n.allowedIncoming = append(n.allowedIncoming, address) return nil @@ -138,6 +143,10 @@ func (n *meshNetworker) BlockIncoming(address meshnet.UniqueAddress) error { return nil } +func (n *meshNetworker) ForbidFileshare() error { + return nil +} + func (n *meshNetworker) BlockFileshare(address meshnet.UniqueAddress) error { n.blockedFileshare = append(n.blockedFileshare, address) return nil diff --git a/go.sum b/go.sum index 371cff2e..bb1c7480 100644 --- a/go.sum +++ b/go.sum @@ -12,8 +12,6 @@ github.com/NordSecurity/gopenvpn v0.0.0-20230117114932-2252c52984b4 h1:2ozEjYEw4 github.com/NordSecurity/gopenvpn v0.0.0-20230117114932-2252c52984b4/go.mod h1:tguhorMSnkMcQExNIHWBX6TRhFeGYlERzbeAWZ4j9Uw= github.com/NordSecurity/libdrop-go/v8 v8.0.0-20241017064027-670787595588 h1:L/nAbQXJGCOFqw1eTTRYEBKmiuaQQeS7b863+0Ifevw= github.com/NordSecurity/libdrop-go/v8 v8.0.0-20241017064027-670787595588/go.mod h1:SRYI0D0K6hSMBskvcB2/t/5ktSTNLPGbOvLaQ5p/sAE= -github.com/NordSecurity/libtelio-go/v5 v5.1.4 h1:o2JbYad8sdRsljFAMZRVmkXGQ7kTVbS32P6TTDOTuL0= -github.com/NordSecurity/libtelio-go/v5 v5.1.4/go.mod h1:mnoTGgXOu8dBQgPxG8MBju4d9C+ljKIT2p8OX5GFom4= github.com/NordSecurity/libtelio-go/v5 v5.1.5 h1:lwP7m2GJ+GkO1EDaRqm5ymDT/CtjIBC/1bN2CL55mnY= github.com/NordSecurity/libtelio-go/v5 v5.1.5/go.mod h1:mnoTGgXOu8dBQgPxG8MBju4d9C+ljKIT2p8OX5GFom4= github.com/NordSecurity/systray v0.0.0-20240327004800-3e3b59c1b83d h1:oUEFXgFRa9Svcjr+O1stzR3vEXZ5OfQxLUcDjqFcOuo= diff --git a/meshnet/jobs.go b/meshnet/jobs.go index cf469d14..dadee687 100644 --- a/meshnet/jobs.go +++ b/meshnet/jobs.go @@ -15,7 +15,7 @@ func (s *Server) StartJobs() { } if _, err := s.scheduler.NewJob( - gocron.DurationJob(5*time.Second), + gocron.DurationJob(1*time.Second), gocron.NewTask(JobMonitorFileshareProcess(s)), gocron.WithName("job monitor fileshare process")); err != nil { log.Println(internal.WarningPrefix, "job monitor fileshare process schedule error:", err) @@ -39,33 +39,58 @@ func JobRefreshMeshnet(s *Server) func() error { } func JobMonitorFileshareProcess(s *Server) func() error { - oldState := false - return func() error { - if !s.isMeshOn() { - return nil - } - newState := internal.IsProcessRunning(internal.FileshareBinaryPath) - if newState == oldState { - // only state change triggers the modifications - return nil - } - - log.Println(internal.InfoPrefix, "fileshare change to running", newState) - peers, err := s.listPeers() - if err != nil { - return err - } + job := monitorFileshareProcessJob{ + isFileshareAllowed: false, + meshChecker: s, + rulesController: s.netw, + processChecker: defaultProcessChecker{}, + } + return job.run +} - isFileshareUp := newState - for _, peer := range peers { - if !isFileshareUp { - s.netw.BlockFileshare(UniqueAddress{UID: peer.PublicKey, Address: peer.Address}) - } else { - s.netw.AllowFileshare(UniqueAddress{UID: peer.PublicKey, Address: peer.Address}) +func (j *monitorFileshareProcessJob) run() error { + if !j.meshChecker.isMeshOn() { + if j.isFileshareAllowed { + if err := j.rulesController.ForbidFileshare(); err == nil { + j.isFileshareAllowed = false } } - oldState = newState - return nil } + + if j.processChecker.isFileshareRunning() { + j.rulesController.PermitFileshare() + j.isFileshareAllowed = true + } else { + j.rulesController.ForbidFileshare() + j.isFileshareAllowed = false + } + + return nil +} + +type defaultProcessChecker struct{} + +func (defaultProcessChecker) isFileshareRunning() bool { + return internal.IsProcessRunning(internal.FileshareBinaryPath) +} + +type monitorFileshareProcessJob struct { + isFileshareAllowed bool + meshChecker meshChecker + rulesController rulesController + processChecker processChecker +} + +type meshChecker interface { + isMeshOn() bool +} + +type rulesController interface { + ForbidFileshare() error + PermitFileshare() error +} + +type processChecker interface { + isFileshareRunning() bool } diff --git a/meshnet/jobs_test.go b/meshnet/jobs_test.go new file mode 100644 index 00000000..16bfede9 --- /dev/null +++ b/meshnet/jobs_test.go @@ -0,0 +1,126 @@ +package meshnet + +import ( + "errors" + "testing" + + "github.com/NordSecurity/nordvpn-linux/test/category" + "github.com/stretchr/testify/assert" +) + +func TestJobMonitorFileshare(t *testing.T) { + category.Set(t, category.Unit) + + tests := []struct { + name string + isFileshareInitiallyAllowed bool + meshChecker meshChecker + processChecker processChecker + wasForbidCalled bool + wasPermitCalled bool + shouldRulesChangeFail bool + isFileshareFinallyAllowed bool + }{ + { + name: "nothing happens with disabled meshnet", + meshChecker: meshCheckerStub{isMeshnetOn: false}, + wasForbidCalled: false, + wasPermitCalled: false, + isFileshareInitiallyAllowed: false, + isFileshareFinallyAllowed: false, + }, + { + name: "fileshare is allowed when meshnet is on and process is running", + meshChecker: meshCheckerStub{isMeshnetOn: true}, + processChecker: processCheckerStub{isFileshareUp: true}, + wasForbidCalled: false, + wasPermitCalled: true, + isFileshareInitiallyAllowed: false, + isFileshareFinallyAllowed: true, + }, + { + name: "fileshare is forbidden when meshnet is on process was stopped", + meshChecker: meshCheckerStub{isMeshnetOn: true}, + processChecker: processCheckerStub{isFileshareUp: false}, + wasForbidCalled: true, + wasPermitCalled: false, + isFileshareInitiallyAllowed: true, + isFileshareFinallyAllowed: false, + }, + { + name: "when mesh is off, but fileshare was allowed in previous run, now it gets blocked", + meshChecker: meshCheckerStub{isMeshnetOn: false}, + wasForbidCalled: true, + wasPermitCalled: false, + isFileshareInitiallyAllowed: true, + isFileshareFinallyAllowed: false, + }, + { + name: "when mesh is off, fileshare was allowed in previous run, the state does not change until block succeeds", + meshChecker: meshCheckerStub{isMeshnetOn: false}, + wasForbidCalled: true, + wasPermitCalled: false, + shouldRulesChangeFail: true, + isFileshareInitiallyAllowed: true, + isFileshareFinallyAllowed: true, + }, + } + + for _, tt := range tests { + rulesController := rulesControllerSpy{shouldFail: tt.shouldRulesChangeFail} + job := monitorFileshareProcessJob{ + isFileshareAllowed: tt.isFileshareInitiallyAllowed, + meshChecker: tt.meshChecker, + rulesController: &rulesController, + processChecker: tt.processChecker, + } + assert.Equal(t, tt.isFileshareInitiallyAllowed, job.isFileshareAllowed) + assert.False(t, rulesController.wasForbidCalled) + assert.False(t, rulesController.wasPermitCalled) + + err := job.run() + + assert.Nil(t, err) + assert.Equal(t, tt.isFileshareFinallyAllowed, job.isFileshareAllowed) + assert.Equal(t, tt.wasForbidCalled, rulesController.wasForbidCalled) + assert.Equal(t, tt.wasPermitCalled, rulesController.wasPermitCalled) + } +} + +type meshCheckerStub struct { + isMeshnetOn bool +} + +func (m meshCheckerStub) isMeshOn() bool { + return m.isMeshnetOn +} + +type rulesControllerSpy struct { + wasForbidCalled bool + wasPermitCalled bool + shouldFail bool +} + +func (rc *rulesControllerSpy) ForbidFileshare() error { + rc.wasForbidCalled = true + if rc.shouldFail { + return errors.New("intentional failure for testing") + } + return nil +} + +func (rc *rulesControllerSpy) PermitFileshare() error { + rc.wasPermitCalled = true + if rc.shouldFail { + return errors.New("intentional failure for testing") + } + return nil +} + +type processCheckerStub struct { + isFileshareUp bool +} + +func (m processCheckerStub) isFileshareRunning() bool { + return m.isFileshareUp +} diff --git a/meshnet/networker.go b/meshnet/networker.go index 3942cf84..57e4743e 100644 --- a/meshnet/networker.go +++ b/meshnet/networker.go @@ -28,8 +28,12 @@ type Networker interface { BlockIncoming(UniqueAddress) error // AllowFileshare creates a rule enabling fileshare port for the given address AllowFileshare(UniqueAddress) error + // PermitFileshare creates a rules enabling fileshare port for all available peers and sets fileshare as permitted + PermitFileshare() error // BlockFileshare removes a rule enabling fileshare port for the given address if it exists BlockFileshare(UniqueAddress) error + // ForbidFileshare removes a rules enabling fileshare port for all available peers and sets fileshare as forbidden + ForbidFileshare() error // ResetRouting is used when there are routing setting changes, // except when routing is denied - then BlockRouting must be used. changedPeer is the peer whose routing settings // changed, peers is the map of all the machine peers(including the changed peer). diff --git a/meshnet/server_test.go b/meshnet/server_test.go index abdf21ce..e4015ac8 100644 --- a/meshnet/server_test.go +++ b/meshnet/server_test.go @@ -44,6 +44,7 @@ type meshRenewChecker struct { func (m meshRenewChecker) IsLoggedIn() bool { return !m.IsNotLoggedIn } + func (m meshRenewChecker) IsMFAEnabled() (bool, error) { return false, nil } @@ -92,6 +93,10 @@ func (n *workingNetworker) AllowFileshare(address UniqueAddress) error { return nil } +func (n *workingNetworker) PermitFileshare() error { + return nil +} + func (n *workingNetworker) AllowIncoming(address UniqueAddress, lanAllowed bool) error { n.allowedIncoming = append(n.allowedIncoming, allowedIncoming{ address: address, @@ -111,6 +116,10 @@ func (n *workingNetworker) BlockFileshare(address UniqueAddress) error { return nil } +func (n *workingNetworker) ForbidFileshare() error { + return nil +} + func (n *workingNetworker) ResetRouting(changedPeer mesh.MachinePeer, peer mesh.MachinePeers) error { n.resetPeers = append(n.resetPeers, changedPeer.PublicKey) diff --git a/networker/networker.go b/networker/networker.go index fb93a89a..944c2dea 100644 --- a/networker/networker.go +++ b/networker/networker.go @@ -47,6 +47,16 @@ var ( defaultMeshSubnet = netip.MustParsePrefix("100.64.0.0/10") ) +// ErrNoSuchRule is returned when networker tried to remove +// a rule, but such rule does not exist +type ErrNoSuchRule struct { + ruleName string +} + +func (e ErrNoSuchRule) Error() string { + return fmt.Sprintf("allow rule does not exist for %s", e.ruleName) +} + const ( // a string to be prepended with peers public key and appended with peers ip address to form the internal rule name // for allowing the incomig connections @@ -168,7 +178,8 @@ type Combined struct { enableLocalTraffic bool // list with the existing OS interfaces when VPN was connected. // This is used at network changes to know when a new interface was inserted - interfaces mapset.Set[string] + interfaces mapset.Set[string] + isFilesharePermitted bool } // NewCombined returns a ready made version of @@ -1509,6 +1520,11 @@ func (netw *Combined) AllowFileshare(uniqueAddress meshnet.UniqueAddress) error } func (netw *Combined) allowFileshare(publicKey string, address netip.Addr) error { + if !netw.isFilesharePermitted { + log.Println(internal.WarningPrefix, "fileshare is not permitted, can't add allow rules") + return nil + } + ruleName := publicKey + "-allow-fileshare-rule-" + address.String() rules := []firewall.Rule{{ Name: ruleName, @@ -1537,6 +1553,27 @@ func (netw *Combined) allowFileshare(publicKey string, address netip.Addr) error return nil } +func (netw *Combined) PermitFileshare() error { + netw.mu.Lock() + defer netw.mu.Unlock() + if netw.isFilesharePermitted { + return nil + } + netw.isFilesharePermitted = true + return netw.allowFileshareAll() +} + +func (netw *Combined) allowFileshareAll() error { + var allErrors []error + for _, peer := range netw.cfg.Peers { + if peer.DoIAllowFileshare { + err := netw.allowFileshare(peer.PublicKey, peer.Address) + allErrors = append(allErrors, err) + } + } + return errors.Join(allErrors...) +} + func (netw *Combined) undenyDNS() error { ruleName := "deny-private-dns" @@ -1608,7 +1645,15 @@ func (netw *Combined) blockIncoming(uniqueAddress meshnet.UniqueAddress) error { func (netw *Combined) BlockFileshare(uniqueAddress meshnet.UniqueAddress) error { netw.mu.Lock() defer netw.mu.Unlock() - ruleName := uniqueAddress.UID + "-allow-fileshare-rule-" + uniqueAddress.Address.String() + return netw.blockFileshare(uniqueAddress.UID, uniqueAddress.Address) +} + +func (netw *Combined) blockFileshare(publicKey string, address netip.Addr) error { + if !netw.isFilesharePermitted { + log.Println(internal.WarningPrefix, "fileshare is already forbidden") + return nil + } + ruleName := publicKey + "-allow-fileshare-rule-" + address.String() return netw.removeRule(ruleName) } @@ -1616,7 +1661,7 @@ func (netw *Combined) removeRule(ruleName string) error { ruleIndex := slices.Index(netw.rules, ruleName) if ruleIndex == -1 { - return fmt.Errorf("allow rule does not exist for %s", ruleName) + return ErrNoSuchRule{ruleName} } if err := netw.fw.Delete([]string{ruleName}); err != nil { @@ -1627,6 +1672,36 @@ func (netw *Combined) removeRule(ruleName string) error { return nil } +func (netw *Combined) ForbidFileshare() error { + netw.mu.Lock() + defer netw.mu.Unlock() + if !netw.isFilesharePermitted { + return nil + } + + err := netw.blockFileshareAll() + // NOTE: Mark fileshare as forbidden only when there was no error here, so it + // can be tried again. + if err == nil { + netw.isFilesharePermitted = false + } + + return err +} + +func (netw *Combined) blockFileshareAll() error { + var allErrors []error + for _, peer := range netw.cfg.Peers { + err := netw.blockFileshare(peer.PublicKey, peer.Address) + // NOTE: It's fine to have the rule already removed which returns [ErrNoSuchRule]. + // It's not fine to have any other errors, so keep those. + if !errors.Is(err, ErrNoSuchRule{}) { + allErrors = append(allErrors, err) + } + } + return errors.Join(allErrors...) +} + func getHostsFromConfig(peers mesh.MachinePeers) dns.Hosts { hosts := make(dns.Hosts, 0, len(peers)) for _, peer := range peers { diff --git a/networker/networker_test.go b/networker/networker_test.go index 8fffd4ef..fd44a0b7 100644 --- a/networker/networker_test.go +++ b/networker/networker_test.go @@ -1872,7 +1872,19 @@ func TestCombined_Refresh(t *testing.T) { for _, rule := range fw.rules { ruleNames = append(ruleNames, rule.Name) } - assert.Equal(t, 6, len(fw.rules), "%d firewall rules were configured, expected 5, rules content: \n%s", + + // fileshare is forbidden here, so no new fileshare rules were added + assert.Equal(t, 5, len(fw.rules), "%d firewall rules were configured, expected 5, rules content: \n%s", + len(fw.rules), + strings.Join(ruleNames, "\n")) + + // permit fileshare + netw.isFilesharePermitted = true + + netw.Refresh(machineMap) + + // now after refresh, fileshare rules are also added + assert.Equal(t, 6, len(fw.rules), "%d firewall rules were configured, expected 6, rules content: \n%s", len(fw.rules), strings.Join(ruleNames, "\n")) diff --git a/test/qa/lib/fileshare.py b/test/qa/lib/fileshare.py index 737ca9c9..746157b4 100644 --- a/test/qa/lib/fileshare.py +++ b/test/qa/lib/fileshare.py @@ -8,6 +8,8 @@ import pytest import sh +import socket +import os from . import FILE_HASH_UTILITY, logging, ssh @@ -370,3 +372,55 @@ class FileSystemEntity(Enum): def __str__(self): return self.value + + +def bind_port() -> socket.socket | None: + for _ in range(3): + try: + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 0) + sock.bind(('0.0.0.0', 49111)) + sock.listen(1) + logging.log("successfully bound to fileshare port") + return sock + except OSError as e: + logging.log(f"failed to bind to fileshare port: {e}") + time.sleep(1) + return None + + +def port_is_allowed() -> bool: + for _ in range(3): + if is_port_allowed(): + return True + time.sleep(1) + return False + + +def is_port_allowed() -> bool: + rules = os.popen("sudo iptables -S").read() + return "49111 -m comment --comment nordvpn-meshnet -j ACCEPT" in rules + + +def port_is_blocked() -> bool: + for _ in range(3): + if not is_port_allowed(): + return True + time.sleep(1) + return False + + +def ensure_mesh_is_on() -> None: + try: + sh.nordvpn.set.meshnet.on() + except sh.ErrorReturnCode_1 as e: + if "Meshnet is already enabled." not in str(e): + raise e + + +def restart_mesh() -> None: + sh.nordvpn.set.meshnet.off() + time.sleep(2) + sh.nordvpn.set.meshnet.on() + time.sleep(5) + diff --git a/test/qa/test_fileshare.py b/test/qa/test_fileshare.py index 573bbdb6..f26197e2 100644 --- a/test/qa/test_fileshare.py +++ b/test/qa/test_fileshare.py @@ -1347,26 +1347,57 @@ def test_clear(): assert len(lines_outgoing) == 3, str(lines_outgoing) -def test_fileshare_process_monitoring(): - # port is open when fileshare is running - rules = os.popen("sudo iptables -S").read() - assert "49111 -m comment --comment nordvpn-meshnet -j ACCEPT" in rules - - sh.pkill("-SIGKILL", "nordfileshare") - # at the time of writing, the monitoring job is executed periodically every 5 seconds, - # wait for 10 to be sure the job executed - time.sleep(10) - - # port is not allowed when fileshare is down - rules = os.popen("sudo iptables -S").read() - assert "49111 -m comment --comment nordvpn-meshnet -j ACCEPT" not in rules - - os.popen("/usr/lib/nordvpn/nordfileshare &") - time.sleep(10) - - # port is allowed again when fileshare process is up - rules = os.popen("sudo iptables -S").read() - assert "49111 -m comment --comment nordvpn-meshnet -j ACCEPT" in rules +def test_fileshare_process_monitoring_manages_fileshare_rules_on_process_state_changes(): + try: + # port is open when fileshare is running + assert fileshare.port_is_allowed() + + sh.pkill("-SIGKILL", "nordfileshare") + # at the time of writing, the monitoring job is executed periodically every second, + # wait for 2 seconds to be sure the job executed + time.sleep(2) + + # port is not allowed when fileshare is down + assert fileshare.port_is_blocked() + + # restart meshet to get fileshare back up + fileshare.restart_mesh() + + # port is allowed again when fileshare process is up + assert fileshare.port_is_allowed() + finally: # meshnet should be on for most of the tests in this module + fileshare.ensure_mesh_is_on() + + +@pytest.mark.skip(reason="LVPN-6691") +def test_fileshare_process_monitoring_cuts_the_port_access_even_when_it_was_taken_before(): + try: + # stop meshnet to bind to 49111 first + sh.nordvpn.set.meshnet.off() + time.sleep(2) + assert fileshare.port_is_blocked() + + # bind to port before fileshare process starts + sock = fileshare.bind_port() + assert sock is not None + + # start meshnet + sh.nordvpn.set.meshnet.on() # now fileshare tries to start but fails because the port is taken + time.sleep(2) + + # port should not be allowed (fileshare is down) + assert fileshare.port_is_blocked() + + # free the port + sock.close() + + # restart meshnet, now fileshare can start properly + fileshare.restart_mesh() + + # fileshare is up so port is allowed + assert fileshare.port_is_allowed() + finally: # meshnet should be on for most of the tests in this module + fileshare.ensure_mesh_is_on() @pytest.mark.parametrize("background_accept", [True, False], ids=["accept_bg", "accept_int"])