Skip to content

fix: fix security vulnerabilities reported by CodeQL #5467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion agent/ports_supported.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
6 changes: 5 additions & 1 deletion cli/clitest/clitest.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
"strings"
"testing"

"github.com/spf13/cobra"
Expand Down Expand Up @@ -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)
Expand All @@ -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()
Expand Down
15 changes: 11 additions & 4 deletions cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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"
}
12 changes: 6 additions & 6 deletions coderd/gitauth/oauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions coderd/workspaceagents.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down
15 changes: 9 additions & 6 deletions codersdk/agentconn.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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)
}
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down
3 changes: 3 additions & 0 deletions provisionersdk/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions site/site.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down