From 286a976503a2b33ceb367ecc1d62a6b3fc169cdb Mon Sep 17 00:00:00 2001 From: Olaf Lessenich Date: Tue, 25 Jun 2024 00:59:22 +0200 Subject: [PATCH] feat: add publisher agreement compliance check This commit enforces that only extension versions compliant with the publisher agreement can be published. Already existing extensions versions are checked as part of a migration job. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich --- .../eclipse/openvsx/ExtensionProcessor.java | 15 +++++ .../org/eclipse/openvsx/ExtensionService.java | 2 +- .../openvsx/entities/ExtensionVersion.java | 13 ++++- ...allyMaliciousExtensionVersionsService.java | 46 +++++++++++++++ .../openvsx/migration/MigrationRunner.java | 7 +++ ...PotentiallyMaliciousJobRequestHandler.java | 57 +++++++++++++++++++ .../PublishExtensionVersionHandler.java | 6 ++ .../ExtensionVersionJooqRepository.java | 7 +++ .../repositories/RepositoryService.java | 4 ++ .../openvsx/jooq/tables/ExtensionVersion.java | 5 ++ .../records/ExtensionVersionRecord.java | 19 ++++++- ..._ExtensionVersion_PotentiallyMalicious.sql | 11 ++++ .../RepositoryServiceSmokeTest.java | 1 + 13 files changed, 190 insertions(+), 3 deletions(-) create mode 100644 server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java create mode 100644 server/src/main/java/org/eclipse/openvsx/migration/PotentiallyMaliciousJobRequestHandler.java create mode 100644 server/src/main/resources/db/migration/V1_46__ExtensionVersion_PotentiallyMalicious.sql diff --git a/server/src/main/java/org/eclipse/openvsx/ExtensionProcessor.java b/server/src/main/java/org/eclipse/openvsx/ExtensionProcessor.java index 514f1cfeb..0d24f5708 100644 --- a/server/src/main/java/org/eclipse/openvsx/ExtensionProcessor.java +++ b/server/src/main/java/org/eclipse/openvsx/ExtensionProcessor.java @@ -34,6 +34,7 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Collectors; +import java.util.zip.ZipEntry; import java.util.zip.ZipException; import java.util.zip.ZipFile; @@ -562,4 +563,18 @@ public FileResource getVsixManifest(ExtensionVersion extVersion) { vsixManifest.setContent(ArchiveUtil.readEntry(zipFile, VSIX_MANIFEST, ObservationRegistry.NOOP)); return vsixManifest; } + + public boolean isPotentiallyMalicious() { + readInputStream(); + Enumeration entries = zipFile.entries(); + while (entries.hasMoreElements()) { + ZipEntry entry = entries.nextElement(); + if (entry.getExtra() != null) { + logger.warn("Potentially harmful zip entry with extra fields detected: {}", entry.getName()); + return true; + } + } + + return false; + } } \ No newline at end of file diff --git a/server/src/main/java/org/eclipse/openvsx/ExtensionService.java b/server/src/main/java/org/eclipse/openvsx/ExtensionService.java index 5533d38dc..e2d45014b 100644 --- a/server/src/main/java/org/eclipse/openvsx/ExtensionService.java +++ b/server/src/main/java/org/eclipse/openvsx/ExtensionService.java @@ -68,7 +68,7 @@ public ExtensionVersion mirrorVersion(TempFile extensionFile, String signatureNa return download.getExtension(); } - public ExtensionVersion publishVersion(InputStream content, PersonalAccessToken token) { + public ExtensionVersion publishVersion(InputStream content, PersonalAccessToken token) throws ErrorResultException { return Observation.createNotStarted("ExtensionService#publishVersion", observations).observe(() -> { var extensionFile = createExtensionFile(content); var download = doPublish(extensionFile, null, token, TimeUtil.getCurrentUTC(), true); diff --git a/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java b/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java index a5b68fbaa..69da4f162 100644 --- a/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java +++ b/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java @@ -75,6 +75,8 @@ public enum Type { boolean active; + boolean potentiallyMalicious; + String displayName; @Column(length = 2048) @@ -318,6 +320,14 @@ public void setActive(boolean active) { this.active = active; } + public boolean isPotentiallyMalicious() { + return potentiallyMalicious; + } + + public void setPotentiallyMalicious(boolean potentiallyMalicious) { + this.potentiallyMalicious = potentiallyMalicious; + } + public String getDisplayName() { return displayName; } @@ -487,6 +497,7 @@ public boolean equals(Object o) { && preRelease == that.preRelease && preview == that.preview && active == that.active + && potentiallyMalicious == that.potentiallyMalicious && Objects.equals(getId(extension), getId(that.extension)) // use id to prevent infinite recursion && Objects.equals(version, that.version) && Objects.equals(targetPlatform, that.targetPlatform) @@ -517,7 +528,7 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash( id, getId(extension), version, targetPlatform, semver, preRelease, preview, timestamp, getId(publishedWith), - active, displayName, description, engines, categories, tags, extensionKind, license, homepage, repository, + active, potentiallyMalicious, displayName, description, engines, categories, tags, extensionKind, license, homepage, repository, sponsorLink, bugs, markdown, galleryColor, galleryTheme, localizedLanguages, qna, dependencies, bundledExtensions, signatureKeyPair, type ); diff --git a/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java b/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java new file mode 100644 index 000000000..71500c8d3 --- /dev/null +++ b/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java @@ -0,0 +1,46 @@ +/******************************************************************************** + * Copyright (c) 2024 STMicroelectronics and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ +package org.eclipse.openvsx.migration; + +import io.micrometer.observation.ObservationRegistry; +import jakarta.persistence.EntityManager; +import jakarta.transaction.Transactional; +import org.jobrunr.jobs.context.JobRunrDashboardLogger; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.eclipse.openvsx.ExtensionProcessor; +import org.eclipse.openvsx.entities.ExtensionVersion; +import org.eclipse.openvsx.util.NamingUtil; +import org.eclipse.openvsx.util.TempFile; +import org.springframework.stereotype.Component; + +@Component +public class CheckPotentiallyMaliciousExtensionVersionsService { + + protected final Logger logger = new JobRunrDashboardLogger(LoggerFactory.getLogger(PotentiallyMaliciousJobRequestHandler.class)); + + private final EntityManager entityManager; + + public CheckPotentiallyMaliciousExtensionVersionsService(EntityManager entityManager) { + this.entityManager = entityManager; + } + + @Transactional + public void checkPotentiallyMaliciousExtensionVersion(ExtensionVersion extVersion, TempFile extensionFile) { + try(var extProcessor = new ExtensionProcessor(extensionFile, ObservationRegistry.NOOP)) { + boolean isMalicious = extProcessor.isPotentiallyMalicious(); + extVersion.setPotentiallyMalicious(isMalicious); + if (isMalicious) { + logger.warn("Extension version is potentially malicious: {}", NamingUtil.toLogFormat(extVersion)); + } + } + entityManager.merge(extVersion); + } +} diff --git a/server/src/main/java/org/eclipse/openvsx/migration/MigrationRunner.java b/server/src/main/java/org/eclipse/openvsx/migration/MigrationRunner.java index e1b3d1a03..1032cb6c4 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/MigrationRunner.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/MigrationRunner.java @@ -50,6 +50,7 @@ public void run(HandlerJobRequest jobRequest) throws Exception { fixTargetPlatformMigration(); generateSha256ChecksumMigration(); extensionVersionSignatureMigration(); + checkPotentiallyMaliciousExtensionVersions(); } private void extractResourcesMigration() { @@ -93,4 +94,10 @@ private void extensionVersionSignatureMigration() { scheduler.enqueue(new HandlerJobRequest<>(GenerateKeyPairJobRequestHandler.class)); } } + + private void checkPotentiallyMaliciousExtensionVersions() { + var jobName = "CheckPotentiallyMaliciousExtensionVersions"; + var handler = PotentiallyMaliciousJobRequestHandler.class; + repositories.findNotMigratedPotentiallyMalicious().forEach(item -> migrations.enqueueMigration(jobName, handler, item)); + } } diff --git a/server/src/main/java/org/eclipse/openvsx/migration/PotentiallyMaliciousJobRequestHandler.java b/server/src/main/java/org/eclipse/openvsx/migration/PotentiallyMaliciousJobRequestHandler.java new file mode 100644 index 000000000..5a255e843 --- /dev/null +++ b/server/src/main/java/org/eclipse/openvsx/migration/PotentiallyMaliciousJobRequestHandler.java @@ -0,0 +1,57 @@ +/******************************************************************************** + * Copyright (c) 2024 STMicroelectronics and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ +package org.eclipse.openvsx.migration; + +import org.eclipse.openvsx.util.NamingUtil; +import org.jobrunr.jobs.annotations.Job; +import org.jobrunr.jobs.context.JobRunrDashboardLogger; +import org.jobrunr.jobs.lambdas.JobRequestHandler; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Component; + +import java.nio.file.Files; +import java.util.AbstractMap; + +@Component +public class PotentiallyMaliciousJobRequestHandler implements JobRequestHandler { + + protected final Logger logger = new JobRunrDashboardLogger(LoggerFactory.getLogger(PotentiallyMaliciousJobRequestHandler.class)); + + private final MigrationService migrations; + private final CheckPotentiallyMaliciousExtensionVersionsService service; + + public PotentiallyMaliciousJobRequestHandler(MigrationService migrations, CheckPotentiallyMaliciousExtensionVersionsService service) { + this.migrations = migrations; + this.service = service; + } + + @Override + @Job(name = "Check published extensions for potentially malicious vsix file", retries = 3) + public void run(MigrationJobRequest jobRequest) throws Exception { + var download = migrations.getResource(jobRequest); + var extVersion = download.getExtension(); + logger.info("Checking extension version for potentially malicious vsix file: {}", NamingUtil.toLogFormat(extVersion)); + + var content = migrations.getContent(download); + var entry = new AbstractMap.SimpleEntry<>(download, content); + try(var extensionFile = migrations.getExtensionFile(entry)) { + if(Files.size(extensionFile.getPath()) == 0) { + logger.info("Extension file is empty, skipping: {}", download.getName()); + return; + } + + logger.info("Checking vsix file for potentially malicious metadata: {}", download.getName()); + service.checkPotentiallyMaliciousExtensionVersion(extVersion, extensionFile); + } + + } + +} diff --git a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java index 4987421f1..1124686e0 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java @@ -204,6 +204,12 @@ public void publishAsync(FileResource download, TempFile extensionFile, Extensio service.storeDownload(download, extensionFile); service.persistResource(download); try(var processor = new ExtensionProcessor(extensionFile, ObservationRegistry.NOOP)) { + extVersion.setPotentiallyMalicious(processor.isPotentiallyMalicious()); + if (extVersion.isPotentiallyMalicious()) { + logger.warn("Extension version is potentially malicious: {}", NamingUtil.toLogFormat(extVersion)); + return; + } + Consumer consumer = resource -> { service.storeResource(resource); service.persistResource(resource); diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/ExtensionVersionJooqRepository.java b/server/src/main/java/org/eclipse/openvsx/repositories/ExtensionVersionJooqRepository.java index 4b48631ef..4ec04859f 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/ExtensionVersionJooqRepository.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/ExtensionVersionJooqRepository.java @@ -46,6 +46,7 @@ public List findAllActiveByExtensionIdAndTargetPlatform(Collec EXTENSION.NAME, EXTENSION_VERSION.ID, EXTENSION_VERSION.VERSION, + EXTENSION_VERSION.POTENTIALLY_MALICIOUS, EXTENSION_VERSION.TARGET_PLATFORM, EXTENSION_VERSION.PREVIEW, EXTENSION_VERSION.PRE_RELEASE, @@ -411,6 +412,7 @@ private SelectQuery findAllActive() { USER_DATA.PROVIDER, EXTENSION_VERSION.ID, EXTENSION_VERSION.VERSION, + EXTENSION_VERSION.POTENTIALLY_MALICIOUS, EXTENSION_VERSION.TARGET_PLATFORM, EXTENSION_VERSION.PREVIEW, EXTENSION_VERSION.PRE_RELEASE, @@ -531,6 +533,7 @@ private ExtensionVersion toExtensionVersionCommon( extVersion.setDependencies(toList(record.get(extensionVersionMapper.map(EXTENSION_VERSION.DEPENDENCIES)), converter)); extVersion.setBundledExtensions(toList(record.get(extensionVersionMapper.map(EXTENSION_VERSION.BUNDLED_EXTENSIONS)), converter)); extVersion.setSponsorLink(record.get(extensionVersionMapper.map(EXTENSION_VERSION.SPONSOR_LINK))); + extVersion.setPotentiallyMalicious(record.get(extensionVersionMapper.map(EXTENSION_VERSION.POTENTIALLY_MALICIOUS))); if(extension == null) { var namespace = new Namespace(); @@ -637,6 +640,7 @@ public ExtensionVersion findLatest( USER_DATA.PROVIDER, EXTENSION_VERSION.ID, EXTENSION_VERSION.VERSION, + EXTENSION_VERSION.POTENTIALLY_MALICIOUS, EXTENSION_VERSION.TARGET_PLATFORM, EXTENSION_VERSION.PREVIEW, EXTENSION_VERSION.PRE_RELEASE, @@ -736,6 +740,7 @@ public List findLatest(UserData user) { latestQuery.addSelect( EXTENSION_VERSION.ID, EXTENSION_VERSION.VERSION, + EXTENSION_VERSION.POTENTIALLY_MALICIOUS, EXTENSION_VERSION.TARGET_PLATFORM, EXTENSION_VERSION.PREVIEW, EXTENSION_VERSION.PRE_RELEASE, @@ -780,6 +785,7 @@ public List findLatest(UserData user) { EXTENSION.LAST_UPDATED_DATE, EXTENSION.ACTIVE, latest.field(EXTENSION_VERSION.ID), + latest.field(EXTENSION_VERSION.POTENTIALLY_MALICIOUS), latest.field(EXTENSION_VERSION.VERSION), latest.field(EXTENSION_VERSION.TARGET_PLATFORM), latest.field(EXTENSION_VERSION.PREVIEW), @@ -909,6 +915,7 @@ public ExtensionVersion find(String namespaceName, String extensionName, String EXTENSION.LAST_UPDATED_DATE, EXTENSION_VERSION.ID, EXTENSION_VERSION.VERSION, + EXTENSION_VERSION.POTENTIALLY_MALICIOUS, EXTENSION_VERSION.TARGET_PLATFORM, EXTENSION_VERSION.PREVIEW, EXTENSION_VERSION.PRE_RELEASE, diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java index de7bbe7a9..9fe060b31 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java @@ -495,6 +495,10 @@ public Streamable findNotMigratedSha256Checksums() { return findNotMigratedItems("V1_35__FileResource_Generate_Sha256_Checksum.sql"); } + public Streamable findNotMigratedPotentiallyMalicious() { + return findNotMigratedItems("V1_46__ExtensionVersion_PotentiallyMalicious.sql"); + } + private Streamable findNotMigratedItems(String migrationScript) { return migrationItemRepo.findByMigrationScriptAndMigrationScheduledFalseOrderById(migrationScript); } diff --git a/server/src/main/jooq-gen/org/eclipse/openvsx/jooq/tables/ExtensionVersion.java b/server/src/main/jooq-gen/org/eclipse/openvsx/jooq/tables/ExtensionVersion.java index 4a57c0fb3..0f49a9f43 100644 --- a/server/src/main/jooq-gen/org/eclipse/openvsx/jooq/tables/ExtensionVersion.java +++ b/server/src/main/jooq-gen/org/eclipse/openvsx/jooq/tables/ExtensionVersion.java @@ -224,6 +224,11 @@ public Class getRecordType() { */ public final TableField UNIVERSAL_TARGET_PLATFORM = createField(DSL.name("universal_target_platform"), SQLDataType.BOOLEAN, this, ""); + /** + * The column publc.extension_version.potentially_malicious. + */ + public final TableField POTENTIALLY_MALICIOUS = createField(DSL.name("potentially_malicious"), SQLDataType.BOOLEAN, this, ""); + private ExtensionVersion(Name alias, Table aliased) { this(alias, aliased, null); } diff --git a/server/src/main/jooq-gen/org/eclipse/openvsx/jooq/tables/records/ExtensionVersionRecord.java b/server/src/main/jooq-gen/org/eclipse/openvsx/jooq/tables/records/ExtensionVersionRecord.java index 927494f36..eb8b598a9 100644 --- a/server/src/main/jooq-gen/org/eclipse/openvsx/jooq/tables/records/ExtensionVersionRecord.java +++ b/server/src/main/jooq-gen/org/eclipse/openvsx/jooq/tables/records/ExtensionVersionRecord.java @@ -511,6 +511,22 @@ public Boolean getUniversalTargetPlatform() { return (Boolean) get(34); } + /** + * Setter for + * public.extension_version.potentially_malicious. + */ + public void setPotentiallyMalicious(Boolean value) { + set(35, value); + } + + /** + * Getter for + * public.extension_version.potentially_malicious. + */ + public Boolean getPotentiallyMalicious() { + return (Boolean) get(35); + } + // ------------------------------------------------------------------------- // Primary key information // ------------------------------------------------------------------------- @@ -534,7 +550,7 @@ public ExtensionVersionRecord() { /** * Create a detached, initialised ExtensionVersionRecord */ - public ExtensionVersionRecord(Long id, String bugs, String description, String displayName, String galleryColor, String galleryTheme, String homepage, String license, String markdown, Boolean preview, String qna, String repository, LocalDateTime timestamp, String version, Long extensionId, Long publishedWithId, Boolean active, String dependencies, String bundledExtensions, String engines, String categories, String tags, String extensionKind, Boolean preRelease, String targetPlatform, String localizedLanguages, String sponsorLink, Long signatureKeyPairId, Integer semverMajor, Integer semverMinor, Integer semverPatch, String semverPreRelease, Boolean semverIsPreRelease, String semverBuildMetadata, Boolean universalTargetPlatform) { + public ExtensionVersionRecord(Long id, String bugs, String description, String displayName, String galleryColor, String galleryTheme, String homepage, String license, String markdown, Boolean preview, String qna, String repository, LocalDateTime timestamp, String version, Long extensionId, Long publishedWithId, Boolean active, String dependencies, String bundledExtensions, String engines, String categories, String tags, String extensionKind, Boolean preRelease, String targetPlatform, String localizedLanguages, String sponsorLink, Long signatureKeyPairId, Integer semverMajor, Integer semverMinor, Integer semverPatch, String semverPreRelease, Boolean semverIsPreRelease, String semverBuildMetadata, Boolean universalTargetPlatform, Boolean potentiallyMalicious) { super(ExtensionVersion.EXTENSION_VERSION); setId(id); @@ -572,6 +588,7 @@ public ExtensionVersionRecord(Long id, String bugs, String description, String d setSemverIsPreRelease(semverIsPreRelease); setSemverBuildMetadata(semverBuildMetadata); setUniversalTargetPlatform(universalTargetPlatform); + setPotentiallyMalicious(potentiallyMalicious); resetChangedOnNotNull(); } } diff --git a/server/src/main/resources/db/migration/V1_46__ExtensionVersion_PotentiallyMalicious.sql b/server/src/main/resources/db/migration/V1_46__ExtensionVersion_PotentiallyMalicious.sql new file mode 100644 index 000000000..d66961e7e --- /dev/null +++ b/server/src/main/resources/db/migration/V1_46__ExtensionVersion_PotentiallyMalicious.sql @@ -0,0 +1,11 @@ +ALTER TABLE extension_version ADD COLUMN potentially_malicious BOOLEAN; +UPDATE extension_version SET potentially_malicious = FALSE; + + +INSERT INTO migration_item(id, migration_script, entity_id, migration_scheduled) +SELECT nextval('hibernate_sequence'), 'V1_46__ExtensionVersion_PotentiallyMalicious.sql', fr.id, FALSE +FROM file_resource fr +JOIN extension_version ev ON ev.id = fr.extension_id +JOIN extension e ON e.id = ev.extension_id +WHERE fr.type = 'download' +ORDER BY e.download_count DESC; \ No newline at end of file diff --git a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java index 5a3a82e69..8707aa1dd 100644 --- a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java +++ b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java @@ -152,6 +152,7 @@ void testExecuteQueries() { () -> repositories.findNotMigratedVsixManifests(), () -> repositories.findNotMigratedTargetPlatforms(), () -> repositories.findNotMigratedSha256Checksums(), + () -> repositories.findNotMigratedPotentiallyMalicious(), () -> repositories.topMostActivePublishingUsers(1), () -> repositories.topNamespaceExtensions(1), () -> repositories.topNamespaceExtensionVersions(1),