From 80244bce554ee94bb1e32f367983233bc6f250e5 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Fri, 11 Feb 2022 17:36:41 -0500 Subject: [PATCH] Fix pkcs7 parsing in some cases (#12519) (#13851) * Fix pkcs7 parsing in some cases brings in https://github.com/mozilla-services/pkcs7/pull/61 from upstream In some cases but not all, aws includes a certificate in the pkcs7 response, and currently vault fails to parse those certificates: ``` URL: PUT https://vault.example.com/v1/auth/aws/login Code: 500. Errors * failed to parse the BER encoded PKCS#7 signature: ber2der: Invalid BER format ``` This fixes logins on those instances. Note we could not readily ascertain why some instances have those certificates and others don't. * Add changelog entry * Correct missed line Co-authored-by: Jacob Burroughs --- builtin/credential/aws/pkcs7/ber.go | 45 ++++++++---- builtin/credential/aws/pkcs7/ber_test.go | 90 +++++++++++++++++++++++- changelog/12519.txt | 3 + 3 files changed, 125 insertions(+), 13 deletions(-) create mode 100644 changelog/12519.txt diff --git a/builtin/credential/aws/pkcs7/ber.go b/builtin/credential/aws/pkcs7/ber.go index 585256739c038..19d9fa7df20cf 100644 --- a/builtin/credential/aws/pkcs7/ber.go +++ b/builtin/credential/aws/pkcs7/ber.go @@ -175,7 +175,7 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) { if offset > berLen { return nil, 0, errors.New("ber2der: cannot move offset forward, end of ber data reached") } - hack := 0 + indefinite := false if l > 0x80 { numberOfBytes := (int)(l & 0x7F) if numberOfBytes > 4 { // int is only guaranteed to be 32bit @@ -197,14 +197,7 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) { } } } else if l == 0x80 { - // find length by searching content - markerIndex := bytes.LastIndex(ber[offset:], []byte{0x0, 0x0}) - if markerIndex == -1 { - return nil, 0, errors.New("ber2der: Invalid BER format") - } - length = markerIndex - hack = 2 - debugprint("--> (compute length) marker found at offset: %d\n", markerIndex+offset) + indefinite = true } else { length = (int)(l) } @@ -220,6 +213,9 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) { debugprint("--> content end : %d\n", contentEnd) debugprint("--> content : % X\n", ber[offset:contentEnd]) var obj asn1Object + if indefinite && kind == 0 { + return nil, 0, errors.New("ber2der: Indefinite form tag must have constructed encoding") + } if kind == 0 { obj = asn1Primitive{ tagBytes: ber[tagStart:tagEnd], @@ -228,14 +224,25 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) { } } else { var subObjects []asn1Object - for offset < contentEnd { + for (offset < contentEnd) || indefinite { var subObj asn1Object var err error - subObj, offset, err = readObject(ber[:contentEnd], offset) + subObj, offset, err = readObject(ber, offset) if err != nil { return nil, 0, err } subObjects = append(subObjects, subObj) + + if indefinite { + terminated, err := isIndefiniteTermination(ber, offset) + if err != nil { + return nil, 0, err + } + + if terminated { + break + } + } } obj = asn1Structured{ tagBytes: ber[tagStart:tagEnd], @@ -243,9 +250,23 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) { } } - return obj, contentEnd + hack, nil + // Apply indefinite form length with 0x0000 terminator. + if indefinite { + contentEnd = offset + 2 + } + + return obj, contentEnd, nil +} + +func isIndefiniteTermination(ber []byte, offset int) (bool, error) { + if len(ber) - offset < 2 { + return false, errors.New("ber2der: Invalid BER format") + } + + return bytes.Index(ber[offset:], []byte{0x0, 0x0}) == 0, nil } func debugprint(format string, a ...interface{}) { //fmt.Printf(format, a) } + diff --git a/builtin/credential/aws/pkcs7/ber_test.go b/builtin/credential/aws/pkcs7/ber_test.go index fcb4b6a24f023..169c78ab701e5 100644 --- a/builtin/credential/aws/pkcs7/ber_test.go +++ b/builtin/credential/aws/pkcs7/ber_test.go @@ -3,6 +3,8 @@ package pkcs7 import ( "bytes" "encoding/asn1" + "encoding/pem" + "fmt" "strings" "testing" ) @@ -45,7 +47,8 @@ func TestBer2Der_Negatives(t *testing.T) { {[]byte{0x30, 0x85}, "tag length too long"}, {[]byte{0x30, 0x84, 0x80, 0x0, 0x0, 0x0}, "length is negative"}, {[]byte{0x30, 0x82, 0x0, 0x1}, "length has leading zero"}, - {[]byte{0x30, 0x80, 0x1, 0x2}, "Invalid BER format"}, + {[]byte{0x30, 0x80, 0x1, 0x2, 0x1, 0x2}, "Invalid BER format"}, + {[]byte{0x30, 0x80, 0x1, 0x2}, "BER tag length is more than available data"}, {[]byte{0x30, 0x03, 0x01, 0x02}, "length is more than available data"}, {[]byte{0x30}, "end of ber data reached"}, } @@ -60,3 +63,88 @@ func TestBer2Der_Negatives(t *testing.T) { } } } + +func TestVerifyIndefiniteLengthBer(t *testing.T) { + decoded := mustDecodePEM([]byte(testPKCS7)) + + _, err := ber2der(decoded) + if err != nil { + t.Errorf("cannot parse indefinite length ber: %v", err) + } +} + +func mustDecodePEM(data []byte) []byte { + var block *pem.Block + block, rest := pem.Decode(data) + if len(rest) != 0 { + panic(fmt.Errorf("unexpected remaining PEM block during decode")) + } + return block.Bytes +} + +const testPKCS7 = ` +-----BEGIN PKCS7----- +MIAGCSqGSIb3DQEHAqCAMIACAQExDzANBglghkgBZQMEAgEFADCABgkqhkiG9w0B +BwGggCSABIIDfXsiQWdlbnRBY3Rpb25PdmVycmlkZXMiOnsiQWdlbnRPdmVycmlk +ZXMiOnsiRmlsZUV4aXN0c0JlaGF2aW9yIjoiT1ZFUldSSVRFIn19LCJBcHBsaWNh +dGlvbklkIjoiZTA0NDIzZTQtN2E2Ny00ZjljLWIyOTEtOTllNjNjMWMyMTU4Iiwi +QXBwbGljYXRpb25OYW1lIjoibWthbmlhLXhyZF9zYW0uY2R3c19lY2hvc2VydmVy +IiwiRGVwbG95bWVudENyZWF0b3IiOiJ1c2VyIiwiRGVwbG95bWVudEdyb3VwSWQi +OiJmYWI5MjEwZi1mNmM3LTQyODUtYWEyZC03Mzc2MGQ4ODE3NmEiLCJEZXBsb3lt +ZW50R3JvdXBOYW1lIjoibWthbmlhLXhyZF9zYW0uY2R3c19lY2hvc2VydmVyX2Rn +IiwiRGVwbG95bWVudElkIjoiZC1UREUxVTNXREEiLCJEZXBsb3ltZW50VHlwZSI6 +IklOX1BMQUNFIiwiR2l0SHViQWNjZXNzVG9rZW4iOm51bGwsIkluc3RhbmNlR3Jv +dXBJZCI6ImZhYjkyMTBmLWY2YzctNDI4NS1hYTJkLTczNzYwZDg4MTc2YSIsIlJl +dmlzaW9uIjp7IkFwcFNwZWNDb250ZW50IjpudWxsLCJDb2RlQ29tbWl0UmV2aXNp +b24iOm51bGwsIkdpdEh1YlJldmlzaW9uIjpudWxsLCJHaXRSZXZpc2lvbiI6bnVs +bCwiUmV2aXNpb25UeXBlIjoiUzMiLCJTM1JldmlzaW9uIjp7IkJ1Y2tldCI6Im1r +YW5pYS1jZHdzLWRlcGxveS1idWNrZXQiLCJCdW5kbGVUeXBlIjoiemlwIiwiRVRh +ZyI6bnVsbCwiS2V5IjoieHJkOjpzYW0uY2R3czo6ZWNob3NlcnZlcjo6MTo6Lnpp +cCIsIlZlcnNpb24iOm51bGx9fSwiUzNSZXZpc2lvbiI6eyJCdWNrZXQiOiJta2Fu +aWEtY2R3cy1kZXBsb3ktYnVja2V0IiwiQnVuZGxlVHlwZSI6InppcCIsIkVUYWci +Om51bGwsIktleSI6InhyZDo6c2FtLmNkd3M6OmVjaG9zZXJ2ZXI6OjE6Oi56aXAi +LCJWZXJzaW9uIjpudWxsfSwiVGFyZ2V0UmV2aXNpb24iOm51bGx9AAAAAAAAoIAw +ggWbMIIEg6ADAgECAhAGrjFMK45t2jcNHtjY1DjEMA0GCSqGSIb3DQEBCwUAMEYx +CzAJBgNVBAYTAlVTMQ8wDQYDVQQKEwZBbWF6b24xFTATBgNVBAsTDFNlcnZlciBD +QSAxQjEPMA0GA1UEAxMGQW1hem9uMB4XDTIwMTExMjAwMDAwMFoXDTIxMTAxNTIz +NTk1OVowNDEyMDAGA1UEAxMpY29kZWRlcGxveS1zaWduZXItdXMtZWFzdC0yLmFt +YXpvbmF3cy5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDit4f+ +I4BSv4rBV/8bJ+f4KqBwTCt9iJeau/r9liQfMgj/C1M2E+aa++u8BtY/LQstB44v +v6KqcaiOyWpkD9OsUty9qb4eNTPF2Y4jpNsi/Hfw0phsd9gLun2foppILmL4lZIG +lBhTeEwv6qV4KbyXOG9abHOX32+jVFtM1rbzHNFvz90ysfZp16TBAi7IRKEZeXvd +MvlJJMAJtAoblxiDIS3A1csY1G4XHYET8xIoCop3mqEZEtAxUUP2epdXXdhD2U0G +7alSRS54o91QW1Dp3A13lu1A1nds9CkWlPkDTpKSUG/qN5y5+6dCCGaydgL5krMs +R79bCrR1sEKm5hi1AgMBAAGjggKVMIICkTAfBgNVHSMEGDAWgBRZpGYGUqB7lZI8 +o5QHJ5Z0W/k90DAdBgNVHQ4EFgQUPF5qTbnTDYhmp7tGmmL/jTmLoHMwNAYDVR0R +BC0wK4IpY29kZWRlcGxveS1zaWduZXItdXMtZWFzdC0yLmFtYXpvbmF3cy5jb20w +DgYDVR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjA7 +BgNVHR8ENDAyMDCgLqAshipodHRwOi8vY3JsLnNjYTFiLmFtYXpvbnRydXN0LmNv +bS9zY2ExYi5jcmwwIAYDVR0gBBkwFzALBglghkgBhv1sAQIwCAYGZ4EMAQIBMHUG +CCsGAQUFBwEBBGkwZzAtBggrBgEFBQcwAYYhaHR0cDovL29jc3Auc2NhMWIuYW1h +em9udHJ1c3QuY29tMDYGCCsGAQUFBzAChipodHRwOi8vY3J0LnNjYTFiLmFtYXpv +bnRydXN0LmNvbS9zY2ExYi5jcnQwDAYDVR0TAQH/BAIwADCCAQQGCisGAQQB1nkC +BAIEgfUEgfIA8AB2APZclC/RdzAiFFQYCDCUVo7jTRMZM7/fDC8gC8xO8WTjAAAB +dboejIcAAAQDAEcwRQIgeqoKXbST17TCEzM1BMWx/jjyVQVBIN3LG17U4OaV364C +IQDPUSJZhJm7uqGea6+VwqeDe/vGuGSuJzkDwTIOeIXPaAB2AFzcQ5L+5qtFRLFe +mtRW5hA3+9X6R9yhc5SyXub2xw7KAAABdboejNQAAAQDAEcwRQIgEKIAwwhjUcq2 +iwzBAagdy+fTiKnBY1Yjf6wOeRpwXfMCIQC8wM3nxiWrGgIpdzzgDvFhZZTV3N81 +JWcYAu+srIVOhTANBgkqhkiG9w0BAQsFAAOCAQEAer9kml53XFy4ZSVzCbdsIFYP +Ohu7LDf5iffHBVZFnGOEVOmiPYYkNwi9R6EHIYaAs7G7GGLCp/6tdc+G4eF1j6wB +IkmXZcxMTxk/87R+S+36yDLg1GBZvqttLfexj0TRVAfVLJc7FjLXAW2+wi7YyNe8 +X17lWBwHxa1r5KgweJshGzYVUsgMTSx0aJ+93ZnqplBp9x+9DSQNqqNlBgxFANxs +ux+dfpduyLd8VLqtlECGC07tYE4mBaAjMiNjCZRWMp8ya/Z6J/bJZ27IDGA4dXzm +l9NNnlbuUDAenAByUqE+0b78J6EmmdAVf+N8siriMg02FdP3lAXJLE8tDeZp8AAA +MYICIDCCAhwCAQEwWjBGMQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRUw +EwYDVQQLEwxTZXJ2ZXIgQ0EgMUIxDzANBgNVBAMTBkFtYXpvbgIQBq4xTCuObdo3 +DR7Y2NQ4xDANBglghkgBZQMEAgEFAKCBmDAYBgkqhkiG9w0BCQMxCwYJKoZIhvcN +AQcBMBwGCSqGSIb3DQEJBTEPFw0yMTA2MjQxOTU1MzFaMC0GCSqGSIb3DQEJNDEg +MB4wDQYJYIZIAWUDBAIBBQChDQYJKoZIhvcNAQELBQAwLwYJKoZIhvcNAQkEMSIE +IP7gMuT2H0/AhgPgj3Eo0NWCIdQOBjJO18coNKIaOnJYMA0GCSqGSIb3DQEBCwUA +BIIBAJX+e87q0YvRon9/ENTvE0FoYMzYblID2Reek6L217ZlZ6pUuRsc4ghhJ5Yh +WZeOCaLwi4mrnQ5/+DGKkJ4a/w5sqFTwtJIGIIAuDCn/uDm8kIDUVkbeznSOLoPA +67cxiqgIdqZ5pqUoid2YsDj20owrGDG4wUF6ZvhM9g/5va3CAhxqvTE2HwjhHTfz +Cgl8Nlvalz7YxXEf2clFEiEVa1fVaGMl9pCyedAmTfd6hoivcpAsopvXfVaaaR2y +iuZidpUfFhSk+Ls7TU/kB74ckfUGj5q/5HcKJgb/S+FYUV7eu0ewzTyW1uRl/d0U +Tb7e7EjgDGJsjOTMdTrMfv8ho8kAAAAAAAA= +-----END PKCS7----- +` diff --git a/changelog/12519.txt b/changelog/12519.txt new file mode 100644 index 0000000000000..df383b9c97659 --- /dev/null +++ b/changelog/12519.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/aws: Fix ec2 auth on instances that have a cert in their PKCS7 signature +```