madwizard-org / webauthn-server

WebAuthn Relying Party server library for PHP
MIT License
51 stars 12 forks source link

TPM attestation parser does not correctly handle ECC keys in pubArea #17

Closed aseigler closed 1 year ago

aseigler commented 2 years ago

When parsing the unique field from pubArea during an attestation verification, unique is a TPM2B_PUBLIC_KEY_RSA only if the TPMI_ALG_PUBLIC is TPM_ALG_RSA. If TPMI_ALG_PUBLIC is TPM_ALG_ECC, unique is a TPMS_ECC_POINT. See TPM Rev 2.0 part 2, structures, section 12.2.3.2.

Relevant code is at:

https://github.com/madwizard-org/webauthn-server/blob/3a2451a38a883e2f3793e3cbb786bf87cbf1f0d5/src/Attestation/Tpm/TpmPublic.php#L82

madwizard-thomas commented 2 years ago

Hi, thanks for your report! The unique field was meant to be an opaque byte array in this class and is parsed diffidently depending on the type later, but I guess there's no guarantee the field is a length prefixed structure?

Do you have any ECC TPM test vectors available I can use in tests? Examples of TPM attestations were very hard to find when I wrote this.

aseigler commented 2 years ago

Examples of TPM attestations were VERY hard to find then, I had the same problem, and ECC examples simply didn't exist until a few weeks ago. Here's a sample:

{
    "id":"BoLAd0jIDI0ztrH1N45XQ_0w_N5ndt3hpNixQi3J2No",
    "rawId":"BoLAd0jIDI0ztrH1N45XQ_0w_N5ndt3hpNixQi3J2No",
    "response":{
    "attestationObject":"o2NmbXRjdHBtZ2F0dFN0bXSmY2FsZzn__mNzaWdZAQAzaz3HmrpCUlkEV2iv-TF2_y0MD7MVc0rLyuD_Ah3X9vx3G21WgeI89PyyvEYw3yEUUdO7sn6YxubMfuePpuSawYKAeSbw3O4LkMDC2fqZmlLyTfoC8L1_8vExv6mWPN7H5U6E_K7IZ38H3mO736ie-mDyoXxalj4WkA9zjKXJM5t7GhHQAqtDaX4HmM47pFH25atgQnoLdB0MTzh6jgYjIiDrMSOqhrQYskiaX_LFfKTiWfviwMOYcMA8FkRPc05LKvPTxp-bx_ghHrd_gIAUA3MjfElVYCVfveMnI61ZwARnf0cTrFp7vfga85YeAXaLOu29JifjodW6DsjL_dnXY3ZlcmMyLjBjeDVjglkFtTCCBbEwggOZoAMCAQICEAaSyUKea0mgpfZbwvZ7byMwDQYJKoZIhvcNAQELBQAwQTE_MD0GA1UEAxM2RVVTLU5UQy1LRVlJRC0yM0Y0RTIyQUQzQkUzNzRBNDQ5NzcyOTU0QUEyODNBRUQ3NTI1NzJFMB4XDTIxMTEyNTIxMzA1NFoXDTI3MDYwMzE3NTE0N1owADCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANwiGFmQdIOYto4qGegANWT-LdSr5T5_tj7E_aKtLSNP8bqc6eP11VvCi9ZFnbjiFxi1NdY2GAbUDb3zr1PnZpOcwvn1gh704PLtkZYFkwvFRvm5bIvtsuqYgn71MCup1GCTeJ3EcylidbVpmwX5s9XK5vyRsMpQ1TxPwxPq32toIBcQ3pgZyb9Ic_m1IfWE_hC_XlwZzqfFnFL7XszCGwJmziFjML9VeBrdv0dkrDWMv1sNI1PDDm_JQ8iZwZ83At3qsgnmwN4zudOMUPRMJBNeiVBj9GjW7tV9tSG2Oa_F_JUo0b1Gr_y08PSMhAckj6ZaR8_EBppoty9CbTm65nsCAwEAAaOCAeQwggHgMA4GA1UdDwEB_wQEAwIHgDAMBgNVHRMBAf8EAjAAMG0GA1UdIAEB_wRjMGEwXwYJKwYBBAGCNxUfMFIwUAYIKwYBBQUHAgIwRB5CAFQAQwBQAEEAIAAgAFQAcgB1AHMAdABlAGQAIAAgAFAAbABhAHQAZgBvAHIAbQAgACAASQBkAGUAbgB0AGkAdAB5MBAGA1UdJQQJMAcGBWeBBQgDMEoGA1UdEQEB_wRAMD6kPDA6MTgwDgYFZ4EFAgMMBWlkOjcyMBAGBWeBBQICDAdOUENUNzV4MBQGBWeBBQIBDAtpZDo0RTU0NDMwMDAfBgNVHSMEGDAWgBTTjd-fy_wwa14b1TQrBpJk2U7fpTAdBgNVHQ4EFgQUeq9wlX_04m4THgx-yMSO7QwViv8wgbIGCCsGAQUFBwEBBIGlMIGiMIGfBggrBgEFBQcwAoaBkmh0dHA6Ly9hemNzcHJvZGV1c2Fpa3B1Ymxpc2guYmxvYi5jb3JlLndpbmRvd3MubmV0L2V1cy1udGMta2V5aWQtMjNmNGUyMmFkM2JlMzc0YTQ0OTc3Mjk1NGFhMjgzYWVkNzUyNTcyZS8xMzY0YTJkMy1hZTU0LTQ3YjktODdmMy0zMjA1NDE5NDc0MGUuY2VyMA0GCSqGSIb3DQEBCwUAA4ICAQCiPgQwqysYPQpMiRDpxbsx24d1xVX_kiUwwcQJE3mSYvwe4tnaQSHjlfB3OkpDMjotxFl33oUMxxScjSrgp_1o6rdkiO6QvPMgsqDMX4w-dmWn00akwNbMasTxg39Ceqtocw4i-R9AlNwndpe3QUIt8xkQ5dhlcIF8lc1dXmgz4mkMAtOi3VgaNvHTsRF9pLbTczJss608X8b4gHqM4t7lfIcRB8DvSyfXc7T3k21-4_3jvAb2HRoCCAyv8_XXn1UwkWTrXMLUSiE1p5Sl8ba8I_86Hsemsc0aflwRZrrY2pC3aaA3QbbfAyskiaFPw-ZibY9p0_QVq1XhAKa-dDd70mWvTGKQdrqfZI_SC5zccvDAm6aefAfnYBY2fV92ZFriihA2ULcJaESz3X3JkiK4eO1k0T2uf9-rL4lUEADibwpnsZOBeNWBsztvXDmcZGR_MSoRIQygKMw2U7AproqBPDRDFwhS5yc9UHvD6dMZ3PLx4i_eo-BLr-QJ2HARoyK8KuV0xLEq3XyjWdfZDbAueUVgtic14wK9jiSbhycRT2WV3-QU8KPm5_QCt_eBPwY81a-q84jm2ue_ok8-LYrmWpvihqRhFhK9MLVS96QaHeeuDehYNDWsSIVCr9jB-lchueZ-kZqwyl_4pPMrM7wLXBOR-bV5_pAPv3u_RvQmhVkG7zCCBuswggTToAMCAQICEzMAAAQHrjuoB9SvW8wAAAAABAcwDQYJKoZIhvcNAQELBQAwgYwxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpXYXNoaW5ndG9uMRAwDgYDVQQHEwdSZWRtb25kMR4wHAYDVQQKExVNaWNyb3NvZnQgQ29ycG9yYXRpb24xNjA0BgNVBAMTLU1pY3Jvc29mdCBUUE0gUm9vdCBDZXJ0aWZpY2F0ZSBBdXRob3JpdHkgMjAxNDAeFw0yMTA2MDMxNzUxNDdaFw0yNzA2MDMxNzUxNDdaMEExPzA9BgNVBAMTNkVVUy1OVEMtS0VZSUQtMjNGNEUyMkFEM0JFMzc0QTQ0OTc3Mjk1NEFBMjgzQUVENzUyNTcyRTCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIBAMkPU9X8JhPBwDxmFm84D31b8xN5NQz0XR8Nji_-Z8v3WtC4lSdEwJUwqvZkj5OQ3wPA_6haONcCHzqTZhyz1aheOPhXmEeWFWjEiJFj07crEZb9wM4rM1fdcf3vCQNSSDlogC5AM-tITx31hm0YffIrzM3n70fNBBfvlw8t-yhZVOavj7l29gKsyvkR0IadruvLVWWVeH9rueHVrOwlU4wUJpjD41d4U87M3FgUGK2YacQxT0BPHzaOCTE9YhylG5fA_eCF7Q1SxAe347uIaS6I3GhAootzJy9XYeFp_uhc1Yp2hMh5wdeRkm15WKb7tE9T4vwHp0VCQEkUQn1ClN_s7PpfKNFp-DB9ez0Fh7tqag6AssrKE6LgOjfWDWUcgzgIiFLvv9Gx797IZj8LDazK1iGSqI2D8zmmxnGG47MevfY8q2udJW1G4nOcjw49x6XZHmnT3VpVKcTDbI9bEsyc2R9vngftF9FgnEVdyt-QRqE0UqEXJmjLhcxBMeyFZJd_bEAutSBpWugPk10IPFRkXppsuHMZFHJVP96IWwVmm6Q4mX018K996XDubAGblbhvPzJ9NFL_e7xM2ev3rAalz2CzSLYs48EXym7dqGTnP7F9DaF2O0IHT0GQ951wFVoGmA-IYsTMVsdlhVaImCuHgahu1W94H6BvtDkGGku7AgMBAAGjggGOMIIBijAOBgNVHQ8BAf8EBAMCAoQwGwYDVR0lBBQwEgYJKwYBBAGCNxUkBgVngQUIAzAWBgNVHSAEDzANMAsGCSsGAQQBgjcVHzASBgNVHRMBAf8ECDAGAQH_AgEAMB0GA1UdDgQWBBTTjd-fy_wwa14b1TQrBpJk2U7fpTAfBgNVHSMEGDAWgBR6jArOL0hiF-KU0a5VwVLscXSkVjBwBgNVHR8EaTBnMGWgY6Bhhl9odHRwOi8vd3d3Lm1pY3Jvc29mdC5jb20vcGtpb3BzL2NybC9NaWNyb3NvZnQlMjBUUE0lMjBSb290JTIwQ2VydGlmaWNhdGUlMjBBdXRob3JpdHklMjAyMDE0LmNybDB9BggrBgEFBQcBAQRxMG8wbQYIKwYBBQUHMAKGYWh0dHA6Ly93d3cubWljcm9zb2Z0LmNvbS9wa2lvcHMvY2VydHMvTWljcm9zb2Z0JTIwVFBNJTIwUm9vdCUyMENlcnRpZmljYXRlJTIwQXV0aG9yaXR5JTIwMjAxNC5jcnQwDQYJKoZIhvcNAQELBQADggIBAIQJqhFB71eZzZMq0w866QXDKlHcGyIa_IkTK4p5ejIdIA7FJ8neeVToAKUt9ULEb1Od2ir1y5Qx5Zp_edf4F8aikn-yw61hNB3FQ4iSV49eqEMe2Fx6OMBmHRWGtUjAlf5g_N2Qc6rHela2d69nQbpSF3Nq7AESguXxnoqZ-4CGUW0jC_b93sTd5fESHs_iwFX-zWKCwCXerqCuI3PqYWOlbCnftYhsI1CD638wJxw4YFXdSmOrF8dDnd6tlH_0qCZrBX-k4N-8QgK1-BDYIxmvUBnpLFDDitB2dP6YIglY0VcjkPd3BDmodHknG4GQeAvJKHpqF91Y3K1rOWvn4JqzHFvL3JgXgL7LbC_h9EF50HeHayPCToTS8Pmg_4dfUaCwNlxPvu9GvjrDKDNNEV5T73iWMV_GQbVsx6JULAljCthYLo-55mONDcr1x7kakXlQT-yIdIQ57Ix8eHz_qkJkvWxbw8vOgrXhkLK0jGAvW_YSkTV7G9_TYDJ--8IjPPHC1bexKq72-L7KetwH6LbWHGeYkJnaZ1zqeN4USxyJn8K4uhwnjSeK2sZ942zn5EnZnjd85yfdkPLcQY8xtYiWNjc_PprTrjhLyMO71VdMkTDiTTtDha37qywNISPV7vBv8YDiDjX8ElsWbTHTC0XgBp0h-RkjaRKI5C4eTUebZ3B1YkFyZWFYdgAjAAsABAByACCd_8vzbDg65pn7mGjcbcuJ1xU4hL4oA5IsEkFYv60irgAQABAAAwAQACCweOEk52r8mnJ6y9bsGcM3V4dL1LWt8I67Jjx5mcrFuAAgjwd_jaCEEOAJLV97kX3VgbxzopPYMC4NqEFjD0m55PpoY2VydEluZm9Yof9UQ0eAFwAiAAvgBLotxyAAbygBG4efe84V0SVYnO6xLrYaC1oyLgTt3QAUjcjAdORvuzxCfLBU7KNxPFSPE84AAAAUHn9jxccO2yRJARoXARNN0IPNWxnEACIACxfcHNQuRgb_05OKyBrS_1kY5IYxOl67gTlqkHd4g6slACIAC7tcXSHNTw8ANLeZd3PKooKsgrMIlGD47aunn05BcquwaGF1dGhEYXRhWKRqubvw35oW-R27M7uxMvr50Xx4LEgmxuxw7O5Y2X71KkUAAAAACJhwWMrcS4G24TDeUNy-lgAgBoLAd0jIDI0ztrH1N45XQ_0w_N5ndt3hpNixQi3J2NqlAQIDJiABIVggsHjhJOdq_JpyesvW7BnDN1eHS9S1rfCOuyY8eZnKxbgiWCCPB3-NoIQQ4AktX3uRfdWBvHOik9gwLg2oQWMPSbnk-g",
    "clientDataJSON":"eyJ0eXBlIjoid2ViYXV0aG4uY3JlYXRlIiwiY2hhbGxlbmdlIjoiRTJZZWJNbUc5OTkyWGlhbHBGTDFsa1BwdE9JQlBlS3NwaE5rdDFKY2JLayIsIm9yaWdpbiI6Imh0dHBzOi8vd2ViYXV0aG4uZmlyc3R5ZWFyLmlkLmF1IiwiY3Jvc3NPcmlnaW4iOmZhbHNlLCJvdGhlcl9rZXlzX2Nhbl9iZV9hZGRlZF9oZXJlIjoiZG8gbm90IGNvbXBhcmUgY2xpZW50RGF0YUpTT04gYWdhaW5zdCBhIHRlbXBsYXRlLiBTZWUgaHR0cHM6Ly9nb28uZ2wveWFiUGV4In0"
    },
    "type":"public-key",
    "extensions":{
        "appid":null,
        "cred_blob":null,
        "cred_props":{
            "rk":true
        }
    }
}

I've written a byte for byte breakdown of the pubArea for that specific sample over here: https://github.com/kanidm/webauthn-rs/issues/169#issuecomment-1213103411.

madwizard-thomas commented 2 years ago

Thanks, very helpful! I'll look into it. Nice to see some crossover contributions from different webauthn library authors btw :)

madwizard-thomas commented 1 year ago

Should be fixed in 0.9!

madwizard-thomas commented 1 year ago

Btw, when implementing the parsing code I looked at the TPM specs again and found more structures that strictly speaking were parsed incorrectly. The symmetric and scheme parameters and kdf for ECC are only 2 bytes if the algorithm is TPM_ALG_NULL. If not, the structures may be longer (e.g. symmetric is TPMT_SYM_DEF_OBJECT which may contain keyBits and mode as additional fields). These parameters should be TPM_ALG_NULL but if they are not the parser would read the wrong bytes for subsequent fields. Your library seems to be affected by this also. I 'fixed' it by immediately throwing an exception while parsing if an any of these fields is not TPM_ALG_NULL. I think it would not happen in practice but reading the wrong data as public key information is questionable at best :)