(v9) Security fixes

api/client/client.go

@@ -746,14 +746,22 @@ func (c *Client) GetBotUsers(ctx context.Context) ([]types.User, error) {
 
 // GetAccessRequests retrieves a list of all access requests matching the provided filter.
 func (c *Client) GetAccessRequests(ctx context.Context, filter types.AccessRequestFilter) ([]types.AccessRequest, error) {
-	rsp, err := c.grpc.GetAccessRequests(ctx, &filter, c.callOpts...)
+	stream, err := c.grpc.GetAccessRequestsV2(ctx, &filter, c.callOpts...)
 	if err != nil {
 		return nil, trail.FromGRPC(err)
 	}
-	reqs := make([]types.AccessRequest, 0, len(rsp.AccessRequests))
-	for _, req := range rsp.AccessRequests {
+	var reqs []types.AccessRequest
+	for {
+		req, err := stream.Recv()
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			return nil, trail.FromGRPC(err)
+		}
 		reqs = append(reqs, req)
 	}
+
 	return reqs, nil
 }
 

api/client/proto/authservice.proto

@@ -1718,7 +1718,10 @@ service AuthService {
     rpc IsMFARequired(IsMFARequiredRequest) returns (IsMFARequiredResponse);
 
     // GetAccessRequests gets all pending access requests.
+    // DEPRECATED, DELETE IN 11.0.0: Use GetAccessRequestsV2 instead.
     rpc GetAccessRequests(types.AccessRequestFilter) returns (AccessRequests);
+    // GetAccessRequestsV2 gets all pending access requests.
+    rpc GetAccessRequestsV2(types.AccessRequestFilter) returns (stream types.AccessRequestV3);
     // CreateAccessRequest creates a new access request.
     rpc CreateAccessRequest(types.AccessRequestV3) returns (google.protobuf.Empty);
     // DeleteAccessRequest deletes an access request.

integration/agent_forwarding_test.go

@@ -0,0 +1,82 @@
+/*
+Copyright 2022 Gravitational, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package integration
+
+import (
+	"os"
+	"os/user"
+	"runtime"
+	"syscall"
+	"testing"
+
+	"github.com/gravitational/teleport/lib/teleagent"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/unix"
+)
+
+func TestAgentSocketPermissions(t *testing.T) {
+	if !isRoot() {
+		t.Skip("This test will be skipped because tests are not being run as root.")
+	}
+
+	agentServer := teleagent.NewServer(nil)
+
+	agentServer.SetTestPermissions(func() {
+		// ListenUnixSocket should not have its uid changed from root
+		require.True(t, isRoot())
+
+		done := make(chan struct{})
+
+		// Start goroutine to attempt privilege escalation during
+		// permission updates on the unix socket.
+		//
+		// For each step of permission updating, it should be impossible
+		// for the user to unlink/remove the socket. If they can unlink
+		// or remove the socket, then it could be replaced with a symlink
+		// which can be used to acquire the permissions of the original socket.
+		go func() {
+			defer close(done)
+
+			// Update uid to nonroot
+			_, _, serr := syscall.Syscall(syscall.SYS_SETUID, 1000, 0, 0)
+			require.Zero(t, serr)
+			require.True(t, !isRoot())
+
+			err := unix.Unlink(agentServer.Path)
+			require.Error(t, err)
+			err = os.Remove(agentServer.Path)
+			require.Error(t, err)
+			err = os.Rename(agentServer.Path, agentServer.Path)
+			require.Error(t, err)
+		}()
+		<-done
+
+		// ListenUnixSocket should not have its uid changed from root
+		require.True(t, isRoot())
+	})
+
+	nonRoot, err := user.LookupId("1000")
+	require.NoError(t, err)
+
+	// lock goroutine to root so that ListenUnixSocket doesn't
+	// pick up the syscall in the testPermissions func
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
+
+	err = agentServer.ListenUnixSocket("test", "sock.agent", nonRoot)
+	require.NoError(t, err)
+}

integration/app_integration_test.go

@@ -1288,6 +1288,7 @@ func (p *pack) makeWebapiRequest(method, endpoint string, payload []byte) (int,
 		Value: p.webCookie,
 	})
 	req.Header.Add("Authorization", fmt.Sprintf("Bearer %v", p.webToken))
+	req.Header.Add("Content-Type", "application/json")
 
 	statusCode, body, err := p.sendRequest(req, nil)
 	return statusCode, []byte(body), trace.Wrap(err)

integration/helpers.go

@@ -1695,20 +1695,8 @@ func externalSSHCommand(o commandOptions) (*exec.Cmd, error) {
 // clobber your system agent.
 func createAgent(me *user.User, privateKeyByte []byte, certificateBytes []byte) (*teleagent.AgentServer, string, string, error) {
 	// create a path to the unix socket
-	sockDir, err := ioutil.TempDir("", "int-test")
-	if err != nil {
-		return nil, "", "", trace.Wrap(err)
-	}
-	sockPath := filepath.Join(sockDir, "agent.sock")
-
-	uid, err := strconv.Atoi(me.Uid)
-	if err != nil {
-		return nil, "", "", trace.Wrap(err)
-	}
-	gid, err := strconv.Atoi(me.Gid)
-	if err != nil {
-		return nil, "", "", trace.Wrap(err)
-	}
+	sockDirName := "int-test"
+	sockName := "agent.sock"
 
 	// transform the key and certificate bytes into something the agent can understand
 	publicKey, _, _, _, err := ssh.ParseAuthorizedKey(certificateBytes)
@@ -1737,13 +1725,13 @@ func createAgent(me *user.User, privateKeyByte []byte, certificateBytes []byte)
 	})
 
 	// start the SSH agent
-	err = teleAgent.ListenUnixSocket(sockPath, uid, gid, 0600)
+	err = teleAgent.ListenUnixSocket(sockDirName, sockName, me)
 	if err != nil {
 		return nil, "", "", trace.Wrap(err)
 	}
 	go teleAgent.Serve()
 
-	return teleAgent, sockDir, sockPath, nil
+	return teleAgent, teleAgent.Dir, teleAgent.Path, nil
 }
 
 func closeAgent(teleAgent *teleagent.AgentServer, socketDirPath string) error {

lib/auth/apiserver_test.go

@@ -117,6 +117,7 @@ func TestUpsertServer(t *testing.T) {
 			require.NoError(t, err)
 			req := httptest.NewRequest(http.MethodPost, "http://localhost", bytes.NewReader(body))
 			req.RemoteAddr = remoteAddr
+			req.Header.Add("Content-Type", "application/json")
 
 			_, err = new(APIServer).upsertServer(s, tt.role, req, httprouter.Params{httprouter.Param{Key: "namespace", Value: apidefaults.Namespace}})
 			tt.assertErr(t, err)

lib/auth/grpcserver.go

@@ -514,6 +514,7 @@ func (g *GRPCServer) GetUsers(req *proto.GetUsersRequest, stream proto.AuthServi
 	return nil
 }
 
+// DEPRECATED, DELETE IN 11.0.0: Use GetAccessRequestsV2 instead.
 func (g *GRPCServer) GetAccessRequests(ctx context.Context, f *types.AccessRequestFilter) (*proto.AccessRequests, error) {
 	auth, err := g.authenticate(ctx)
 	if err != nil {
@@ -541,6 +542,34 @@ func (g *GRPCServer) GetAccessRequests(ctx context.Context, f *types.AccessReque
 	}, nil
 }
 
+func (g *GRPCServer) GetAccessRequestsV2(f *types.AccessRequestFilter, stream proto.AuthService_GetAccessRequestsV2Server) error {
+	ctx := stream.Context()
+	auth, err := g.authenticate(ctx)
+	if err != nil {
+		return trace.Wrap(err)
+	}
+	var filter types.AccessRequestFilter
+	if f != nil {
+		filter = *f
+	}
+	reqs, err := auth.ServerWithRoles.GetAccessRequests(ctx, filter)
+	if err != nil {
+		return trace.Wrap(err)
+	}
+	for _, req := range reqs {
+		r, ok := req.(*types.AccessRequestV3)
+		if !ok {
+			err = trace.BadParameter("unexpected access request type %T", req)
+			return trace.Wrap(err)
+		}
+
+		if err := stream.Send(r); err != nil {
+			return trace.Wrap(err)
+		}
+	}
+	return nil
+}
+
 func (g *GRPCServer) CreateAccessRequest(ctx context.Context, req *types.AccessRequestV3) (*empty.Empty, error) {
 	auth, err := g.authenticate(ctx)
 	if err != nil {

lib/httplib/httplib.go

@@ -21,6 +21,7 @@ package httplib
 import (
 	"encoding/json"
 	"errors"
+	"mime"
 	"net"
 	"net/http"
 	"net/url"
@@ -115,6 +116,18 @@ func WithCSRFProtection(fn HandlerFunc) httprouter.Handle {
 // ReadJSON reads HTTP json request and unmarshals it
 // into passed interface{} obj
 func ReadJSON(r *http.Request, val interface{}) error {
+	// Check content type to mitigate CSRF attack.
+	contentType, _, err := mime.ParseMediaType(r.Header.Get("Content-Type"))
+	if err != nil {
+		log.Warningf("Error parsing media type for reading JSON: %v", err)
+		return trace.BadParameter("invalid request")
+	}
+
+	if contentType != "application/json" {
+		log.Warningf("Invalid HTTP request header content-type %q for reading JSON", contentType)
+		return trace.BadParameter("invalid request")
+	}
+
 	data, err := utils.ReadAtMost(r.Body, teleport.MaxHTTPRequestSize)
 	if err != nil {
 		return trace.Wrap(err)

lib/httplib/httplib_test.go

@@ -17,11 +17,15 @@ limitations under the License.
 package httplib
 
 import (
+	"bytes"
+	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"github.com/julienschmidt/httprouter"
+	"github.com/stretchr/testify/require"
 	. "gopkg.in/check.v1"
 )
 
@@ -77,3 +81,59 @@ func (h *testHandler) postSessionChunkNamespace(w http.ResponseWriter, r *http.R
 	h.capturedID = p.ByName("id")
 	return "ok", nil
 }
+
+func TestReadJSON_ContentType(t *testing.T) {
+	t.Parallel()
+
+	type TestJSON struct {
+		Name string `json:"name"`
+		Age  int    `json:"age"`
+	}
+
+	testCases := []struct {
+		name        string
+		contentType string
+		wantErr     bool
+	}{
+		{
+			name:        "empty value",
+			contentType: "",
+			wantErr:     true,
+		},
+		{
+			name:        "invalid type",
+			contentType: "multipart/form-data",
+			wantErr:     true,
+		},
+		{
+			name:        "just type/subtype",
+			contentType: "application/json",
+		},
+		{
+			name:        "type/subtype with params",
+			contentType: "application/json; charset=utf-8",
+		},
+	}
+
+	body := TestJSON{Name: "foo", Age: 60}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			payloadBuf := new(bytes.Buffer)
+			require.NoError(t, json.NewEncoder(payloadBuf).Encode(body))
+
+			httpReq, err := http.NewRequest("", "", payloadBuf)
+			require.NoError(t, err)
+			httpReq.Header.Add("Content-Type", tc.contentType)
+
+			output := TestJSON{}
+			err = ReadJSON(httpReq, &output)
+			if tc.wantErr {
+				require.True(t, strings.Contains(err.Error(), "invalid request"))
+				require.Empty(t, output)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, body, output)
+			}
+		})
+	}
+}

lib/services/access_request.go

@@ -32,6 +32,8 @@ import (
 	"github.com/vulcand/predicate"
 )
 
+const maxAccessRequestReasonSize = 4096
+
 // ValidateAccessRequest validates the AccessRequest and sets default values
 func ValidateAccessRequest(ar types.AccessRequest) error {
 	if err := ar.CheckAndSetDefaults(); err != nil {
@@ -41,6 +43,12 @@ func ValidateAccessRequest(ar types.AccessRequest) error {
 	if err != nil {
 		return trace.BadParameter("invalid access request id %q", ar.GetName())
 	}
+	if len(ar.GetRequestReason()) > maxAccessRequestReasonSize {
+		return trace.BadParameter("access request reason is too long, max %v bytes", maxAccessRequestReasonSize)
+	}
+	if len(ar.GetResolveReason()) > maxAccessRequestReasonSize {
+		return trace.BadParameter("access request resolve reason is too long, max %v bytes", maxAccessRequestReasonSize)
+	}
 	return nil
 }
 

lib/services/access_request_test.go

@@ -519,6 +519,20 @@ func TestReviewThresholds(t *testing.T) {
 	}
 }
 
+// TestMaxLength tests that we reject too large access requests.
+func TestMaxLength(t *testing.T) {
+	req, err := types.NewAccessRequest("some-id", "dave", "dictator", "never")
+	require.NoError(t, err)
+
+	var s []byte
+	for i := 0; i <= maxAccessRequestReasonSize; i++ {
+		s = append(s, 'a')
+	}
+
+	req.SetRequestReason(string(s))
+	require.Error(t, ValidateAccessRequest(req))
+}
+
 // TestThresholdReviewFilter verifies basic filter syntax.
 func TestThresholdReviewFilter(t *testing.T) {