From 099ae4bc6cc8ab95ef16343acb87c33b8197711c Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Fri, 17 Jul 2026 10:10:22 +0200 Subject: [PATCH] [client] Sanitize peer FQDN/hostname in generated SSH config (#6805) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes Validate peer-supplied FQDN and hostname before they are written into the generated NetBird SSH client config (`client/ssh/config/manager.go`). These values originate from remote peers and were previously written verbatim into the config; malformed values (e.g. containing unexpected characters) could produce a broken or unintended config. FQDN/hostname are now checked with `domain.IsValidDomainNoWildcard`, and invalid, non-empty values are dropped with a warning. IPs are unaffected (already validated `netip.Addr`). Added a test covering malformed hostnames. ## Issue ticket number and link Internal input-validation hardening for peer-supplied hostnames in the generated SSH client config (`client/ssh/config/manager.go`). ## Stack - \#6726 - \#6805 :point\_left: ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal client SSH config generation. No public API, gRPC, CLI/service flag, or configuration change — only input validation on peer-supplied hostnames before they are written to the generated ssh\_config. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from here: N/A --- client/ssh/config/manager.go | 14 +++++++++-- client/ssh/config/manager_test.go | 39 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/client/ssh/config/manager.go b/client/ssh/config/manager.go index 20695cb4d..e15330739 100644 --- a/client/ssh/config/manager.go +++ b/client/ssh/config/manager.go @@ -14,6 +14,7 @@ import ( log "github.com/sirupsen/logrus" nbssh "github.com/netbirdio/netbird/client/ssh" + "github.com/netbirdio/netbird/shared/management/domain" ) const ( @@ -218,11 +219,20 @@ func (m *Manager) buildHostPatterns(peer PeerSSHInfo) []string { if peer.IPv6.IsValid() { hostPatterns = append(hostPatterns, peer.IPv6.String()) } - if peer.FQDN != "" { + // Peer FQDNs and hostnames originate from remote peers, so they must be + // validated as plain DNS names before being embedded in the ssh_config + // "Match host" pattern list. This prevents injection of arbitrary + // ssh_config directives via embedded quotes, whitespace, newlines, the + // comma pattern separator, or the "*"/"?" pattern metacharacters. + if domain.IsValidDomainNoWildcard(peer.FQDN) { hostPatterns = append(hostPatterns, peer.FQDN) + } else if peer.FQDN != "" { + log.Warnf("skipping peer FQDN with invalid characters in SSH config: %q", peer.FQDN) } - if peer.Hostname != "" && peer.Hostname != peer.FQDN { + if peer.Hostname != peer.FQDN && domain.IsValidDomainNoWildcard(peer.Hostname) { hostPatterns = append(hostPatterns, peer.Hostname) + } else if peer.Hostname != "" && peer.Hostname != peer.FQDN { + log.Warnf("skipping peer hostname with invalid characters in SSH config: %q", peer.Hostname) } return hostPatterns } diff --git a/client/ssh/config/manager_test.go b/client/ssh/config/manager_test.go index 8e6be40a3..f65d0ba6d 100644 --- a/client/ssh/config/manager_test.go +++ b/client/ssh/config/manager_test.go @@ -148,6 +148,45 @@ func TestManager_MatchHostFormat(t *testing.T) { "should use Match host with comma-separated patterns") } +func TestManager_HostPatternInjection(t *testing.T) { + tempDir, err := os.MkdirTemp("", "netbird-ssh-config-test") + require.NoError(t, err) + defer func() { assert.NoError(t, os.RemoveAll(tempDir)) }() + + manager := &Manager{ + sshConfigDir: filepath.Join(tempDir, "ssh_config.d"), + sshConfigFile: "99-netbird.conf", + } + + // A malicious peer FQDN/hostname attempts to break out of the Match host + // directive and inject arbitrary ssh_config (a ProxyCommand executing a + // command). It must be rejected, not written to the config. + peers := []PeerSSHInfo{ + { + Hostname: "evil\"\n ProxyCommand touch /tmp/pwned\nHost x", + IP: netip.MustParseAddr("100.125.1.1"), + FQDN: "evil\"\n ProxyCommand touch /tmp/pwned\nHost x.nb.internal", + }, + {Hostname: "peer2", IP: netip.MustParseAddr("100.125.1.2"), FQDN: "peer2.nb.internal"}, + } + + err = manager.SetupSSHClientConfig(peers) + require.NoError(t, err) + + configPath := filepath.Join(manager.sshConfigDir, manager.sshConfigFile) + content, err := os.ReadFile(configPath) + require.NoError(t, err) + configStr := string(content) + + assert.NotContains(t, configStr, "ProxyCommand touch /tmp/pwned", + "injected directive must not appear in generated config") + assert.NotContains(t, configStr, "evil", + "malicious pattern must be dropped entirely") + // The valid peer must still be present, on a single Match host line. + assert.Contains(t, configStr, "Match host \"100.125.1.1,100.125.1.2,peer2.nb.internal,peer2\"", + "valid peers must survive, injected patterns dropped") +} + func TestManager_ForcedSSHConfig(t *testing.T) { // Set force environment variable t.Setenv(EnvForceSSHConfig, "true")