From d3d922146a1272e0caa919840540d8f93bff7788 Mon Sep 17 00:00:00 2001 From: grimsi <9295182+grimsi@users.noreply.github.com> Date: Wed, 2 Apr 2025 17:37:20 +0200 Subject: [PATCH] Detect tampering with signed plugins more reliably and stop plugin classes from loading in the case of suspected tampering --- .../general/cards/PluginManagementCard.tsx | 73 +++++++++++------ .../plugins/config/PluginConfigService.kt | 10 ++- .../config/PluginConfigValidationResult.kt | 7 ++ ....kt => GameyfinDevelopmentPluginLoader.kt} | 2 +- .../management/GameyfinJarPluginLoader.kt | 31 +++++++ .../management/GameyfinPluginClassLoader.kt | 24 ++++++ .../management/GameyfinPluginManager.kt | 82 +++++++++---------- .../management/PluginManagementEndpoint.kt | 4 +- .../management/PluginManagementService.kt | 56 +++++++------ gameyfin/vite.generated.ts | 2 +- 10 files changed, 194 insertions(+), 97 deletions(-) create mode 100644 gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/config/PluginConfigValidationResult.kt rename gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/{GameyfinPluginLoader.kt => GameyfinDevelopmentPluginLoader.kt} (94%) create mode 100644 gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinJarPluginLoader.kt create mode 100644 gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginClassLoader.kt diff --git a/gameyfin/src/main/frontend/components/general/cards/PluginManagementCard.tsx b/gameyfin/src/main/frontend/components/general/cards/PluginManagementCard.tsx index 71d30f6..7e965a1 100644 --- a/gameyfin/src/main/frontend/components/general/cards/PluginManagementCard.tsx +++ b/gameyfin/src/main/frontend/components/general/cards/PluginManagementCard.tsx @@ -4,13 +4,15 @@ import { PauseCircle, PlayCircle, Power, + Question, QuestionMark, SealCheck, SealQuestion, SealWarning, SlidersHorizontal, StopCircle, - WarningCircle + WarningCircle, + XCircle } from "@phosphor-icons/react"; import {PluginManagementEndpoint} from "Frontend/generated/endpoints"; import PluginDto from "Frontend/generated/de/grimsi/gameyfin/core/plugins/management/PluginDto"; @@ -19,25 +21,29 @@ import React, {ReactNode, useEffect, useState} from "react"; import PluginDetailsModal from "Frontend/components/general/modals/PluginDetailsModal"; import PluginLogo from "Frontend/components/general/PluginLogo"; import PluginTrustLevel from "Frontend/generated/de/grimsi/gameyfin/core/plugins/management/PluginTrustLevel"; +import PluginConfigValidationResult + from "Frontend/generated/de/grimsi/gameyfin/core/plugins/config/PluginConfigValidationResult"; export function PluginManagementCard({plugin, updatePlugin}: { plugin: PluginDto, updatePlugin: (plugin: PluginDto) => void }) { const pluginDetailsModal = useDisclosure(); - const [configValid, setConfigValid] = useState(undefined); + const [configValidationResult, setConfigValidationResult] = useState(undefined); useEffect(() => { - PluginManagementEndpoint.validatePluginConfig(plugin.id).then((response: boolean) => { + PluginManagementEndpoint.validatePluginConfig(plugin.id).then((response: PluginConfigValidationResult | undefined) => { if (response === undefined) return; - setConfigValid(response); + setConfigValidationResult(response); }); }, [pluginDetailsModal.isOpen]); - function borderColor(state: PluginState | undefined): "success" | "warning" | "danger" | "default" { + function borderColor(state: PluginState | undefined, trustLevel: PluginTrustLevel | undefined): "success" | "warning" | "danger" | "default" { + if (trustLevel === PluginTrustLevel.UNTRUSTED) return "danger"; + if (isDisabled(state)) return "warning"; - if (configValid === undefined) return "default"; - if (!configValid) return "danger"; + if (configValidationResult === undefined) return "default"; + if (!configValidationResult) return "danger"; return stateToColor(state); } @@ -62,11 +68,37 @@ export function PluginManagementCard({plugin, updatePlugin}: { return ; case PluginState.FAILED: return ; + case PluginState.UNLOADED: + case PluginState.RESOLVED: + return ; default: return ; } } + function configValidationResultToChip(validationResult: PluginConfigValidationResult | undefined): ReactNode { + switch (validationResult) { + case PluginConfigValidationResult.VALID: + return + + + + + case PluginConfigValidationResult.INVALID: + return + + + + ; + default: + return + + + + + } + } + function trustLevelToBadge(trustLevel: PluginTrustLevel | undefined): React.ReactNode { switch (trustLevel) { case PluginTrustLevel.OFFICIAL: @@ -82,7 +114,7 @@ export function PluginManagementCard({plugin, updatePlugin}: { ; case PluginTrustLevel.UNTRUSTED: - return + return ; default: @@ -117,11 +149,16 @@ export function PluginManagementCard({plugin, updatePlugin}: { // @ts-ignore return ( <> - +
- @@ -145,19 +182,9 @@ export function PluginManagementCard({plugin, updatePlugin}: { {stateToIcon(plugin.state)} - {configValid === undefined ? - - : configValid ? - - - - - : - - - - - + {configValidationResult === undefined ? + : + configValidationResultToChip(configValidationResult) }
diff --git a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/config/PluginConfigService.kt b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/config/PluginConfigService.kt index 4082b0a..9bcc13c 100644 --- a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/config/PluginConfigService.kt +++ b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/config/PluginConfigService.kt @@ -16,7 +16,15 @@ class PluginConfigService( fun getConfigMetadata(pluginId: String): List { log.info { "Getting config metadata for plugin $pluginId" } - val plugin = pluginManager.getPlugin(pluginId).plugin as GameyfinPlugin + + val plugin = try { + pluginManager.getPlugin(pluginId).plugin + } catch (_: NoClassDefFoundError) { + return emptyList() + } + + if (plugin !is GameyfinPlugin) return emptyList() + return plugin.configMetadata } diff --git a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/config/PluginConfigValidationResult.kt b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/config/PluginConfigValidationResult.kt new file mode 100644 index 0000000..181b5ee --- /dev/null +++ b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/config/PluginConfigValidationResult.kt @@ -0,0 +1,7 @@ +package de.grimsi.gameyfin.core.plugins.config + +enum class PluginConfigValidationResult { + VALID, + INVALID, + UNKNWOWN, +} \ No newline at end of file diff --git a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginLoader.kt b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinDevelopmentPluginLoader.kt similarity index 94% rename from gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginLoader.kt rename to gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinDevelopmentPluginLoader.kt index 2a5caf9..fabc5dd 100644 --- a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginLoader.kt +++ b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinDevelopmentPluginLoader.kt @@ -9,7 +9,7 @@ import java.nio.file.Path /** * @see https://stackoverflow.com/questions/73654174/my-application-cant-find-the-extension-with-pf4j */ -class GameyfinPluginLoader( +class GameyfinDevelopmentPluginLoader( pluginManager: PluginManager, private val parentClassLoader: ClassLoader ) : DevelopmentPluginLoader(pluginManager) { diff --git a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinJarPluginLoader.kt b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinJarPluginLoader.kt new file mode 100644 index 0000000..5b9b60d --- /dev/null +++ b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinJarPluginLoader.kt @@ -0,0 +1,31 @@ +package de.grimsi.gameyfin.core.plugins.management + +import org.pf4j.DevelopmentPluginLoader +import org.pf4j.PluginDescriptor +import org.pf4j.PluginManager +import org.pf4j.util.FileUtils +import java.nio.file.Files +import java.nio.file.Path + +/** + * JAR plugin loader using a [GameyfinPluginClassLoader] + */ +class GameyfinJarPluginLoader( + pluginManager: PluginManager +) : DevelopmentPluginLoader(pluginManager) { + + override fun isApplicable(pluginPath: Path): Boolean { + return Files.exists(pluginPath) && FileUtils.isJarFile(pluginPath) + } + + override fun loadPlugin(pluginPath: Path, pluginDescriptor: PluginDescriptor?): ClassLoader { + if (pluginDescriptor == null) { + throw IllegalArgumentException("Plugin descriptor cannot be null") + } + + val pluginClassLoader = GameyfinPluginClassLoader(pluginManager, pluginDescriptor, javaClass.getClassLoader()) + pluginClassLoader.addFile(pluginPath.toFile()) + + return pluginClassLoader + } +} \ No newline at end of file diff --git a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginClassLoader.kt b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginClassLoader.kt new file mode 100644 index 0000000..df8986a --- /dev/null +++ b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginClassLoader.kt @@ -0,0 +1,24 @@ +package de.grimsi.gameyfin.core.plugins.management + +import org.pf4j.PluginClassLoader +import org.pf4j.PluginDescriptor +import org.pf4j.PluginManager + +/** + * Adds custom functionality to the [PluginClassLoader] for Gameyfin (mostly related to JAR signature validation). + */ +class GameyfinPluginClassLoader( + pluginManager: PluginManager, + pluginDescriptor: PluginDescriptor, + parentClassLoader: ClassLoader, +) : PluginClassLoader(pluginManager, pluginDescriptor, parentClassLoader) { + + override fun loadClass(className: String?): Class<*>? { + try { + return super.loadClass(className) + } catch (_: SecurityException) { + } + + return null + } +} \ No newline at end of file diff --git a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginManager.kt b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginManager.kt index cf2f5fa..0e1e3c2 100644 --- a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginManager.kt +++ b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/GameyfinPluginManager.kt @@ -1,6 +1,7 @@ package de.grimsi.gameyfin.core.plugins.management import de.grimsi.gameyfin.core.plugins.config.PluginConfigRepository +import de.grimsi.gameyfin.core.plugins.config.PluginConfigValidationResult import de.grimsi.gameyfin.pluginapi.core.GameyfinPlugin import io.github.oshai.kotlinlogging.KotlinLogging import org.pf4j.* @@ -51,13 +52,10 @@ class GameyfinPluginManager( } override fun createPluginLoader(): PluginLoader { - val compoundPluginLoader = CompoundPluginLoader() - val developmentPluginLoader = GameyfinPluginLoader(this, javaClass.classLoader) - val jarPluginLoader = JarPluginLoader(this) - - return compoundPluginLoader - .add(developmentPluginLoader, this::isDevelopment) - .add(jarPluginLoader, this::isNotDevelopment) + return when (this.isDevelopment) { + true -> GameyfinDevelopmentPluginLoader(this, javaClass.classLoader) + false -> GameyfinJarPluginLoader(this) + } } override fun createPluginStatusProvider(): PluginStatusProvider { @@ -69,9 +67,6 @@ class GameyfinPluginManager( if (pluginWrapper == null || pluginPath == null) return null - // Inject config after loading, before starting - configurePlugin(pluginWrapper) - var pluginManagementEntry = pluginManagementRepository.findByIdOrNull(pluginWrapper.pluginId) if (pluginManagementEntry == null) { @@ -104,6 +99,16 @@ class GameyfinPluginManager( } } + // If the plugin is untrusted, we disable it regardless of the previous state + if (pluginManagementEntry.trustLevel == PluginTrustLevel.UNTRUSTED) { + log.warn { "Plugin ${pluginWrapper.pluginId} is untrusted, disabling" } + pluginManagementEntry.enabled = false + } + + // Inject config after loading and verification, before starting + // Note: If the plugin is untrusted, we don't want to inject the config + if (pluginManagementEntry.trustLevel != PluginTrustLevel.UNTRUSTED) configurePlugin(pluginWrapper) + log.debug { "Plugin ${pluginWrapper.pluginId} verification status: ${pluginManagementEntry.trustLevel}" } pluginManagementRepository.save(pluginManagementEntry) @@ -113,8 +118,17 @@ class GameyfinPluginManager( override fun startPlugin(pluginId: String?): PluginState? { if (pluginId == null) return PluginState.FAILED + val trustLevel = pluginManagementRepository.findByIdOrNull(pluginId)?.trustLevel ?: PluginTrustLevel.UNKNOWN + if (trustLevel == PluginTrustLevel.UNTRUSTED) { + val pluginWrapper = getPlugin(pluginId) + val pluginState = PluginState.UNLOADED + + this.firePluginStateEvent(PluginStateEvent(this, pluginWrapper, pluginState)) + return pluginState + } + // Validate config before starting the plugin - if (!validatePluginConfig(pluginId)) { + if (validatePluginConfig(pluginId) == PluginConfigValidationResult.INVALID) { log.warn { "Plugin $pluginId has invalid configuration" } val pluginWrapper = getPlugin(pluginId) @@ -127,34 +141,8 @@ class GameyfinPluginManager( } override fun startPlugins() { - for (pluginWrapper in resolvedPlugins) { - val pluginState = pluginWrapper.pluginState - if (!pluginState.isDisabled && !pluginState.isStarted) { - - // Validate config before starting the plugin - if (!validatePluginConfig(pluginWrapper.pluginId)) { - log.error { "Plugin ${pluginWrapper.pluginId} has invalid configuration" } - pluginWrapper.pluginState = PluginState.FAILED - - firePluginStateEvent(PluginStateEvent(this, pluginWrapper, pluginState)) - return - } - - try { - log.info { "Start plugin '${getPluginLabel(pluginWrapper.descriptor)}'" } - pluginWrapper.plugin.start() - pluginWrapper.pluginState = PluginState.STARTED - pluginWrapper.failedException = null - startedPlugins.add(pluginWrapper) - } catch (e: Exception) { - pluginWrapper.pluginState = PluginState.FAILED - pluginWrapper.failedException = e - log.error { "Unable to start plugin '${getPluginLabel(pluginWrapper.descriptor)}': $e" } - } finally { - firePluginStateEvent(PluginStateEvent(this, pluginWrapper, pluginState)) - } - } - } + val pluginsToStart = resolvedPlugins.filter { !it.pluginState.isDisabled && !it.pluginState.isStarted } + pluginsToStart.forEach { startPlugin(it.pluginId) } } fun restart(pluginId: String) { @@ -164,12 +152,18 @@ class GameyfinPluginManager( startPlugin(pluginId) } - fun validatePluginConfig(pluginId: String): Boolean { - val plugin = getPlugin(pluginId)?.plugin ?: return false - if (plugin is GameyfinPlugin) { - return plugin.validateConfig() + fun validatePluginConfig(pluginId: String): PluginConfigValidationResult { + val plugin = try { + getPlugin(pluginId)?.plugin + } catch (_: NoClassDefFoundError) { + return PluginConfigValidationResult.UNKNWOWN } - return false + + if (plugin is GameyfinPlugin && plugin.validateConfig()) { + return PluginConfigValidationResult.VALID + } + + return PluginConfigValidationResult.INVALID } private fun configurePlugin(pluginWrapper: PluginWrapper) { diff --git a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/PluginManagementEndpoint.kt b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/PluginManagementEndpoint.kt index 904c088..7c3f5a9 100644 --- a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/PluginManagementEndpoint.kt +++ b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/PluginManagementEndpoint.kt @@ -2,6 +2,7 @@ package de.grimsi.gameyfin.core.plugins.management import com.vaadin.hilla.Endpoint import de.grimsi.gameyfin.core.Role +import de.grimsi.gameyfin.core.plugins.config.PluginConfigValidationResult import jakarta.annotation.security.RolesAllowed @Endpoint @@ -23,7 +24,8 @@ class PluginManagementEndpoint( fun disablePlugin(pluginId: String) = pluginManagementService.disablePlugin(pluginId) - fun validatePluginConfig(pluginId: String): Boolean = pluginManagementService.validatePluginConfig(pluginId) + fun validatePluginConfig(pluginId: String): PluginConfigValidationResult = + pluginManagementService.validatePluginConfig(pluginId) fun setPluginPriority(pluginId: String, priority: Int) = pluginManagementService.setPluginPriority(pluginId, priority) diff --git a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/PluginManagementService.kt b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/PluginManagementService.kt index b060f2d..0d30db3 100644 --- a/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/PluginManagementService.kt +++ b/gameyfin/src/main/kotlin/de/grimsi/gameyfin/core/plugins/management/PluginManagementService.kt @@ -1,7 +1,9 @@ package de.grimsi.gameyfin.core.plugins.management +import de.grimsi.gameyfin.core.plugins.config.PluginConfigValidationResult import de.grimsi.gameyfin.pluginapi.core.GameyfinPlugin import org.pf4j.ExtensionPoint +import org.pf4j.PluginWrapper import org.springframework.data.repository.findByIdOrNull import org.springframework.stereotype.Service import java.io.InputStream @@ -9,37 +11,15 @@ import java.io.InputStream @Service class PluginManagementService( private val pluginManager: GameyfinPluginManager, - private val pluginManagementRepository: PluginManagementRepository + private val pluginManagementRepository: PluginManagementRepository, ) { fun getPluginDtos(): List { - return pluginManager.plugins.map { - val pluginManagementEntry = getPluginManagementEntry(it.pluginId) - PluginDto( - it.pluginId, - it.descriptor.pluginDescription, - it.descriptor.version, - it.descriptor.provider, - (it.plugin as GameyfinPlugin).hasLogo(), - it.pluginState, - pluginManagementEntry.priority, - pluginManagementEntry.trustLevel - ) - } + return pluginManager.plugins.map { toDto(it) } } fun getPluginDto(pluginId: String): PluginDto { val plugin = pluginManager.getPlugin(pluginId) - val pluginManagementEntry = getPluginManagementEntry(pluginId) - return PluginDto( - plugin.pluginId, - plugin.descriptor.pluginDescription, - plugin.descriptor.version, - plugin.descriptor.provider, - (plugin.plugin as GameyfinPlugin).hasLogo(), - plugin.pluginState, - pluginManagementEntry.priority, - pluginManagementEntry.trustLevel - ) + return toDto(plugin) } fun getPluginManagementEntry(pluginId: String): PluginManagementEntry { @@ -73,7 +53,7 @@ class PluginManagementService( pluginManager.disablePlugin(pluginId) } - fun validatePluginConfig(pluginId: String): Boolean { + fun validatePluginConfig(pluginId: String): PluginConfigValidationResult { return pluginManager.validatePluginConfig(pluginId) } @@ -95,4 +75,28 @@ class PluginManagementService( val plugin = pluginManager.getPlugin(pluginId).plugin as GameyfinPlugin return plugin.getLogo() } + + private fun toDto(pluginWrapper: PluginWrapper): PluginDto { + val pluginManagementEntry = getPluginManagementEntry(pluginWrapper.pluginId) + + val hasLogo = try { + when (pluginWrapper.plugin is GameyfinPlugin) { + true -> (pluginWrapper.plugin as GameyfinPlugin).hasLogo() + false -> false + } + } catch (_: NoClassDefFoundError) { + false + } + + return PluginDto( + pluginWrapper.pluginId, + pluginWrapper.descriptor.pluginDescription, + pluginWrapper.descriptor.version, + pluginWrapper.descriptor.provider, + hasLogo, + pluginWrapper.pluginState, + pluginManagementEntry.priority, + pluginManagementEntry.trustLevel + ) + } } \ No newline at end of file diff --git a/gameyfin/vite.generated.ts b/gameyfin/vite.generated.ts index 22fde9f..6bebb28 100644 --- a/gameyfin/vite.generated.ts +++ b/gameyfin/vite.generated.ts @@ -274,7 +274,7 @@ function statsExtracterPlugin(): PluginOption { const frontendFiles: Record = {}; frontendFiles['index.html'] = createHash('sha256').update(customIndexData.replace(/\r\n/g, '\n'), 'utf8').digest('hex'); - const projectFileExtensions = ['.js', '.js.map', '.ts', '.ts.map', '.tsx', '.tsx.map', '.css', '.css.map', '.']; + const projectFileExtensions = ['.js', '.js.map', '.ts', '.ts.map', '.tsx', '.tsx.map', '.css', '.css.map']; const isThemeComponentsResource = (id: string) => id.startsWith(themeOptions.frontendGeneratedFolder.replace(/\\/g, '/'))