From c821fc50d9cc4006794eb4540161784d10824076 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Mon, 19 Dec 2022 18:30:22 +0000 Subject: [PATCH 1/2] fix: fix security vulnerabilities reported by CodeQL --- agent/ports_supported.go | 2 +- cli/clitest/clitest.go | 6 +++++- cli/server.go | 15 +++++++++++---- coderd/gitauth/oauth.go | 12 ++++++------ coderd/workspaceagents.go | 8 ++++---- codersdk/agentconn.go | 15 +++++++++------ provisionersdk/archive.go | 3 +++ site/site.go | 3 +++ 8 files changed, 42 insertions(+), 22 deletions(-) diff --git a/agent/ports_supported.go b/agent/ports_supported.go index 0455717b0b4b8..7c9449d1e2400 100644 --- a/agent/ports_supported.go +++ b/agent/ports_supported.go @@ -32,7 +32,7 @@ func (lp *listeningPortsHandler) getListeningPorts() ([]codersdk.ListeningPort, seen := make(map[uint16]struct{}, len(tabs)) ports := []codersdk.ListeningPort{} for _, tab := range tabs { - if tab.LocalAddr == nil || tab.LocalAddr.Port < uint16(codersdk.MinimumListeningPort) { + if tab.LocalAddr == nil || tab.LocalAddr.Port < codersdk.MinimumListeningPort { continue } diff --git a/cli/clitest/clitest.go b/cli/clitest/clitest.go index d2f426793e17f..086ffa7401e87 100644 --- a/cli/clitest/clitest.go +++ b/cli/clitest/clitest.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "os" "path/filepath" + "strings" "testing" "github.com/spf13/cobra" @@ -55,7 +56,7 @@ func CreateTemplateVersionSource(t *testing.T, responses *echo.Responses) string directory := t.TempDir() f, err := ioutil.TempFile(directory, "*.tf") require.NoError(t, err) - f.Close() + _ = f.Close() data, err := echo.Tar(responses) require.NoError(t, err) extractTar(t, data, directory) @@ -70,6 +71,9 @@ func extractTar(t *testing.T, data []byte, directory string) { break } require.NoError(t, err) + if header.Name == "." || strings.Contains(header.Name, "..") { + continue + } // #nosec path := filepath.Join(directory, header.Name) mode := header.FileInfo().Mode() diff --git a/cli/server.go b/cli/server.go index 8825313fa29da..cdb444e1dc02d 100644 --- a/cli/server.go +++ b/cli/server.go @@ -691,16 +691,17 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co } client := codersdk.New(localURL) - if cfg.TLS.Enable.Value { - // Secure transport isn't needed for locally communicating! + if localURL.Scheme == "https" && isLocalhost(localURL.Hostname()) { + // The certificate will likely be self-signed or for a different + // hostname, so we need to skip verification. client.HTTPClient.Transport = &http.Transport{ TLSClientConfig: &tls.Config{ //nolint:gosec InsecureSkipVerify: true, }, } - defer client.HTTPClient.CloseIdleConnections() } + defer client.HTTPClient.CloseIdleConnections() // This is helpful for tests, but can be silently ignored. // Coder may be ran as users that don't have permission to write in the homedir, @@ -1404,7 +1405,7 @@ func startBuiltinPostgres(ctx context.Context, cfg config.Root, logger slog.Logg if err != nil { return "", nil, xerrors.Errorf("read postgres port: %w", err) } - pgPort, err := strconv.Atoi(pgPortRaw) + pgPort, err := strconv.ParseUint(pgPortRaw, 10, 16) if err != nil { return "", nil, xerrors.Errorf("parse postgres port: %w", err) } @@ -1465,3 +1466,9 @@ func redirectHTTPToAccessURL(handler http.Handler, accessURL *url.URL) http.Hand handler.ServeHTTP(w, r) }) } + +// isLocalhost returns true if the host points to the local machine. Intended to +// be called with `u.Hostname()`. +func isLocalhost(host string) bool { + return host == "localhost" || host == "127.0.0.1" || host == "::1" +} diff --git a/coderd/gitauth/oauth.go b/coderd/gitauth/oauth.go index 47a8e94f88803..021ebdf310635 100644 --- a/coderd/gitauth/oauth.go +++ b/coderd/gitauth/oauth.go @@ -45,13 +45,13 @@ var scope = map[codersdk.GitProvider][]string{ codersdk.GitProviderGitHub: {"repo", "workflow"}, } -// regex provides defaults for each Git provider to -// match their SaaS host URL. This is configurable by each provider. +// regex provides defaults for each Git provider to match their SaaS host URL. +// This is configurable by each provider. var regex = map[codersdk.GitProvider]*regexp.Regexp{ - codersdk.GitProviderAzureDevops: regexp.MustCompile(`dev\.azure\.com`), - codersdk.GitProviderBitBucket: regexp.MustCompile(`bitbucket\.org`), - codersdk.GitProviderGitLab: regexp.MustCompile(`gitlab\.com`), - codersdk.GitProviderGitHub: regexp.MustCompile(`github\.com`), + codersdk.GitProviderAzureDevops: regexp.MustCompile(`^(https?:\/\/)?dev\.azure\.com(\/.*)?$`), + codersdk.GitProviderBitBucket: regexp.MustCompile(`^(https?:\/\/)?bitbucket\.org(\/.*)?$`), + codersdk.GitProviderGitLab: regexp.MustCompile(`^(https?:\/\/)?gitlab\.com(\/.*)?$`), + codersdk.GitProviderGitHub: regexp.MustCompile(`^(https?:\/\/)?github\.com(\/.*)?$`), } // newJWTOAuthConfig creates a new OAuth2 config that uses a custom diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index e7dfd62fe64ce..649d1849df2c9 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -222,11 +222,11 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) { }) return } - height, err := strconv.Atoi(r.URL.Query().Get("height")) + height, err := strconv.ParseUint(r.URL.Query().Get("height"), 10, 16) if err != nil { height = 80 } - width, err := strconv.Atoi(r.URL.Query().Get("width")) + width, err := strconv.ParseUint(r.URL.Query().Get("width"), 10, 16) if err != nil { width = 80 } @@ -330,7 +330,7 @@ func (api *API) workspaceAgentListeningPorts(rw http.ResponseWriter, r *http.Req if port == "" { continue } - portNum, err := strconv.Atoi(port) + portNum, err := strconv.ParseUint(port, 10, 16) if err != nil { continue } @@ -344,7 +344,7 @@ func (api *API) workspaceAgentListeningPorts(rw http.ResponseWriter, r *http.Req // common non-HTTP ports such as databases, FTP, SSH, etc. filteredPorts := make([]codersdk.ListeningPort, 0, len(portsResponse.Ports)) for _, port := range portsResponse.Ports { - if port.Port < uint16(codersdk.MinimumListeningPort) { + if port.Port < codersdk.MinimumListeningPort { continue } if _, ok := appPorts[port.Port]; ok { diff --git a/codersdk/agentconn.go b/codersdk/agentconn.go index 33bf16bcc3406..f5439bcdf4cfa 100644 --- a/codersdk/agentconn.go +++ b/codersdk/agentconn.go @@ -27,7 +27,10 @@ var ( // TailnetIP is a static IPv6 address with the Tailscale prefix that is used to route // connections from clients to this node. A dynamic address is not required because a Tailnet // client only dials a single agent at a time. - TailnetIP = netip.MustParseAddr("fd7a:115c:a1e0:49d6:b259:b7ac:b1b2:48f4") + TailnetIP = netip.MustParseAddr("fd7a:115c:a1e0:49d6:b259:b7ac:b1b2:48f4") +) + +const ( TailnetSSHPort = 1 TailnetReconnectingPTYPort = 2 TailnetSpeedtestPort = 3 @@ -171,7 +174,7 @@ func (c *AgentConn) ReconnectingPTY(ctx context.Context, id uuid.UUID, height, w ctx, span := tracing.StartSpan(ctx) defer span.End() - conn, err := c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, uint16(TailnetReconnectingPTYPort))) + conn, err := c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, TailnetReconnectingPTYPort)) if err != nil { return nil, err } @@ -199,7 +202,7 @@ func (c *AgentConn) ReconnectingPTY(ctx context.Context, id uuid.UUID, height, w func (c *AgentConn) SSH(ctx context.Context) (net.Conn, error) { ctx, span := tracing.StartSpan(ctx) defer span.End() - return c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, uint16(TailnetSSHPort))) + return c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, TailnetSSHPort)) } // SSHClient calls SSH to create a client that uses a weak cipher @@ -226,7 +229,7 @@ func (c *AgentConn) SSHClient(ctx context.Context) (*ssh.Client, error) { func (c *AgentConn) Speedtest(ctx context.Context, direction speedtest.Direction, duration time.Duration) ([]speedtest.Result, error) { ctx, span := tracing.StartSpan(ctx) defer span.End() - speedConn, err := c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, uint16(TailnetSpeedtestPort))) + speedConn, err := c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, TailnetSpeedtestPort)) if err != nil { return nil, xerrors.Errorf("dial speedtest: %w", err) } @@ -244,7 +247,7 @@ func (c *AgentConn) DialContext(ctx context.Context, network string, addr string return nil, xerrors.New("network must be tcp or udp") } _, rawPort, _ := net.SplitHostPort(addr) - port, _ := strconv.Atoi(rawPort) + port, _ := strconv.ParseUint(rawPort, 10, 16) ipp := netip.AddrPortFrom(TailnetIP, uint16(port)) if network == "udp" { return c.Conn.DialContextUDP(ctx, ipp) @@ -272,7 +275,7 @@ func (c *AgentConn) statisticsClient() *http.Client { return nil, xerrors.Errorf("request %q does not appear to be for statistics server", addr) } - conn, err := c.DialContextTCP(context.Background(), netip.AddrPortFrom(TailnetIP, uint16(TailnetStatisticsPort))) + conn, err := c.DialContextTCP(context.Background(), netip.AddrPortFrom(TailnetIP, TailnetStatisticsPort)) if err != nil { return nil, xerrors.Errorf("dial statistics: %w", err) } diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go index b4da315e25dc2..e5513d0f6e8b3 100644 --- a/provisionersdk/archive.go +++ b/provisionersdk/archive.go @@ -131,6 +131,9 @@ func Untar(directory string, archive []byte) error { if err != nil { return err } + if header.Name == "." || strings.Contains(header.Name, "..") { + continue + } // #nosec target := filepath.Join(directory, filepath.FromSlash(header.Name)) switch header.Typeflag { diff --git a/site/site.go b/site/site.go index d264071fffb82..884f50a62482c 100644 --- a/site/site.go +++ b/site/site.go @@ -611,6 +611,9 @@ func extractBin(dest string, r io.Reader) (numExtracted int, err error) { } return n, xerrors.Errorf("read tar archive failed: %w", err) } + if h.Name == "." || strings.Contains(h.Name, "..") { + continue + } name := filepath.Join(dest, filepath.Base(h.Name)) f, err := os.Create(name) From 7c42d427e279619f234970a6294e8a7daabc561c Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 20 Dec 2022 05:21:04 +1000 Subject: [PATCH 2/2] Update coderd/gitauth/oauth.go --- coderd/gitauth/oauth.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/gitauth/oauth.go b/coderd/gitauth/oauth.go index 021ebdf310635..c9008dff7697b 100644 --- a/coderd/gitauth/oauth.go +++ b/coderd/gitauth/oauth.go @@ -48,10 +48,10 @@ var scope = map[codersdk.GitProvider][]string{ // regex provides defaults for each Git provider to match their SaaS host URL. // This is configurable by each provider. var regex = map[codersdk.GitProvider]*regexp.Regexp{ - codersdk.GitProviderAzureDevops: regexp.MustCompile(`^(https?:\/\/)?dev\.azure\.com(\/.*)?$`), - codersdk.GitProviderBitBucket: regexp.MustCompile(`^(https?:\/\/)?bitbucket\.org(\/.*)?$`), - codersdk.GitProviderGitLab: regexp.MustCompile(`^(https?:\/\/)?gitlab\.com(\/.*)?$`), - codersdk.GitProviderGitHub: regexp.MustCompile(`^(https?:\/\/)?github\.com(\/.*)?$`), + codersdk.GitProviderAzureDevops: regexp.MustCompile(`^(https?://)?dev\.azure\.com(/.*)?$`), + codersdk.GitProviderBitBucket: regexp.MustCompile(`^(https?://)?bitbucket\.org(/.*)?$`), + codersdk.GitProviderGitLab: regexp.MustCompile(`^(https?://)?gitlab\.com(/.*)?$`), + codersdk.GitProviderGitHub: regexp.MustCompile(`^(https?://)?github\.com(/.*)?$`), } // newJWTOAuthConfig creates a new OAuth2 config that uses a custom