x509: unhandled critical extension error when using TPM attestation format

[on-keyday]

Version

0.11.0

Description

Thank you for developing this library

My Test Environment:
OS: Windows 11 Home
Version: 23H2
OS Build: 22631.3880
Browser: Brave (Brave 1.68.137 Chromium: 127.0.6533.100 (Official Build) （64 bit)

Actual library version I used is 0.11.1 but Version selection of issue template is not shown so selected 0.11.0.

I encountered below error when using this library at registration logic with Windows Hello.

Unable to validate attestation signature statement during attestation validation: invalid certificate chain from MDS: x509: unhandled critical extension

When I used version 0.10.2 of this library, this error never happened.
I tried using metadata.Provider feature so I guessed it is the reason.
I read some code and found the reason.

The error happens here.

webauthn/protocol/attestation.go

Lines 257 to 259 in cf1758a

	if _, err = x5c.Verify(entry.MetadataStatement.Verifier()); err != nil {
	return ErrInvalidAttestation.WithDetails(fmt.Sprintf("Unable to validate attestation signature statement during attestation validation: invalid certificate chain from MDS: %v", err))
	}

and when I was debugging the code in x509.ParseCertificate in standard library, I reached here unhandled = true
https://github.com/golang/go/blob/72735094660a475a69050b7368c56b25346f5406/src/crypto/x509/parser.go#L691

out.DNSNames, out.EmailAddresses, out.IPAddresses, out.URIs, err = parseSANExtension(e.Value)
if err != nil {
    return err
}

if len(out.DNSNames) == 0 && len(out.EmailAddresses) == 0 && len(out.IPAddresses) == 0 && len(out.URIs) == 0 {
    // If we didn't parse anything then we do the critical check, below.
    unhandled = true
}

after that, reaches here
https://github.com/golang/go/blob/72735094660a475a69050b7368c56b25346f5406/src/crypto/x509/parser.go#L819-L821

if e.Critical && unhandled {
      out.UnhandledCriticalExtensions = append(out.UnhandledCriticalExtensions, e.Id)
}

and then in x5c.Verify finally reaches here
https://github.com/golang/go/blob/72735094660a475a69050b7368c56b25346f5406/src/crypto/x509/verify.go#L564-L566

if len(c.UnhandledCriticalExtensions) > 0 {
    return UnhandledCriticalExtension{}
}

parseSANExtension in standard library is here.
https://github.com/golang/go/blob/72735094660a475a69050b7368c56b25346f5406/src/crypto/x509/parser.go#L374-L417

func parseSANExtension(der cryptobyte.String) (dnsNames, emailAddresses []string, ipAddresses []net.IP, uris []*url.URL, err error) {
	err = forEachSAN(der, func(tag int, data []byte) error {
		switch tag {
		case nameTypeEmail:
			email := string(data)
			if err := isIA5String(email); err != nil {
				return errors.New("x509: SAN rfc822Name is malformed")
			}
			emailAddresses = append(emailAddresses, email)
		case nameTypeDNS:
			name := string(data)
			if err := isIA5String(name); err != nil {
				return errors.New("x509: SAN dNSName is malformed")
			}
			dnsNames = append(dnsNames, string(name))
		case nameTypeURI:
			uriStr := string(data)
			if err := isIA5String(uriStr); err != nil {
				return errors.New("x509: SAN uniformResourceIdentifier is malformed")
			}
			uri, err := url.Parse(uriStr)
			if err != nil {
				return fmt.Errorf("x509: cannot parse URI %q: %s", uriStr, err)
			}
			if len(uri.Host) > 0 {
				if _, ok := domainToReverseLabels(uri.Host); !ok {
					return fmt.Errorf("x509: cannot parse URI %q: invalid domain", uriStr)
				}
			}
			uris = append(uris, uri)
		case nameTypeIP:
			switch len(data) {
			case net.IPv4len, net.IPv6len:
				ipAddresses = append(ipAddresses, data)
			default:
				return errors.New("x509: cannot parse IP address of length " + strconv.Itoa(len(data)))
			}
		}

		return nil
	})

	return
}

By the way, at attestation_tpm.go, custom certificate extension validation logic exists like below.

webauthn/protocol/attestation_tpm.go

Lines 175 to 182 in 9ca2fae

	for _, ext := range aikCert.Extensions {
	if ext.Id.Equal([]int{2, 5, 29, 17}) {
	manufacturer, model, version, err = parseSANExtension(ext.Value)
	if err != nil {
	return "", nil, err
	}
	}
	}

and custom parseSANExtension which is not compatible with standard library

webauthn/protocol/attestation_tpm.go

Lines 301 to 336 in 9ca2fae

	func parseSANExtension(value []byte) (manufacturer string, model string, version string, err error) {
	err = forEachSAN(value, func(tag int, data []byte) error {
	switch tag {
	case nameTypeDN:
	tpmDeviceAttributes := pkix.RDNSequence{}
	_, err := asn1.Unmarshal(data, &tpmDeviceAttributes)
	if err != nil {
	return err
	}
	for _, rdn := range tpmDeviceAttributes {
	if len(rdn) == 0 {
	continue
	}
	for _, atv := range rdn {
	value, ok := atv.Value.(string)
	if !ok {
	continue
	}
	if atv.Type.Equal(tcgAtTpmManufacturer) {
	manufacturer = strings.TrimPrefix(value, "id:")
	}
	if atv.Type.Equal(tcgAtTpmModel) {
	model = value
	}
	if atv.Type.Equal(tcgAtTpmVersion) {
	version = strings.TrimPrefix(value, "id:")
	}
	}
	}
	}
	return nil
	})
	return
	}

that's why this error happens.

Reproduction

static/register.html

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Bug Test</title>
</head>
<body>
    <script type="module">
        
        const base64ToUint8Array = (base64) =>{
            const base64Characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/';
            //const base64URLCharacters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_';

            let cleanedBase64 = String(base64).replace(/-/g, '+').replace(/_/g, '/');
            const padding = (4 - (cleanedBase64.length % 4)) % 4;
            cleanedBase64 += '='.repeat(padding);

            const rawLength = cleanedBase64.length;
            const decodedLength = (rawLength * 3) / 4 - padding;

            const uint8Array = new Uint8Array(decodedLength);

            let byteIndex = 0;
            for (let i = 0; i < rawLength; i += 4) {
                const encoded1 = base64Characters.indexOf(cleanedBase64[i]);
                const encoded2 = base64Characters.indexOf(cleanedBase64[i + 1]);
                const encoded3 = base64Characters.indexOf(cleanedBase64[i + 2]);
                const encoded4 = base64Characters.indexOf(cleanedBase64[i + 3]);

                const decoded1 = (encoded1 << 2) | (encoded2 >> 4);
                const decoded2 = ((encoded2 & 15) << 4) | (encoded3 >> 2);
                const decoded3 = ((encoded3 & 3) << 6) | encoded4;

                uint8Array[byteIndex++] = decoded1;
                if (encoded3 !== 64) uint8Array[byteIndex++] = decoded2;
                if (encoded4 !== 64) uint8Array[byteIndex++] = decoded3;
            }

            return uint8Array;
        }

        const Uint8ArrayToBase64 = (uint8Array) =>{
            //const base64Characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/';
            const base64URLCharacters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_';
            const base64Characters = base64URLCharacters;

            let base64 = '';
            const { length } = uint8Array;

            for (let i = 0; i < length; i += 3) {
                const byte1 = uint8Array[i];
                const byte2 = uint8Array[i + 1];
                const byte3 = uint8Array[i + 2];

                const encoded1 = byte1 >> 2;
                const encoded2 = ((byte1 & 3) << 4) | (byte2 >> 4);
                const encoded3 = ((byte2 & 15) << 2) | (byte3 >> 6);
                const encoded4 = byte3 & 63;

                base64 += base64Characters[encoded1] + base64Characters[encoded2];
                base64 += byte2 !== undefined ? base64Characters[encoded3] : '=';
                base64 += byte3 !== undefined ? base64Characters[encoded4] : '=';
            }

            return base64;
        }

        const j = await fetch("/register").then(r => r.json());
        j.publicKey.challenge = base64ToUint8Array(j.publicKey.challenge).buffer;
        j.publicKey.user.id = base64ToUint8Array(j.publicKey.user.id).buffer;
        console.log(j);
        const r = await navigator.credentials.create({
            publicKey: {
                ...j.publicKey,
               attestation: "direct" 
            }
        });
        console.log(r);
        //r.rawId = Uint8ArrayToBase64(new Uint8Array(r.rawId))
        const js =JSON.stringify({
            id: r.id,
            rawId: Uint8ArrayToBase64(new Uint8Array(r.rawId)),
            response: {
                clientDataJSON: Uint8ArrayToBase64(new Uint8Array(r.response.clientDataJSON)),
                attestationObject: Uint8ArrayToBase64(new Uint8Array(r.response.attestationObject))
            },
            type: r.type
        })
        console.log(js);
        await fetch("/verify", {
            method: "POST",
            headers: {
                "Content-Type": "application/json"
            },
            body: js
        });


    </script>
</body>
</html>

main.go

package main

import (
	_ "embed"
	"encoding/base64"
	"encoding/json"
	"log"
	"net/http"

	"github.com/go-webauthn/webauthn/metadata/providers/cached"
	"github.com/go-webauthn/webauthn/protocol"
	"github.com/go-webauthn/webauthn/webauthn"
)

type user struct{}

func (u *user) WebAuthnID() []byte {
	return []byte("test")
}

func (u *user) WebAuthnName() string {
	return "test"
}

func (u *user) WebAuthnDisplayName() string {
	return "test"
}

func (u *user) WebAuthnIcon() string {
	return ""
}

func (u *user) WebAuthnCredentials() []webauthn.Credential {
	return nil
}

var _ webauthn.User = &user{}

func marshal(v interface{}) string {
	b, err := json.Marshal(v)
	if err != nil {
		panic(err)
	}
	return string(b)
}

func marshalAndBase64(v interface{}) string {
	return base64.StdEncoding.EncodeToString([]byte(marshal(v)))
}

func unmarshal(s string, v interface{}) error {
	return json.Unmarshal([]byte(s), v)
}

func unmarshalFromBase64(s string, v interface{}) error {
	b, err := base64.StdEncoding.DecodeString(s)
	if err != nil {
		return err
	}
	return unmarshal(string(b), v)
}

//go:embed static/register.html
var registerHTML string

func main() {
	v, err := cached.New(cached.WithPath("metadata.json"))
	if err != nil {
		log.Fatal("metadata.json error:", err)
	}
	a, err := webauthn.New(&webauthn.Config{
		RPID: "localhost",
		RPOrigins: []string{
			"http://localhost:8081",
		},
		RPDisplayName: "localhost",
		MDS:           v,
	})
	if err != nil {
		log.Fatal("webauthn error:", err)
	}
	http.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) {
		creation, session, err := a.BeginRegistration(&user{}, webauthn.WithAttestationFormats([]protocol.AttestationFormat{
			protocol.AttestationFormatTPM,
		}))
		if err != nil {
			log.Println(err)
			return
		}
		http.SetCookie(w, &http.Cookie{
			Name:     "session",
			Value:    marshalAndBase64(session),
			SameSite: http.SameSiteStrictMode,
		})
		w.Header().Set("Content-Type", "application/json")
		w.Write([]byte(marshal(creation)))
	})

	http.HandleFunc("/verify", func(w http.ResponseWriter, r *http.Request) {
		session := &webauthn.SessionData{}
		cookie, err := r.Cookie("session")
		if err != nil {
			log.Println(err)
			return
		}
		err = unmarshalFromBase64(cookie.Value, session)
		if err != nil {
			log.Println(err)
			return
		}
		attestation, err := a.FinishRegistration(&user{}, *session, r)
		if err != nil {
			log.Println(err) // error will be printed from here
			return
		}
		w.Header().Set("Content-Type", "application/json")
		w.Write([]byte(marshal(attestation)))
	})

	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("Content-Type", "text/html")
		w.Write([]byte(registerHTML))
	})
        
        log.Println("listening on localhost:8081")
	log.Fatal(http.ListenAndServe("localhost:8081", nil))
}

(sorry, the code may be bit dirty)

1. paste above code into main.go and static/register.html
2. go mod init if at the first time
3. go run .
4. then access to localhost:8081 and do WebAuthn registration
5. the error will be printed on the terminal

Expectations

Currently, I would skip this check by using metadata/providers/memory.WithValidateTrustAnchor(false) option.
But I think this additional validation feature is maybe unnecessary because custom trust anchor verification logics looks already implemented for each attestation type. Otherwise I think verification features should be merged in different way.
(note that this is only my opinion from my narrow (not understanding all of features of this library and not perfectly understanding certificate verification) perspective)

Documentation

No response

[james-d-elliott]

Thanks for reporting, and the very detailed report. This is unlikely to be a bug here but to be an issue with that particular MDS entry. I'll take a closer look soon to make sure. There are narrower expectations of an MDS3 entry than a standard X.509 certificate.

The reason this happens now and not prior is no trust anchor validation was done previously. This was the intent of the major change in 0.11. It's likely disabling that option would work around it.

[james-d-elliott]

Yeah okay so I think it's specifically a bug in the new functionality which is optional. Specifically because the AIK certificate is parsed via stdlib the SAN critical extension isn't parsed since the stdlib ignores the ASN.1 tag that's part of the SAN extension for TPM 2.0 AIK certificates.

Because there is now additional logic to validate the attestation against the MDS now the error occurs. I'll put up a PR for you to test. The idea is that if the certificate is parsed and the type is TPM, and the parsed certificate has not had that critical extension parsed we will reparse it internally to determine if that's actually accurate and update the certificate accordingly.

[james-d-elliott]

I'll have a personal look later but I think github.com/go-webauthn/webauthn@6aeb62ba4e1f542c42e54483b75861fe23585bc0 fixes this.