Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

join_ec2.parseAndVerifyIID: Add Defer Func Recovery #29231

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions lib/auth/fuzz_test.go
Original file line number Diff line number Diff line change
@@ -17,12 +17,78 @@ limitations under the License.
package auth

import (
"encoding/base64"
"testing"

"github.com/stretchr/testify/require"
)

func unverifiedBase64Bytes(str string) []byte {
bytes, _ := base64.StdEncoding.DecodeString(str)
return bytes
}

func FuzzParseAndVerifyIID(f *testing.F) {
f.Add(unverifiedBase64Bytes("WDcwMA=="))
f.Add(unverifiedBase64Bytes("N0IwQUMwMDAwMDAwMDAwMDAwMDA3Q0FBQTAwMDAwMEEwQVgwMDAwMDAwMEMwMDAwQTIyQTBBMDAw" +
"MDAwMDAwMDAwMDAwMDAwMFlJMA=="))
// Inputs from unit tests
f.Add(unverifiedBase64Bytes("YmFkIGRvY3VtZW50"))
f.Add(unverifiedBase64Bytes("TUlBR0NTcUdTSWIzRFFFSEFxQ0FNSUFDQVFFeER6QU5CZ2xnaGtnQlpRTUVBZ0VGQURDQUJna3Fo" +
"a2lHOXcwQkJ3R2dnQ1NBQklJQgoyM3NLSUNBaVlXTmpiM1Z1ZEVsa0lpQTZJQ0l5TnpnMU56WXlN" +
"akEwTlRNaUxBb2dJQ0poY21Ob2FYUmxZM1IxY21VaUlEb2dJbmc0Ck5sODJOQ0lzQ2lBZ0ltRjJZ" +
"V2xzWVdKcGJHbDBlVnB2Ym1VaUlEb2dJblZ6TFhkbGMzUXRNbUVpTEFvZ0lDSmlhV3hzYVc1blVI" +
"SnYKWkhWamRITWlJRG9nYm5Wc2JDd0tJQ0FpWkdWMmNHRjVVSEp2WkhWamRFTnZaR1Z6SWlBNklH" +
"NTFiR3dzQ2lBZ0ltMWhjbXRsZEhCcwpZV05sVUhKdlpIVmpkRU52WkdWeklpQTZJRzUxYkd3c0Np" +
"QWdJbWx0WVdkbFNXUWlJRG9nSW1GdGFTMHdabUU1WlRGbU5qUXhOREpqClpHVXhOeUlzQ2lBZ0lt" +
"bHVjM1JoYm1ObFNXUWlJRG9nSW1rdE1EYzROVEUzWTJFNFlUY3dZVEZrWkdVaUxBb2dJQ0pwYm5O" +
"MFlXNWoKWlZSNWNHVWlJRG9nSW5ReUxtMWxaR2wxYlNJc0NpQWdJbXRsY201bGJFbGtJaUE2SUc1" +
"MWJHd3NDaUFnSW5CbGJtUnBibWRVYVcxbApJaUE2SUNJeU1ESXhMVEE1TFRBelZESXhPakkxT2pR" +
"MFdpSXNDaUFnSW5CeWFYWmhkR1ZKY0NJZ09pQWlNVEF1TUM0d0xqSXdPU0lzCkNpQWdJbkpoYldS" +
"cGMydEpaQ0lnT2lCdWRXeHNMQW9nSUNKeVpXZHBiMjRpSURvZ0luVnpMWGRsYzNRdE1pSXNDaUFn" +
"SW5abGNuTnAKYjI0aUlEb2dJakl3TVRjdE1Ea3RNekFpQ24wQUFBQUFBQUF4Z2dJdk1JSUNLd0lC" +
"QVRCcE1Gd3hDekFKQmdOVkJBWVRBbFZUTVJrdwpGd1lEVlFRSUV4QlhZWE5vYVc1bmRHOXVJRk4w" +
"WVhSbE1SQXdEZ1lEVlFRSEV3ZFRaV0YwZEd4bE1TQXdIZ1lEVlFRS0V4ZEJiV0Y2CmIyNGdWMlZp" +
"SUZObGNuWnBZMlZ6SUV4TVF3SUpBTFpMM2xyUUNTVE1NQTBHQ1dDR1NBRmxBd1FDQVFVQW9JR1lN" +
"QmdHQ1NxR1NJYjMKRFFFSkF6RUxCZ2txaGtpRzl3MEJCd0V3SEFZSktvWklodmNOQVFrRk1ROFhE" +
"VEl4TURrd016SXhNalUwTjFvd0xRWUpLb1pJaHZjTgpBUWswTVNBd0hqQU5CZ2xnaGtnQlpRTUVB" +
"Z0VGQUtFTkJna3Foa2lHOXcwQkFRc0ZBREF2QmdrcWhraUc5dzBCQ1FReElnUWdDSDJkCkppS21k" +
"eDl1aHhsbThPYldBdkZPaHFKYjdrNzkrRFcvVDNlendWVXdEUVlKS29aSWh2Y05BUUVMQlFBRWdn" +
"RUFOV2F1dGlncy9xWjYKdzhnNS9FZldzQUZqOGtIZ1VEK3hxc1ExSERyQlV4M0lRNDk4Tk1CWjc4" +
"Mzc5QjhSQmZ1emVWamJhZit5dWdvdjBmWXJEYkd2U1JSdwpteXk0OVRmWjlnZGxwV1FYendTZzNP" +
"UE1ETlRvUm9LdzAwL0xRalN4Y1RDYVBQNHZNREVJallNVXFaM2k0dVdZSkpKMExiN2ZETURrCkFu" +
"dTd5SG9sVmZibnZJQXVaZThsR3BjN29mQ1NCRzV3dWxtKy9wcXpPMjVZUE1IMWNMRXZPYWRFKzNO" +
"Mkd4SzZnUlRMSm9FOThyc20KTERwNk91VS9iMlFmYXhVMGVjNk9vZ2R0U0p0by9VUkkwL3lnSG1O" +
"QXpCaXM0NzBBMjl5aDVuVndtNkFrWTRrcmpQc0s3dWlCSVJocwpscjV4MFg2K2dnUWZGMkJLQUov" +
"QlJjQUhOZ0FBQUFBQUFBPT0="))
f.Add(unverifiedBase64Bytes("TUlBR0NTcUdTSWIzRFFFSEFxQ0FNSUFDQVFFeER6QU5CZ2xnaGtnQlpRTUVBZ0VGQURDQUJna3Fo" +
"a2lHOXcwQkJ3R2dnQ1NBQklJQgozWHNLSUNBaVlXTmpiM1Z1ZEVsa0lpQTZJQ0k0T0RNME56UTJO" +
"akk0T0RnaUxBb2dJQ0poY21Ob2FYUmxZM1IxY21VaUlEb2dJbmc0Ck5sODJOQ0lzQ2lBZ0ltRjJZ" +
"V2xzWVdKcGJHbDBlVnB2Ym1VaUlEb2dJblZ6TFhkbGMzUXRNV01pTEFvZ0lDSmlhV3hzYVc1blVI" +
"SnYKWkhWamRITWlJRG9nYm5Wc2JDd0tJQ0FpWkdWMmNHRjVVSEp2WkhWamRFTnZaR1Z6SWlBNklH" +
"NTFiR3dzQ2lBZ0ltMWhjbXRsZEhCcwpZV05sVUhKdlpIVmpkRU52WkdWeklpQTZJRzUxYkd3c0Np" +
"QWdJbWx0WVdkbFNXUWlJRG9nSW1GdGFTMHdZMlV6WXpVMVlUTXhaREk1Ck1EUXdaU0lzQ2lBZ0lt" +
"bHVjM1JoYm1ObFNXUWlJRG9nSW1rdE1ERmlPVFF3WXpRMVptUXhNV1psTnpRaUxBb2dJQ0pwYm5O" +
"MFlXNWoKWlZSNWNHVWlJRG9nSW5ReUxtMXBZM0p2SWl3S0lDQWlhMlZ5Ym1Wc1NXUWlJRG9nYm5W" +
"c2JDd0tJQ0FpY0dWdVpHbHVaMVJwYldVaQpJRG9nSWpJd01qRXRNRGt0TVRGVU1EQTZNVFE2TVRo" +
"YUlpd0tJQ0FpY0hKcGRtRjBaVWx3SWlBNklDSXhOekl1TXpFdU1USXVNalV4Cklpd0tJQ0FpY21G" +
"dFpHbHphMGxrSWlBNklHNTFiR3dzQ2lBZ0luSmxaMmx2YmlJZ09pQWlkWE10ZDJWemRDMHhJaXdL" +
"SUNBaWRtVnkKYzJsdmJpSWdPaUFpTWpBeE55MHdPUzB6TUNJS2ZRQUFBQUFBQURHQ0FpOHdnZ0ly" +
"QWdFQk1Ha3dYREVMTUFrR0ExVUVCaE1DVlZNeApHVEFYQmdOVkJBZ1RFRmRoYzJocGJtZDBiMjRn" +
"VTNSaGRHVXhFREFPQmdOVkJBY1RCMU5sWVhSMGJHVXhJREFlQmdOVkJBb1RGMEZ0CllYcHZiaUJY" +
"WldJZ1UyVnlkbWxqWlhNZ1RFeERBZ2tBMDArUWlseklTMGd3RFFZSllJWklBV1VEQkFJQkJRQ2dn" +
"Wmd3R0FZSktvWkkKaHZjTkFRa0RNUXNHQ1NxR1NJYjNEUUVIQVRBY0Jna3Foa2lHOXcwQkNRVXhE" +
"eGNOTWpFd09URXhNREF4TkRJeVdqQXRCZ2txaGtpRwo5dzBCQ1RReElEQWVNQTBHQ1dDR1NBRmxB" +
"d1FDQVFVQW9RMEdDU3FHU0liM0RRRUJDd1VBTUM4R0NTcUdTSWIzRFFFSkJERWlCQ0RTCjFnTnZ4" +
"YlluRUw2cGxWdThYL1FtS1BKRkp3SUpmaSsyaElWanlLQU90akFOQmdrcWhraUc5dzBCQVFzRkFB" +
"U0NBUUFCS21naEFUZzgKVlhrZGlJR2NUSVBmS3JjMnYvekVJZExVQWkrRXc1bHJHVVZqbk5xclA5" +
"aXJHSzRkOXNWdGN1LzhVS3A5UkRvZUpPUTZJL3BSY3d2VApQSlZIbGhHbkx5eWJyNVpWcWt4aUMw" +
"OUdBU05uUGUxMmR6Q0trS0QycnZXNm1HUjkxY3hwTTk0WHFpNVVBL1pScWlYd3BIbzNMRUNOCkd1" +
"MzhIcGR2NnNCZ0QvYXYyT2hkK3ZFSDJ6dllWa3A3WmZuRnVETFdSU0JRWmdtS3dWS1ZkT2pyTW1Q" +
"MzJ2YjN2emhNQnVPaitqYlEKUndFbVlJa1JhRUdOYnJaZ2F0ak1KWW1UV3VMRzI2endzM2F2T0s2" +
"a0w2dTM4RFYzd0pQVkIvRzBJcmE1TXZDL29qR3lhK0RyVm5nVwpWVVArM2pnZW5QcmQ3T3lDV1BT" +
"d09vT0JNaFNsQUFBQUFBQUE="))

f.Fuzz(func(t *testing.T, iidBytes []byte) {
require.NotPanics(t, func() {
parseAndVerifyIID(iidBytes)
Comment on lines 93 to 94
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this fuzz test pretty much worthless now that we just recover from all panics in the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fuzzing was not enabled in oss-fuzz for exactly that reason. But the seeds above serve as test cases to make sure the defer is functioning correctly.

That said, I have now prepared the library changes which should no longer necessitate the defer recover: gravitational/pkcs7#2

16 changes: 12 additions & 4 deletions lib/auth/join_ec2.go
Original file line number Diff line number Diff line change
@@ -123,7 +123,15 @@ func checkInstanceRunning(ctx context.Context, instanceID, region, IAMRole strin

// parseAndVerifyIID parses the given Instance Identity Document and checks that
// the signature is valid.
func parseAndVerifyIID(iidBytes []byte) (*imds.InstanceIdentityDocument, error) {
func parseAndVerifyIID(iidBytes []byte) (iid *imds.InstanceIdentityDocument, err error) {
// handle potential panic from pkcs7 index out of range panic
defer func() {
if r := recover(); r != nil {
iid = nil
err = fmt.Errorf("unable to parse PKCS7")
}
}()

sigPEM := fmt.Sprintf("-----BEGIN PKCS7-----\n%s\n-----END PKCS7-----", string(iidBytes))
sigBER, _ := pem.Decode([]byte(sigPEM))
if sigBER == nil {
@@ -135,8 +143,8 @@ func parseAndVerifyIID(iidBytes []byte) (*imds.InstanceIdentityDocument, error)
return nil, trace.Wrap(err)
}

var iid imds.InstanceIdentityDocument
if err := json.Unmarshal(p7.Content, &iid); err != nil {
iid = &imds.InstanceIdentityDocument{}
if err := json.Unmarshal(p7.Content, iid); err != nil {
return nil, trace.Wrap(err)
}

@@ -154,7 +162,7 @@ func parseAndVerifyIID(iidBytes []byte) (*imds.InstanceIdentityDocument, error)
return nil, trace.AccessDenied("invalid signature")
}

return &iid, nil
return iid, nil
}

func checkPendingTime(iid *imds.InstanceIdentityDocument, provisionToken types.ProvisionToken, clock clockwork.Clock) error {