Skip to content

Commit 1bc4eb5

Browse files
deansheatherdeansheather
authored
fix: fix security vulnerabilities reported by CodeQL (coder#5467)
1 parent e359f3c commit 1bc4eb5

File tree

8 files changed

+42
-22
lines changed

8 files changed

+42
-22
lines changed

‎agent/ports_supported.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func (lp *listeningPortsHandler) getListeningPorts() ([]codersdk.ListeningPort,
3232
seen := make(map[uint16]struct{}, len(tabs))
3333
ports := []codersdk.ListeningPort{}
3434
for _, tab := range tabs {
35-
if tab.LocalAddr == nil || tab.LocalAddr.Port < uint16(codersdk.MinimumListeningPort) {
35+
if tab.LocalAddr == nil || tab.LocalAddr.Port < codersdk.MinimumListeningPort {
3636
continue
3737
}
3838

‎cli/clitest/clitest.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"io/ioutil"
99
"os"
1010
"path/filepath"
11+
"strings"
1112
"testing"
1213

1314
"github.com/spf13/cobra"
@@ -55,7 +56,7 @@ func CreateTemplateVersionSource(t *testing.T, responses *echo.Responses) string
5556
directory := t.TempDir()
5657
f, err := ioutil.TempFile(directory, "*.tf")
5758
require.NoError(t, err)
58-
f.Close()
59+
_ = f.Close()
5960
data, err := echo.Tar(responses)
6061
require.NoError(t, err)
6162
extractTar(t, data, directory)
@@ -70,6 +71,9 @@ func extractTar(t *testing.T, data []byte, directory string) {
7071
break
7172
}
7273
require.NoError(t, err)
74+
if header.Name == "." || strings.Contains(header.Name, "..") {
75+
continue
76+
}
7377
// #nosec
7478
path := filepath.Join(directory, header.Name)
7579
mode := header.FileInfo().Mode()

‎cli/server.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -691,16 +691,17 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co
691691
}
692692

693693
client := codersdk.New(localURL)
694-
if cfg.TLS.Enable.Value {
695-
// Secure transport isn't needed for locally communicating!
694+
if localURL.Scheme == "https" && isLocalhost(localURL.Hostname()) {
695+
// The certificate will likely be self-signed or for a different
696+
// hostname, so we need to skip verification.
696697
client.HTTPClient.Transport = &http.Transport{
697698
TLSClientConfig: &tls.Config{
698699
//nolint:gosec
699700
InsecureSkipVerify: true,
700701
},
701702
}
702-
defer client.HTTPClient.CloseIdleConnections()
703703
}
704+
defer client.HTTPClient.CloseIdleConnections()
704705

705706
// This is helpful for tests, but can be silently ignored.
706707
// 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
14041405
if err != nil {
14051406
return "", nil, xerrors.Errorf("read postgres port: %w", err)
14061407
}
1407-
pgPort, err := strconv.Atoi(pgPortRaw)
1408+
pgPort, err := strconv.ParseUint(pgPortRaw, 10, 16)
14081409
if err != nil {
14091410
return "", nil, xerrors.Errorf("parse postgres port: %w", err)
14101411
}
@@ -1465,3 +1466,9 @@ func redirectHTTPToAccessURL(handler http.Handler, accessURL *url.URL) http.Hand
14651466
handler.ServeHTTP(w, r)
14661467
})
14671468
}
1469+
1470+
// isLocalhost returns true if the host points to the local machine. Intended to
1471+
// be called with `u.Hostname()`.
1472+
func isLocalhost(host string) bool {
1473+
return host == "localhost" || host == "127.0.0.1" || host == "::1"
1474+
}

‎coderd/gitauth/oauth.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,13 @@ var scope = map[codersdk.GitProvider][]string{
4545
codersdk.GitProviderGitHub: {"repo", "workflow"},
4646
}
4747

48-
// regex provides defaults for each Git provider to
49-
// match their SaaS host URL. This is configurable by each provider.
48+
// regex provides defaults for each Git provider to match their SaaS host URL.
49+
// This is configurable by each provider.
5050
var regex = map[codersdk.GitProvider]*regexp.Regexp{
51-
codersdk.GitProviderAzureDevops: regexp.MustCompile(`dev\.azure\.com`),
52-
codersdk.GitProviderBitBucket: regexp.MustCompile(`bitbucket\.org`),
53-
codersdk.GitProviderGitLab: regexp.MustCompile(`gitlab\.com`),
54-
codersdk.GitProviderGitHub: regexp.MustCompile(`github\.com`),
51+
codersdk.GitProviderAzureDevops: regexp.MustCompile(`^(https?://)?dev\.azure\.com(/.*)?$`),
52+
codersdk.GitProviderBitBucket: regexp.MustCompile(`^(https?://)?bitbucket\.org(/.*)?$`),
53+
codersdk.GitProviderGitLab: regexp.MustCompile(`^(https?://)?gitlab\.com(/.*)?$`),
54+
codersdk.GitProviderGitHub: regexp.MustCompile(`^(https?://)?github\.com(/.*)?$`),
5555
}
5656

5757
// newJWTOAuthConfig creates a new OAuth2 config that uses a custom

‎coderd/workspaceagents.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -222,11 +222,11 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) {
222222
})
223223
return
224224
}
225-
height, err := strconv.Atoi(r.URL.Query().Get("height"))
225+
height, err := strconv.ParseUint(r.URL.Query().Get("height"), 10, 16)
226226
if err != nil {
227227
height = 80
228228
}
229-
width, err := strconv.Atoi(r.URL.Query().Get("width"))
229+
width, err := strconv.ParseUint(r.URL.Query().Get("width"), 10, 16)
230230
if err != nil {
231231
width = 80
232232
}
@@ -330,7 +330,7 @@ func (api *API) workspaceAgentListeningPorts(rw http.ResponseWriter, r *http.Req
330330
if port == "" {
331331
continue
332332
}
333-
portNum, err := strconv.Atoi(port)
333+
portNum, err := strconv.ParseUint(port, 10, 16)
334334
if err != nil {
335335
continue
336336
}
@@ -344,7 +344,7 @@ func (api *API) workspaceAgentListeningPorts(rw http.ResponseWriter, r *http.Req
344344
// common non-HTTP ports such as databases, FTP, SSH, etc.
345345
filteredPorts := make([]codersdk.ListeningPort, 0, len(portsResponse.Ports))
346346
for _, port := range portsResponse.Ports {
347-
if port.Port < uint16(codersdk.MinimumListeningPort) {
347+
if port.Port < codersdk.MinimumListeningPort {
348348
continue
349349
}
350350
if _, ok := appPorts[port.Port]; ok {

‎codersdk/agentconn.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@ var (
2727
// TailnetIP is a static IPv6 address with the Tailscale prefix that is used to route
2828
// connections from clients to this node. A dynamic address is not required because a Tailnet
2929
// client only dials a single agent at a time.
30-
TailnetIP = netip.MustParseAddr("fd7a:115c:a1e0:49d6:b259:b7ac:b1b2:48f4")
30+
TailnetIP = netip.MustParseAddr("fd7a:115c:a1e0:49d6:b259:b7ac:b1b2:48f4")
31+
)
32+
33+
const (
3134
TailnetSSHPort = 1
3235
TailnetReconnectingPTYPort = 2
3336
TailnetSpeedtestPort = 3
@@ -171,7 +174,7 @@ func (c *AgentConn) ReconnectingPTY(ctx context.Context, id uuid.UUID, height, w
171174
ctx, span := tracing.StartSpan(ctx)
172175
defer span.End()
173176

174-
conn, err := c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, uint16(TailnetReconnectingPTYPort)))
177+
conn, err := c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, TailnetReconnectingPTYPort))
175178
if err != nil {
176179
return nil, err
177180
}
@@ -199,7 +202,7 @@ func (c *AgentConn) ReconnectingPTY(ctx context.Context, id uuid.UUID, height, w
199202
func (c *AgentConn) SSH(ctx context.Context) (net.Conn, error) {
200203
ctx, span := tracing.StartSpan(ctx)
201204
defer span.End()
202-
return c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, uint16(TailnetSSHPort)))
205+
return c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, TailnetSSHPort))
203206
}
204207

205208
// 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) {
226229
func (c *AgentConn) Speedtest(ctx context.Context, direction speedtest.Direction, duration time.Duration) ([]speedtest.Result, error) {
227230
ctx, span := tracing.StartSpan(ctx)
228231
defer span.End()
229-
speedConn, err := c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, uint16(TailnetSpeedtestPort)))
232+
speedConn, err := c.DialContextTCP(ctx, netip.AddrPortFrom(TailnetIP, TailnetSpeedtestPort))
230233
if err != nil {
231234
return nil, xerrors.Errorf("dial speedtest: %w", err)
232235
}
@@ -244,7 +247,7 @@ func (c *AgentConn) DialContext(ctx context.Context, network string, addr string
244247
return nil, xerrors.New("network must be tcp or udp")
245248
}
246249
_, rawPort, _ := net.SplitHostPort(addr)
247-
port, _ := strconv.Atoi(rawPort)
250+
port, _ := strconv.ParseUint(rawPort, 10, 16)
248251
ipp := netip.AddrPortFrom(TailnetIP, uint16(port))
249252
if network == "udp" {
250253
return c.Conn.DialContextUDP(ctx, ipp)
@@ -272,7 +275,7 @@ func (c *AgentConn) statisticsClient() *http.Client {
272275
return nil, xerrors.Errorf("request %q does not appear to be for statistics server", addr)
273276
}
274277

275-
conn, err := c.DialContextTCP(context.Background(), netip.AddrPortFrom(TailnetIP, uint16(TailnetStatisticsPort)))
278+
conn, err := c.DialContextTCP(context.Background(), netip.AddrPortFrom(TailnetIP, TailnetStatisticsPort))
276279
if err != nil {
277280
return nil, xerrors.Errorf("dial statistics: %w", err)
278281
}

‎provisionersdk/archive.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,9 @@ func Untar(directory string, archive []byte) error {
131131
if err != nil {
132132
return err
133133
}
134+
if header.Name == "." || strings.Contains(header.Name, "..") {
135+
continue
136+
}
134137
// #nosec
135138
target := filepath.Join(directory, filepath.FromSlash(header.Name))
136139
switch header.Typeflag {

‎site/site.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,9 @@ func extractBin(dest string, r io.Reader) (numExtracted int, err error) {
611611
}
612612
return n, xerrors.Errorf("read tar archive failed: %w", err)
613613
}
614+
if h.Name == "." || strings.Contains(h.Name, "..") {
615+
continue
616+
}
614617

615618
name := filepath.Join(dest, filepath.Base(h.Name))
616619
f, err := os.Create(name)

0 commit comments

Comments
 (0)