From 1cd5a66575695324b86e0cda6804e573f789a851 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 4 Dec 2023 13:00:13 +0100 Subject: [PATCH 1/3] adding setup key name to the event meta for adding peers by setup key --- management/server/peer.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/management/server/peer.go b/management/server/peer.go index 67076921b..356353db6 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -525,6 +525,7 @@ func (am *DefaultAccountManager) AddPeer(setupKey, userID string, peer *Peer) (* opEvent.InitiatorID = sk.Id opEvent.Activity = activity.PeerAddedWithSetupKey ephemeral = sk.Ephemeral + opEvent.Meta["setup_key_name"] = sk.Name } else { opEvent.InitiatorID = userID opEvent.Activity = activity.PeerAddedByUser @@ -598,7 +599,11 @@ func (am *DefaultAccountManager) AddPeer(setupKey, userID string, peer *Peer) (* } opEvent.TargetID = newPeer.ID - opEvent.Meta = newPeer.EventMeta(am.GetDNSDomain()) + peerMeta := newPeer.EventMeta(am.GetDNSDomain()) + for k, v := range peerMeta { + opEvent.Meta[k] = v + } + am.StoreEvent(opEvent.InitiatorID, opEvent.TargetID, opEvent.AccountID, opEvent.Activity, opEvent.Meta) am.updateAccountPeers(account) From e37a337164cd4fae3498e1b9c15a420f9553529a Mon Sep 17 00:00:00 2001 From: Yury Gargay Date: Mon, 4 Dec 2023 13:34:06 +0100 Subject: [PATCH 2/3] Add gosec linter (#1342) This PR adds `gosec` linter with the following checks disabled: - G102: Bind to all interfaces - G107: Url provided to HTTP request as taint input - G112: Potential slowloris attack - G114: Use of net/http serve function that has no support for setting timeouts - G204: Audit use of command execution - G401: Detect the usage of DES, RC4, MD5 or SHA1 - G402: Look for bad TLS connection settings - G404: Insecure random number source (rand) - G501: Import blocklist: crypto/md5 - G505: Import blocklist: crypto/sha1 We have complaints related to the checks above. They have to be addressed separately. --- .golangci.yaml | 52 ++++++++++++++++--- client/firewall/uspfilter/uspfilter.go | 6 ++- .../internal/routemanager/systemops_linux.go | 5 +- client/ui/client_ui.go | 2 +- iface/tun_linux.go | 3 +- management/server/http/policies_handler.go | 6 ++- management/server/metrics/selfhosted.go | 12 +++-- 7 files changed, 68 insertions(+), 18 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index d847e63c3..757a60a39 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -12,6 +12,44 @@ linters-settings: # Default: false check-type-assertions: false + gosec: + includes: + - G101 # Look for hard coded credentials + #- G102 # Bind to all interfaces + - G103 # Audit the use of unsafe block + - G104 # Audit errors not checked + - G106 # Audit the use of ssh.InsecureIgnoreHostKey + #- G107 # Url provided to HTTP request as taint input + - G108 # Profiling endpoint automatically exposed on /debug/pprof + - G109 # Potential Integer overflow made by strconv.Atoi result conversion to int16/32 + - G110 # Potential DoS vulnerability via decompression bomb + - G111 # Potential directory traversal + #- G112 # Potential slowloris attack + - G113 # Usage of Rat.SetString in math/big with an overflow (CVE-2022-23772) + #- G114 # Use of net/http serve function that has no support for setting timeouts + - G201 # SQL query construction using format string + - G202 # SQL query construction using string concatenation + - G203 # Use of unescaped data in HTML templates + #- G204 # Audit use of command execution + - G301 # Poor file permissions used when creating a directory + - G302 # Poor file permissions used with chmod + - G303 # Creating tempfile using a predictable path + - G304 # File path provided as taint input + - G305 # File traversal when extracting zip/tar archive + - G306 # Poor file permissions used when writing to a new file + - G307 # Poor file permissions used when creating a file with os.Create + #- G401 # Detect the usage of DES, RC4, MD5 or SHA1 + #- G402 # Look for bad TLS connection settings + - G403 # Ensure minimum RSA key length of 2048 bits + #- G404 # Insecure random number source (rand) + #- G501 # Import blocklist: crypto/md5 + - G502 # Import blocklist: crypto/des + - G503 # Import blocklist: crypto/rc4 + - G504 # Import blocklist: net/http/cgi + #- G505 # Import blocklist: crypto/sha1 + - G601 # Implicit memory aliasing of items from a range statement + - G602 # Slice access out of bounds + gocritic: disabled-checks: - commentFormatting @@ -49,6 +87,7 @@ linters: - durationcheck # durationcheck checks for two durations multiplied together - forbidigo # forbidigo forbids identifiers - gocritic # provides diagnostics that check for bugs, performance and style issues + - gosec # inspects source code for security problems - mirror # mirror reports wrong mirror patterns of bytes/strings usage - misspell # misspess finds commonly misspelled English words in comments - nilerr # finds the code that returns nil even if it checks that the error is not nil @@ -65,19 +104,20 @@ issues: exclude-rules: # allow fmt - - path: management/cmd/root.go + - path: management/cmd/root\.go linters: forbidigo - - path: signal/cmd/root.go + - path: signal/cmd/root\.go linters: forbidigo - - path: sharedsock/filter.go + - path: sharedsock/filter\.go linters: - unused - - path: client/firewall/iptables/rule.go + - path: client/firewall/iptables/rule\.go linters: - unused - - path: test.go + - path: test\.go linters: - mirror - - path: mock.go + - gosec + - path: mock\.go linters: - nilnil diff --git a/client/firewall/uspfilter/uspfilter.go b/client/firewall/uspfilter/uspfilter.go index 6fd11e652..7119e791c 100644 --- a/client/firewall/uspfilter/uspfilter.go +++ b/client/firewall/uspfilter/uspfilter.go @@ -355,14 +355,16 @@ func (m *Manager) RemovePacketHook(hookID string) error { for _, arr := range m.incomingRules { for _, r := range arr { if r.id == hookID { - return m.DeleteRule(&r) + rule := r + return m.DeleteRule(&rule) } } } for _, arr := range m.outgoingRules { for _, r := range arr { if r.id == hookID { - return m.DeleteRule(&r) + rule := r + return m.DeleteRule(&rule) } } } diff --git a/client/internal/routemanager/systemops_linux.go b/client/internal/routemanager/systemops_linux.go index b5b4f5696..0562826a5 100644 --- a/client/internal/routemanager/systemops_linux.go +++ b/client/internal/routemanager/systemops_linux.go @@ -107,7 +107,8 @@ loop: break loop case syscall.RTM_NEWROUTE: rt := (*routeInfoInMemory)(unsafe.Pointer(&m.Data[0])) - attrs, err := syscall.ParseNetlinkRouteAttr(&m) + msg := m + attrs, err := syscall.ParseNetlinkRouteAttr(&msg) if err != nil { return nil, err } @@ -146,5 +147,5 @@ func enableIPForwarding() error { return nil } - return os.WriteFile(ipv4ForwardingPath, []byte("1"), 0644) + return os.WriteFile(ipv4ForwardingPath, []byte("1"), 0644) //nolint:gosec } diff --git a/client/ui/client_ui.go b/client/ui/client_ui.go index da230e214..1dc0bb374 100644 --- a/client/ui/client_ui.go +++ b/client/ui/client_ui.go @@ -634,5 +634,5 @@ func checkPIDFile() error { } } - return os.WriteFile(pidFile, []byte(fmt.Sprintf("%d", os.Getpid())), 0o664) + return os.WriteFile(pidFile, []byte(fmt.Sprintf("%d", os.Getpid())), 0o664) //nolint:gosec } diff --git a/iface/tun_linux.go b/iface/tun_linux.go index 93c03436e..1a3537394 100644 --- a/iface/tun_linux.go +++ b/iface/tun_linux.go @@ -99,7 +99,8 @@ func (c *tunDevice) assignAddr() error { } if len(list) > 0 { for _, a := range list { - err = netlink.AddrDel(link, &a) + addr := a + err = netlink.AddrDel(link, &addr) if err != nil { return err } diff --git a/management/server/http/policies_handler.go b/management/server/http/policies_handler.go index f8c876a41..c7b69897a 100644 --- a/management/server/http/policies_handler.go +++ b/management/server/http/policies_handler.go @@ -290,11 +290,13 @@ func toPolicyResponse(account *server.Account, policy *server.Policy) *api.Polic Enabled: policy.Enabled, } for _, r := range policy.Rules { + rID := r.ID + rDescription := r.Description rule := api.PolicyRule{ - Id: &r.ID, + Id: &rID, Name: r.Name, Enabled: r.Enabled, - Description: &r.Description, + Description: &rDescription, Bidirectional: r.Bidirectional, Protocol: api.PolicyRuleProtocol(r.Protocol), Action: api.PolicyRuleAction(r.Action), diff --git a/management/server/metrics/selfhosted.go b/management/server/metrics/selfhosted.go index 90d69b47b..9a8c65b8e 100644 --- a/management/server/metrics/selfhosted.go +++ b/management/server/metrics/selfhosted.go @@ -390,14 +390,18 @@ func getMinMaxVersion(inputList []string) (string, string) { } } } - switch len(versions) { + + targetIndex := 1 + l := len(versions) + + switch l { case 0: return "", "" - case 1: - v := versions[0].String() + case targetIndex: + v := versions[targetIndex-1].String() return v, v default: sort.Sort(version.Collection(versions)) - return versions[0].String(), versions[len(versions)-1].String() + return versions[targetIndex-1].String(), versions[l-1].String() } } From 92adf57fea25258726f75d0b510659def823db52 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 4 Dec 2023 13:49:46 +0100 Subject: [PATCH 3/3] fix map assignment --- management/server/peer.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/management/server/peer.go b/management/server/peer.go index 356353db6..dcfe26410 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -510,6 +510,7 @@ func (am *DefaultAccountManager) AddPeer(setupKey, userID string, peer *Peer) (* } var ephemeral bool + setupKeyName := "" if !addedByUser { // validate the setup key if adding with a key sk, err := account.FindSetupKey(upperKey) @@ -525,7 +526,7 @@ func (am *DefaultAccountManager) AddPeer(setupKey, userID string, peer *Peer) (* opEvent.InitiatorID = sk.Id opEvent.Activity = activity.PeerAddedWithSetupKey ephemeral = sk.Ephemeral - opEvent.Meta["setup_key_name"] = sk.Name + setupKeyName = sk.Name } else { opEvent.InitiatorID = userID opEvent.Activity = activity.PeerAddedByUser @@ -599,9 +600,9 @@ func (am *DefaultAccountManager) AddPeer(setupKey, userID string, peer *Peer) (* } opEvent.TargetID = newPeer.ID - peerMeta := newPeer.EventMeta(am.GetDNSDomain()) - for k, v := range peerMeta { - opEvent.Meta[k] = v + opEvent.Meta = newPeer.EventMeta(am.GetDNSDomain()) + if !addedByUser { + opEvent.Meta["setup_key_name"] = setupKeyName } am.StoreEvent(opEvent.InitiatorID, opEvent.TargetID, opEvent.AccountID, opEvent.Activity, opEvent.Meta)