MFA Fuzzing and Json.Unmarshal Panic fix

fuzz/oss-fuzz-build.sh

@@ -123,7 +123,10 @@ build_teleport_fuzzers() {
     FuzzParseU2FRegistrationResponse fuzz_parse_u2f_registration_response
 
   compile_native_go_fuzzer $TELEPORT_PREFIX/lib/web \
-    FuzzTdpMFACodecDecode fuzz_tdp_mfa_codec_decode
+    FuzzTdpMFACodecDecodeChallenge fuzz_tdp_mfa_codec_decode_challenge
+
+  compile_native_go_fuzzer $TELEPORT_PREFIX/lib/web \
+    FuzzTdpMFACodecDecodeResponse fuzz_tdp_mfa_codec_decode_response
 
   compile_native_go_fuzzer $TELEPORT_PREFIX/lib/multiplexer \
     FuzzReadProxyLineV1 fuzz_read_proxy_linec_v1

lib/auth/http_client.go

@@ -780,7 +780,7 @@ func (c *HTTPClient) ValidateOIDCAuthCallback(ctx context.Context, q url.Values)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
-	var rawResponse *OIDCAuthRawResponse
+	var rawResponse OIDCAuthRawResponse
 	if err := json.Unmarshal(out.Bytes(), &rawResponse); err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -818,7 +818,7 @@ func (c *HTTPClient) ValidateSAMLResponse(ctx context.Context, re string, connec
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
-	var rawResponse *SAMLAuthRawResponse
+	var rawResponse SAMLAuthRawResponse
 	if err := json.Unmarshal(out.Bytes(), &rawResponse); err != nil {
 		return nil, trace.Wrap(err)
 	}

lib/auth/trustedcluster.go

@@ -654,7 +654,7 @@ func (a *Server) sendValidateRequestToProxy(host string, validateRequest *Valida
 		return nil, trace.Wrap(err)
 	}
 
-	var validateResponseRaw *ValidateTrustedClusterResponseRaw
+	var validateResponseRaw ValidateTrustedClusterResponseRaw
 	err = json.Unmarshal(out.Bytes(), &validateResponseRaw)
 	if err != nil {
 		return nil, trace.Wrap(err)

lib/client/redirect.go

@@ -204,13 +204,13 @@ func (rd *Redirector) issueSSOLoginConsoleRequest(req SSOLoginConsoleReq) (*SSOL
 		return nil, trace.Wrap(err)
 	}
 
-	var re *SSOLoginConsoleResponse
+	var re SSOLoginConsoleResponse
 	err = json.Unmarshal(out.Bytes(), &re)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
 
-	return re, nil
+	return &re, nil
 }
 
 // Done is called when redirector is closed
@@ -255,13 +255,13 @@ func (rd *Redirector) callback(w http.ResponseWriter, r *http.Request) (*auth.SS
 		return nil, trace.BadParameter("failed to decrypt response: in %v, err: %v", r.URL.String(), err)
 	}
 
-	var re *auth.SSHLoginResponse
+	var re auth.SSHLoginResponse
 	err = json.Unmarshal(plaintext, &re)
 	if err != nil {
 		return nil, trace.BadParameter("failed to decrypt response: in %v, err: %v", r.URL.String(), err)
 	}
 
-	return re, nil
+	return &re, nil
 }
 
 // Close closes redirector and releases all resources

lib/client/weblogin.go

@@ -464,13 +464,13 @@ func SSHAgentLogin(ctx context.Context, login SSHLoginDirect) (*auth.SSHLoginRes
 		return nil, trace.Wrap(err)
 	}
 
-	var out *auth.SSHLoginResponse
+	var out auth.SSHLoginResponse
 	err = json.Unmarshal(re.Bytes(), &out)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
 
-	return out, nil
+	return &out, nil
 }
 
 // SSHAgentHeadlessLogin begins the headless login ceremony, returning new user certificates if successful.
@@ -497,13 +497,13 @@ func SSHAgentHeadlessLogin(ctx context.Context, login SSHLoginHeadless) (*auth.S
 		return nil, trace.Wrap(err)
 	}
 
-	var out *auth.SSHLoginResponse
+	var out auth.SSHLoginResponse
 	err = json.Unmarshal(re.Bytes(), &out)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
 
-	return out, nil
+	return &out, nil
 }
 
 // SSHAgentPasswordlessLogin requests a passwordless MFA challenge via the proxy.

lib/srv/desktop/tdp/proto.go

@@ -596,6 +596,8 @@ func DecodeMFA(in byteReader) (*MFA, error) {
 	if length > maxMFADataLength {
 		_, _ = io.CopyN(io.Discard, in, int64(length))
 		return nil, mfaDataMaxLenErr
+	} else if length == 0 {
+		return nil, trace.BadParameter("mfa data missing")
 	}
 
 	b := make([]byte, int(length))
@@ -636,24 +638,23 @@ func DecodeMFAChallenge(in byteReader) (*MFA, error) {
 
 	if length > maxMFADataLength {
 		return nil, trace.BadParameter("mfa challenge data exceeds maximum length")
+	} else if length == 0 {
+		return nil, trace.BadParameter("mfa challenge data missing")
 	}
 
 	b := make([]byte, int(length))
 	if _, err := io.ReadFull(in, b); err != nil {
 		return nil, trace.Wrap(err)
 	}
 
-	var req *client.MFAAuthenticateChallenge
+	var req client.MFAAuthenticateChallenge
 	if err := json.Unmarshal(b, &req); err != nil {
 		return nil, trace.Wrap(err)
 	}
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
 
 	return &MFA{
 		Type:                     mt,
-		MFAAuthenticateChallenge: req,
+		MFAAuthenticateChallenge: &req,
 	}, nil
 }
 

lib/web/apiserver.go

@@ -2800,7 +2800,7 @@ func (h *Handler) siteNodeConnect(
 	if params == "" {
 		return nil, trace.BadParameter("missing params")
 	}
-	var req *TerminalRequest
+	var req TerminalRequest
 	if err := json.Unmarshal([]byte(params), &req); err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -2824,13 +2824,13 @@ func (h *Handler) siteNodeConnect(
 	clusterName := site.GetName()
 	if req.SessionID.IsZero() {
 		// An existing session ID was not provided so we need to create a new one.
-		sessionData, err = h.generateSession(ctx, clt, req, clusterName, sessionCtx)
+		sessionData, err = h.generateSession(ctx, clt, &req, clusterName, sessionCtx)
 		if err != nil {
 			h.log.WithError(err).Debug("Unable to generate new ssh session.")
 			return nil, trace.Wrap(err)
 		}
 	} else {
-		sessionData, tracker, err = h.fetchExistingSession(ctx, clt, req, clusterName)
+		sessionData, tracker, err = h.fetchExistingSession(ctx, clt, &req, clusterName)
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}

lib/web/command.go

@@ -134,7 +134,7 @@ func (h *Handler) executeCommand(
 	if params == "" {
 		return nil, trace.BadParameter("missing params")
 	}
-	var req *CommandRequest
+	var req CommandRequest
 	if err := json.Unmarshal([]byte(params), &req); err != nil {
 		return nil, trace.BadParameter("failed to read JSON message: %v", err)
 	}

lib/web/fuzz_test.go

@@ -17,13 +17,87 @@ limitations under the License.
 package web
 
 import (
+	"bytes"
+	"encoding/binary"
+	"encoding/json"
+	"math"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
+	apiproto "github.com/gravitational/teleport/api/client/proto"
+	wanpb "github.com/gravitational/teleport/api/types/webauthn"
+	"github.com/gravitational/teleport/lib/defaults"
+	"github.com/gravitational/teleport/lib/srv/desktop/tdp"
 )
 
-func FuzzTdpMFACodecDecode(f *testing.F) {
-	f.Add([]byte(""))
+func FuzzTdpMFACodecDecodeChallenge(f *testing.F) {
+	allowedCreds := wanpb.CredentialDescriptor{
+		Type: "public-key",
+		Id:   []byte{0x02, 0x02, 0x02, 0x02},
+	}
+	extensions := wanpb.AuthenticationExtensionsClientInputs{AppId: "id"}
+	jsonData, err := json.Marshal(&apiproto.MFAAuthenticateChallenge{
+		WebauthnChallenge: &wanpb.CredentialAssertion{
+			PublicKey: &wanpb.PublicKeyCredentialRequestOptions{
+				Challenge:        []byte{0xAA, 0xAA, 0xAA, 0xAA},
+				TimeoutMs:        int64(120),
+				RpId:             "RelyingPartyID",
+				AllowCredentials: []*wanpb.CredentialDescriptor{&allowedCreds},
+				Extensions:       &extensions,
+				UserVerification: "verification",
+			},
+		},
+	})
+	require.NoError(f, err)
+	var normalBuf bytes.Buffer
+	var maxSizeBuf bytes.Buffer
+	// add initial bytes for protocol
+	_, err = normalBuf.Write([]byte{byte(tdp.TypeMFA), []byte(defaults.WebsocketWebauthnChallenge)[0]})
+	require.NoError(f, err)
+	_, err = maxSizeBuf.Write([]byte{byte(tdp.TypeMFA), []byte(defaults.WebsocketWebauthnChallenge)[0]})
+	require.NoError(f, err)
+	// Write the length using BigEndian encoding
+	require.NoError(f, binary.Write(&normalBuf, binary.BigEndian, uint32(len(jsonData))))
+	require.NoError(f, binary.Write(&maxSizeBuf, binary.BigEndian, uint32(math.MaxUint32)))
+	// Write the JSON data itself
+	_, err = normalBuf.Write(jsonData)
+	require.NoError(f, err)
+	_, err = maxSizeBuf.Write(jsonData)
+	require.NoError(f, err)
+
+	f.Add(normalBuf.Bytes())
+	f.Add(maxSizeBuf.Bytes())
+	f.Add([]byte{0xa, 0x6e, 0x0, 0x0, 0x0, 0x4, 0x6e, 0x75, 0x6c, 0x6c}) // nil json unmarshal without error
+
+	f.Fuzz(func(t *testing.T, buf []byte) {
+		require.NotPanics(t, func() {
+			codec := tdpMFACodec{}
+			_, _ = codec.decodeChallenge(buf, "")
+		})
+	})
+}
+
+func FuzzTdpMFACodecDecodeResponse(f *testing.F) {
+	var normalBuf bytes.Buffer
+	var maxSizeBuf bytes.Buffer
+	// add initial bytes for protocol
+	_, err := normalBuf.Write([]byte{byte(tdp.TypeMFA), []byte(defaults.WebsocketWebauthnChallenge)[0]})
+	require.NoError(f, err)
+	_, err = maxSizeBuf.Write([]byte{byte(tdp.TypeMFA), []byte(defaults.WebsocketWebauthnChallenge)[0]})
+	require.NoError(f, err)
+	mfaData := []byte("fake-data")
+	// Write the length using BigEndian encoding
+	require.NoError(f, binary.Write(&normalBuf, binary.BigEndian, uint32(len(mfaData))))
+	require.NoError(f, binary.Write(&maxSizeBuf, binary.BigEndian, uint32(math.MaxUint32)))
+	// add data field
+	_, err = normalBuf.Write(mfaData)
+	require.NoError(f, err)
+	_, err = maxSizeBuf.Write(mfaData)
+	require.NoError(f, err)
+
+	f.Add(normalBuf.Bytes())
+	f.Add(maxSizeBuf.Bytes())
 
 	f.Fuzz(func(t *testing.T, buf []byte) {
 		require.NotPanics(t, func() {

lib/web/session/cookie.go

@@ -46,11 +46,11 @@ func DecodeCookie(b string) (*Cookie, error) {
 	if err != nil {
 		return nil, err
 	}
-	var c *Cookie
+	var c Cookie
 	if err := json.Unmarshal(bytes, &c); err != nil {
 		return nil, err
 	}
-	return c, nil
+	return &c, nil
 }
 
 // SetCookie encodes the provided user and session id via [EncodeCookie]