From 0d09f3e2025e1b01dc831dbf6f87f1e26fe217de Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 4 Aug 2023 12:14:29 +0200 Subject: [PATCH] Prevent data races with multiple PKCS7 encryption operations --- scep/authority.go | 46 +++++++++++++++++--------- scep/authority_test.go | 73 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 15 deletions(-) create mode 100644 scep/authority_test.go diff --git a/scep/authority.go b/scep/authority.go index d4ca37f2..8f270c15 100644 --- a/scep/authority.go +++ b/scep/authority.go @@ -28,7 +28,8 @@ type Authority struct { decrypterCertificate *x509.Certificate scepProvisionerNames []string - mu sync.RWMutex + provisionersMutex sync.RWMutex + encryptionAlgorithmMutex sync.Mutex } type authorityKey struct{} @@ -86,8 +87,8 @@ func (a *Authority) Validate() error { return nil } - a.mu.RLock() - defer a.mu.RUnlock() + a.provisionersMutex.RLock() + defer a.provisionersMutex.RUnlock() noDefaultDecrypterAvailable := a.defaultDecrypter == nil for _, name := range a.scepProvisionerNames { @@ -118,8 +119,8 @@ func (a *Authority) UpdateProvisioners(scepProvisionerNames []string) { return } - a.mu.Lock() - defer a.mu.Unlock() + a.provisionersMutex.Lock() + defer a.provisionersMutex.Unlock() a.scepProvisionerNames = scepProvisionerNames } @@ -307,20 +308,13 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m // and create a degenerate cert structure deg, err := microscep.DegenerateCertificates([]*x509.Certificate{cert}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed generating degenerate certificate: %w", err) } - // apparently the pkcs7 library uses a global default setting for the content encryption - // algorithm to use when en- or decrypting data. We need to restore the current setting after - // the cryptographic operation, so that other usages of the library are not influenced by - // this call to Encrypt(). We are not required to use the same algorithm the SCEP client uses. - encryptionAlgorithmToRestore := pkcs7.ContentEncryptionAlgorithm - pkcs7.ContentEncryptionAlgorithm = p.GetContentEncryptionAlgorithm() - e7, err := pkcs7.Encrypt(deg, msg.P7.Certificates) + e7, err := a.encrypt(deg, msg.P7.Certificates, p.GetContentEncryptionAlgorithm()) if err != nil { - return nil, err + return nil, fmt.Errorf("failed encrypting degenerate certificate: %w", err) } - pkcs7.ContentEncryptionAlgorithm = encryptionAlgorithmToRestore // PKIMessageAttributes to be signed config := pkcs7.SignerInfoConfig{ @@ -391,6 +385,28 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m return crepMsg, nil } +func (a *Authority) encrypt(content []byte, recipients []*x509.Certificate, algorithm int) ([]byte, error) { + // apparently the pkcs7 library uses a global default setting for the content encryption + // algorithm to use when en- or decrypting data. We need to restore the current setting after + // the cryptographic operation, so that other usages of the library are not influenced by + // this call to Encrypt(). We are not required to use the same algorithm the SCEP client uses. + a.encryptionAlgorithmMutex.Lock() + defer a.encryptionAlgorithmMutex.Unlock() + + encryptionAlgorithmToRestore := pkcs7.ContentEncryptionAlgorithm + defer func() { + pkcs7.ContentEncryptionAlgorithm = encryptionAlgorithmToRestore + }() + + pkcs7.ContentEncryptionAlgorithm = algorithm + e7, err := pkcs7.Encrypt(content, recipients) + if err != nil { + return nil, err + } + + return e7, nil +} + // CreateFailureResponse creates an appropriately signed reply for PKI operations func (a *Authority) CreateFailureResponse(ctx context.Context, _ *x509.CertificateRequest, msg *PKIMessage, info FailInfoName, infoText string) (*PKIMessage, error) { config := pkcs7.SignerInfoConfig{ diff --git a/scep/authority_test.go b/scep/authority_test.go new file mode 100644 index 00000000..0aa81b49 --- /dev/null +++ b/scep/authority_test.go @@ -0,0 +1,73 @@ +package scep + +import ( + "crypto/x509" + "crypto/x509/pkix" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.mozilla.org/pkcs7" + "go.step.sm/crypto/keyutil" + "go.step.sm/crypto/minica" + "go.step.sm/crypto/randutil" +) + +func generateContent(t *testing.T, size int) []byte { + t.Helper() + b, err := randutil.Bytes(size) + require.NoError(t, err) + return b +} + +func generateRecipients(t *testing.T) []*x509.Certificate { + ca, err := minica.New() + require.NoError(t, err) + s, err := keyutil.GenerateSigner("RSA", "", 2048) + require.NoError(t, err) + tmpl := &x509.Certificate{ + PublicKey: s.Public(), + Subject: pkix.Name{CommonName: "Test PKCS#7 Encryption"}, + } + cert, err := ca.Sign(tmpl) + require.NoError(t, err) + return []*x509.Certificate{cert} +} + +func TestAuthority_encrypt(t *testing.T) { + t.Parallel() + a := &Authority{} + recipients := generateRecipients(t) + type args struct { + content []byte + recipients []*x509.Certificate + algorithm int + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"alg-0", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmDESCBC}, false}, + {"alg-1", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmAES128CBC}, false}, + {"alg-2", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmAES256CBC}, false}, + {"alg-3", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmAES128GCM}, false}, + {"alg-4", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmAES256GCM}, false}, + {"alg-unknown", args{generateContent(t, 32), recipients, 42}, true}, + } + for _, tt := range tests { + tc := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := a.encrypt(tc.args.content, tc.args.recipients, tc.args.algorithm) + if tc.wantErr { + assert.Error(t, err) + assert.Nil(t, got) + return + } + + assert.NoError(t, err) + assert.NotEmpty(t, got) + }) + } +}