From 4aef7c02b2b7143610cabb6238c9e426c7fcae3a Mon Sep 17 00:00:00 2001 From: vasraz Date: Thu, 5 Sep 2019 14:25:39 +0100 Subject: Avoid duplicated verification if package is signed Fix potential thread leak by using try-with-resource. Change-Id: I20eda524cd028b9dfdfa83207a059f4e1dfeff5e Signed-off-by: Vasyl Razinkov Issue-ID: SDC-2580 --- .../sdcrests/vsp/rest/data/PackageArchive.java | 55 ++++---- .../security/SecurityManager.java | 138 +++++++++++---------- 2 files changed, 100 insertions(+), 93 deletions(-) diff --git a/openecomp-be/api/openecomp-sdc-rest-webapp/vendor-software-products-rest/vendor-software-products-rest-services/src/main/java/org/openecomp/sdcrests/vsp/rest/data/PackageArchive.java b/openecomp-be/api/openecomp-sdc-rest-webapp/vendor-software-products-rest/vendor-software-products-rest-services/src/main/java/org/openecomp/sdcrests/vsp/rest/data/PackageArchive.java index bf029fb29e..97bc375eb8 100644 --- a/openecomp-be/api/openecomp-sdc-rest-webapp/vendor-software-products-rest/vendor-software-products-rest-services/src/main/java/org/openecomp/sdcrests/vsp/rest/data/PackageArchive.java +++ b/openecomp-be/api/openecomp-sdc-rest-webapp/vendor-software-products-rest/vendor-software-products-rest-services/src/main/java/org/openecomp/sdcrests/vsp/rest/data/PackageArchive.java @@ -19,6 +19,11 @@ */ package org.openecomp.sdcrests.vsp.rest.data; +import java.io.IOException; +import java.security.cert.CertificateException; +import java.util.List; +import java.util.Map; +import java.util.Optional; import org.apache.commons.io.FilenameUtils; import org.apache.commons.lang3.tuple.Pair; import org.apache.cxf.jaxrs.ext.multipart.Attachment; @@ -29,17 +34,12 @@ import org.openecomp.sdc.logging.api.LoggerFactory; import org.openecomp.sdc.vendorsoftwareproduct.security.SecurityManager; import org.openecomp.sdc.vendorsoftwareproduct.security.SecurityManagerException; -import java.io.IOException; -import java.security.cert.CertificateException; -import java.util.List; -import java.util.Map; -import java.util.Optional; - /** - * Class responsible for processing zip archive and verify if this package corresponds - * SOL004 option 2 signed package format, verifies the cms signature if package is signed + * Class responsible for processing zip archive and verify if this package corresponds SOL004 option 2 signed package + * format, verifies the cms signature if package is signed */ public class PackageArchive { + private static final Logger LOG = LoggerFactory.getLogger(PackageArchive.class); private static final String[] ALLOWED_ARCHIVE_EXTENSIONS = {"csar", "zip"}; private static final String[] ALLOWED_SIGNATURE_EXTENSIONS = {"cms"}; @@ -49,6 +49,7 @@ public class PackageArchive { private final SecurityManager securityManager; private final byte[] outerPackageFileBytes; private Pair> handlerPair; + private Boolean signatureValid; public PackageArchive(Attachment uploadedFile) { this(uploadedFile.getObject(byte[].class)); @@ -59,7 +60,7 @@ public class PackageArchive { this.securityManager = SecurityManager.getInstance(); try { handlerPair = CommonUtil.getFileContentMapFromOrchestrationCandidateZip( - outerPackageFileBytes); + outerPackageFileBytes); } catch (IOException exception) { LOG.error("Error reading files inside archive", exception); } @@ -88,6 +89,7 @@ public class PackageArchive { /** * Gets csar/zip package content from zip archive + * * @return csar package content * @throws SecurityManagerException */ @@ -97,37 +99,38 @@ public class PackageArchive { return handlerPair.getKey().getFiles().get(getArchiveFileName().orElseThrow(CertificateException::new)); } } catch (CertificateException exception) { - LOG.info("Error verifying signature " + exception); + LOG.info("Error verifying signature ", exception); } return outerPackageFileBytes; } /** * Validates package signature against trusted certificates + * * @return true if signature verified * @throws SecurityManagerException */ public boolean isSignatureValid() throws SecurityManagerException { - Map files = handlerPair.getLeft().getFiles(); - Optional signatureFileName = getSignatureFileName(); - Optional archiveFileName = getArchiveFileName(); - if (files.isEmpty() || !signatureFileName.isPresent() || !archiveFileName.isPresent()) { - return false; - } - Optional certificateFile = getCertificateFileName(); - if(certificateFile.isPresent()){ - return securityManager.verifySignedData(files.get(signatureFileName.get()), - files.get(certificateFile.get()), files.get(archiveFileName.get())); - }else { - return securityManager.verifySignedData(files.get(signatureFileName.get()), - null, files.get(archiveFileName.get())); + if (signatureValid == null) { + final Map files = handlerPair.getLeft().getFiles(); + final Optional signatureFileName = getSignatureFileName(); + final Optional archiveFileName = getArchiveFileName(); + if (files.isEmpty() || !signatureFileName.isPresent() || !archiveFileName.isPresent()) { + signatureValid = false; + } else { + final Optional certificateFile = getCertificateFileName(); + signatureValid = securityManager.verifySignedData(files.get(signatureFileName.get()), + certificateFile.map(files::get).orElse(null), files.get(archiveFileName.get())); + } + } + return signatureValid; } private boolean isPackageSizeMatches() { return handlerPair.getRight().isEmpty() - && (handlerPair.getLeft().getFiles().size() == NUMBER_OF_FILES_FOR_SIGNATURE_WITH_CERT_INSIDE - || handlerPair.getLeft().getFiles().size() == NUMBER_OF_FILES_FOR_SIGNATURE_WITHOUT_CERT_INSIDE); + && (handlerPair.getLeft().getFiles().size() == NUMBER_OF_FILES_FOR_SIGNATURE_WITH_CERT_INSIDE + || handlerPair.getLeft().getFiles().size() == NUMBER_OF_FILES_FOR_SIGNATURE_WITHOUT_CERT_INSIDE); } private Optional getSignatureFileName() { @@ -147,7 +150,7 @@ public class PackageArchive { private Optional getCertificateFileName() { Optional certFileName = getFileByExtension(ALLOWED_CERTIFICATE_EXTENSIONS); - if(!certFileName.isPresent()){ + if (!certFileName.isPresent()) { return Optional.empty(); } String certNameWithoutExtension = FilenameUtils.removeExtension(certFileName.get()); diff --git a/openecomp-be/backend/openecomp-sdc-vendor-software-product-manager/src/main/java/org/openecomp/sdc/vendorsoftwareproduct/security/SecurityManager.java b/openecomp-be/backend/openecomp-sdc-vendor-software-product-manager/src/main/java/org/openecomp/sdc/vendorsoftwareproduct/security/SecurityManager.java index 7b1890dcaa..90bfb67977 100644 --- a/openecomp-be/backend/openecomp-sdc-vendor-software-product-manager/src/main/java/org/openecomp/sdc/vendorsoftwareproduct/security/SecurityManager.java +++ b/openecomp-be/backend/openecomp-sdc-vendor-software-product-manager/src/main/java/org/openecomp/sdc/vendorsoftwareproduct/security/SecurityManager.java @@ -20,21 +20,6 @@ package org.openecomp.sdc.vendorsoftwareproduct.security; import com.google.common.collect.ImmutableSet; -import org.bouncycastle.asn1.cms.ContentInfo; -import org.bouncycastle.cert.X509CertificateHolder; -import org.bouncycastle.cms.CMSException; -import org.bouncycastle.cms.CMSProcessableByteArray; -import org.bouncycastle.cms.CMSSignedData; -import org.bouncycastle.cms.CMSTypedData; -import org.bouncycastle.cms.SignerInformation; -import org.bouncycastle.cms.jcajce.JcaSimpleSignerInfoVerifierBuilder; -import org.bouncycastle.jce.provider.BouncyCastleProvider; -import org.bouncycastle.openssl.PEMParser; -import org.bouncycastle.operator.OperatorCreationException; -import org.bouncycastle.util.Store; -import org.openecomp.sdc.logging.api.Logger; -import org.openecomp.sdc.logging.api.LoggerFactory; - import java.io.ByteArrayInputStream; import java.io.File; import java.io.FileInputStream; @@ -65,14 +50,30 @@ import java.security.cert.X509Certificate; import java.util.Collection; import java.util.HashSet; import java.util.Set; +import org.bouncycastle.asn1.cms.ContentInfo; +import org.bouncycastle.cert.X509CertificateHolder; +import org.bouncycastle.cms.CMSException; +import org.bouncycastle.cms.CMSProcessableByteArray; +import org.bouncycastle.cms.CMSSignedData; +import org.bouncycastle.cms.CMSTypedData; +import org.bouncycastle.cms.SignerInformation; +import org.bouncycastle.cms.jcajce.JcaSimpleSignerInfoVerifierBuilder; +import org.bouncycastle.jce.provider.BouncyCastleProvider; +import org.bouncycastle.openssl.PEMParser; +import org.bouncycastle.operator.OperatorCreationException; +import org.bouncycastle.util.Store; +import org.openecomp.sdc.logging.api.Logger; +import org.openecomp.sdc.logging.api.LoggerFactory; /** - * This is temporary solution. When AAF provides functionality for verifying trustedCertificates, this class should be reviewed - * Class is responsible for providing root trustedCertificates from configured location in onboarding container. + * This is temporary solution. When AAF provides functionality for verifying trustedCertificates, this class should be + * reviewed Class is responsible for providing root trustedCertificates from configured location in onboarding + * container. */ public class SecurityManager { + private static final String CERTIFICATE_DEFAULT_LOCATION = "cert"; - private static final SecurityManager INSTANCE = new SecurityManager(); + private static SecurityManager INSTANCE = null; private Logger logger = LoggerFactory.getLogger(SecurityManager.class); private Set trustedCertificates = new HashSet<>(); @@ -88,12 +89,14 @@ public class SecurityManager { certificateDirectory = this.getcertDirectory(); } - public static SecurityManager getInstance(){ + public static SecurityManager getInstance() { + if (INSTANCE == null) { + INSTANCE = new SecurityManager(); + } return INSTANCE; } /** - * * Checks the configured location for available trustedCertificates * * @return set of trustedCertificates @@ -116,12 +119,11 @@ public class SecurityManager { /** * Cleans certificate collection */ - public void cleanTrustedCertificates(){ + public void cleanTrustedCertificates() { trustedCertificates.clear(); } /** - * * Verifies if packaged signed with trusted certificate * * @param messageSyntaxSignature - signature data in cms format @@ -131,34 +133,33 @@ public class SecurityManager { * @throws SecurityManagerException */ public boolean verifySignedData(final byte[] messageSyntaxSignature, final byte[] packageCert, - final byte[] innerPackageFile) throws SecurityManagerException{ - try (ByteArrayInputStream signatureStream = new ByteArrayInputStream(messageSyntaxSignature)) { - Object parsedObject = new PEMParser(new InputStreamReader(signatureStream)).readObject(); + final byte[] innerPackageFile) throws SecurityManagerException { + try (ByteArrayInputStream signatureStream = new ByteArrayInputStream(messageSyntaxSignature); + final PEMParser pemParser = new PEMParser(new InputStreamReader(signatureStream))) { + final Object parsedObject = pemParser.readObject(); if (!(parsedObject instanceof ContentInfo)) { throw new SecurityManagerException("Signature is not recognized"); } - ContentInfo signature = ContentInfo.getInstance(parsedObject); - CMSTypedData signedContent = new CMSProcessableByteArray(innerPackageFile); - CMSSignedData signedData = new CMSSignedData(signedContent, signature); - - Collection signers = signedData.getSignerInfos().getSigners(); - SignerInformation firstSigner = signers.iterator().next(); - Store certificates = signedData.getCertificates(); - X509Certificate cert; + final ContentInfo signature = ContentInfo.getInstance(parsedObject); + final CMSTypedData signedContent = new CMSProcessableByteArray(innerPackageFile); + final CMSSignedData signedData = new CMSSignedData(signedContent, signature); + + final Collection signers = signedData.getSignerInfos().getSigners(); + final SignerInformation firstSigner = signers.iterator().next(); + final X509Certificate cert; if (packageCert == null) { - Collection firstSignerCertificates = certificates.getMatches(firstSigner.getSID()); - if(!firstSignerCertificates.iterator().hasNext()){ - throw new SecurityManagerException("No certificate found in cms signature that should contain one!"); + final Collection firstSignerCertificates = signedData.getCertificates() + .getMatches(firstSigner.getSID()); + if (!firstSignerCertificates.iterator().hasNext()) { + throw new SecurityManagerException( + "No certificate found in cms signature that should contain one!"); } - X509CertificateHolder firstSignerFirstCertificate = firstSignerCertificates.iterator().next(); - cert = loadCertificate(firstSignerFirstCertificate.getEncoded()); + cert = loadCertificate(firstSignerCertificates.iterator().next().getEncoded()); } else { cert = loadCertificate(packageCert); } - PKIXCertPathBuilderResult result = verifyCertificate(cert, getTrustedCertificates()); - - if (result == null) { + if (verifyCertificate(cert, getTrustedCertificates()) == null) { return false; } @@ -166,7 +167,7 @@ public class SecurityManager { } catch (OperatorCreationException | IOException | CMSException e) { logger.error(e.getMessage(), e); throw new SecurityManagerException("Unexpected error occurred during signature validation!", e); - } catch (GeneralSecurityException e){ + } catch (GeneralSecurityException e) { throw new SecurityManagerException("Could not verify signature!", e); } } @@ -214,36 +215,37 @@ public class SecurityManager { } private PKIXCertPathBuilderResult verifyCertificate(X509Certificate cert, - Set additionalCerts) throws GeneralSecurityException, SecurityManagerException { - if (null == cert) { - throw new SecurityManagerException("The certificate is empty!"); - } + Set additionalCerts) + throws GeneralSecurityException, SecurityManagerException { + if (null == cert) { + throw new SecurityManagerException("The certificate is empty!"); + } - if (isExpired(cert)) { - throw new SecurityManagerException("The certificate expired on: " + cert.getNotAfter()); - } + if (isExpired(cert)) { + throw new SecurityManagerException("The certificate expired on: " + cert.getNotAfter()); + } - if (isSelfSigned(cert)) { - throw new SecurityManagerException("The certificate is self-signed."); - } + if (isSelfSigned(cert)) { + throw new SecurityManagerException("The certificate is self-signed."); + } - Set trustedRootCerts = new HashSet<>(); - Set intermediateCerts = new HashSet<>(); - for (X509Certificate additionalCert : additionalCerts) { - if (isSelfSigned(additionalCert)) { - trustedRootCerts.add(additionalCert); - } else { - intermediateCerts.add(additionalCert); - } + Set trustedRootCerts = new HashSet<>(); + Set intermediateCerts = new HashSet<>(); + for (X509Certificate additionalCert : additionalCerts) { + if (isSelfSigned(additionalCert)) { + trustedRootCerts.add(additionalCert); + } else { + intermediateCerts.add(additionalCert); } + } - return verifyCertificate(cert, trustedRootCerts, intermediateCerts); + return verifyCertificate(cert, trustedRootCerts, intermediateCerts); } private PKIXCertPathBuilderResult verifyCertificate(X509Certificate cert, Set allTrustedRootCerts, Set allIntermediateCerts) - throws GeneralSecurityException { + throws GeneralSecurityException { // Create the selector that specifies the starting certificate X509CertSelector selector = new X509CertSelector(); @@ -272,13 +274,15 @@ public class SecurityManager { pkixParams.addCertStore(createCertStore(allIntermediateCerts)); pkixParams.addCertStore(createCertStore(allTrustedRootCerts)); - CertPathBuilder builder = CertPathBuilder.getInstance(CertPathBuilder.getDefaultType(), BouncyCastleProvider.PROVIDER_NAME); + CertPathBuilder builder = CertPathBuilder + .getInstance(CertPathBuilder.getDefaultType(), BouncyCastleProvider.PROVIDER_NAME); return (PKIXCertPathBuilderResult) builder.build(pkixParams); } private CertStore createCertStore(Set certificateSet) throws InvalidAlgorithmParameterException, - NoSuchAlgorithmException, NoSuchProviderException { - return CertStore.getInstance("Collection", new CollectionCertStoreParameters(certificateSet), BouncyCastleProvider.PROVIDER_NAME); + NoSuchAlgorithmException, NoSuchProviderException { + return CertStore.getInstance("Collection", new CollectionCertStoreParameters(certificateSet), + BouncyCastleProvider.PROVIDER_NAME); } private boolean isExpired(X509Certificate cert) { @@ -295,8 +299,8 @@ public class SecurityManager { } private boolean isSelfSigned(Certificate cert) - throws CertificateException, NoSuchAlgorithmException, - NoSuchProviderException { + throws CertificateException, NoSuchAlgorithmException, + NoSuchProviderException { try { // Try to verify certificate signature with its own public key PublicKey key = cert.getPublicKey(); -- cgit 1.2.3-korg