From 0fc9c1c9a11eb967b78f9eea09a9fbfbb3fc0d9d Mon Sep 17 00:00:00 2001 From: Javid Khan Date: Sun, 5 Jul 2026 17:25:22 +0530 Subject: [PATCH 1/2] check parser type before instantiating class in createFileEntryParser During listing auto-detection the parser key is taken from the server's SYST reply. A key shaped like a qualified class name was passed to Class.forName and constructed before the type was checked, so any classpath class could be instantiated for its side effects. Verify the class implements FTPFileEntryParser before calling the constructor. --- .../DefaultFTPFileEntryParserFactory.java | 9 +++++--- .../DefaultFTPFileEntryParserFactoryTest.java | 23 +++++++++++++++++-- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactory.java b/src/main/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactory.java index e3771798a..b1a71f280 100644 --- a/src/main/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactory.java +++ b/src/main/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactory.java @@ -119,11 +119,14 @@ private FTPFileEntryParser createFileEntryParser(final String key, final FTPClie if (JAVA_QUALIFIED_NAME_PATTERN.matcher(key).matches()) { try { final Class parserClass = Class.forName(key); + // Check the type before constructing, so a key resolving to some unrelated class + // (e.g. one taken from an untrusted SYST reply) is not instantiated for its side effects. + if (!FTPFileEntryParser.class.isAssignableFrom(parserClass)) { + throw new ParserInitializationException( + parserClass.getName() + " does not implement the interface " + FTPFileEntryParser.class.getCanonicalName()); + } try { parser = (FTPFileEntryParser) parserClass.getConstructor().newInstance(); - } catch (final ClassCastException e) { - throw new ParserInitializationException( - parserClass.getName() + " does not implement the interface " + FTPFileEntryParser.class.getCanonicalName(), e); } catch (final Exception | LinkageError e) { throw new ParserInitializationException("Error initializing parser", e); } diff --git a/src/test/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactoryTest.java b/src/test/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactoryTest.java index 4833d67b0..a72f35658 100644 --- a/src/test/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactoryTest.java +++ b/src/test/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactoryTest.java @@ -29,6 +29,16 @@ import org.junit.jupiter.api.Test; class DefaultFTPFileEntryParserFactoryTest { + + /** A non-parser class whose constructor records that it was invoked. */ + public static final class ConstructorProbe { + static boolean instantiated; + + public ConstructorProbe() { + instantiated = true; + } + } + private void checkParserClass(final FTPFileEntryParserFactory fact, final String key, final Class expected) { final FTPClientConfig config = key == null ? new FTPClientConfig() : new FTPClientConfig(key); final FTPFileEntryParser parser = fact.createFileEntryParser(config); @@ -104,7 +114,7 @@ void testDefaultParserFactory() { factory.createFileEntryParser("org.apache.commons.net.ftp.parser.DefaultFTPFileEntryParserFactory"); fail("Exception should have been thrown. \"DefaultFTPFileEntryParserFactory\" does not implement FTPFileEntryParser"); } catch (final ParserInitializationException pie) { - assertTrue(pie.getCause() instanceof ClassCastException); + assertTrue(pie.getMessage().contains("does not implement the interface"), pie.getMessage()); } try { @@ -112,7 +122,7 @@ void testDefaultParserFactory() { factory.createFileEntryParser("org.apache.commons.net.ftp.parser.FTPFileEntryParserFactory"); fail("ParserInitializationException should have been thrown."); } catch (final ParserInitializationException pie) { - assertTrue(pie.getCause() instanceof ReflectiveOperationException, pie.getCause().toString()); + assertTrue(pie.getMessage().contains("does not implement the interface"), pie.getMessage()); } try { // Class exists, but is abstract @@ -152,4 +162,13 @@ void testDefaultParserFactoryConfig() throws Exception { // Note: exact matching via config is the only way to generate NTFTPEntryParser and OS400FTPEntryParser // using DefaultFTPFileEntryParserFactory } + + @Test + void testNonParserClassIsNotInstantiated() { + final DefaultFTPFileEntryParserFactory factory = new DefaultFTPFileEntryParserFactory(); + ConstructorProbe.instantiated = false; + // A qualified class name can arrive from the server's SYST reply during auto-detection. + assertThrows(ParserInitializationException.class, () -> factory.createFileEntryParser(ConstructorProbe.class.getName())); + assertFalse(ConstructorProbe.instantiated, "a class that is not an FTPFileEntryParser must not be instantiated"); + } } From 66507898b21be7b322f94e2f5e639a8c7bd19327 Mon Sep 17 00:00:00 2001 From: Javid Khan Date: Sun, 5 Jul 2026 18:00:46 +0530 Subject: [PATCH 2/2] load parser class without initialising before the type check Class.forName(key) initialises the class, so an unrelated class named in the key ran its static initialiser before isAssignableFrom rejected it. Load with the three-arg forName (initialize=false) so only a real FTPFileEntryParser is initialised, when it is constructed. Test now asserts the static initialiser does not run. Signed-off-by: Javid Khan --- .../parser/DefaultFTPFileEntryParserFactory.java | 6 +++--- .../DefaultFTPFileEntryParserFactoryTest.java | 16 +++++++++++----- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactory.java b/src/main/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactory.java index b1a71f280..d544a474a 100644 --- a/src/main/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactory.java +++ b/src/main/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactory.java @@ -118,9 +118,9 @@ private FTPFileEntryParser createFileEntryParser(final String key, final FTPClie // Is the key a possible class name? if (JAVA_QUALIFIED_NAME_PATTERN.matcher(key).matches()) { try { - final Class parserClass = Class.forName(key); - // Check the type before constructing, so a key resolving to some unrelated class - // (e.g. one taken from an untrusted SYST reply) is not instantiated for its side effects. + // Load without initialising, so an unrelated class (e.g. one taken from an untrusted SYST reply) + // does not run its static initialiser before the type is checked. + final Class parserClass = Class.forName(key, false, getClass().getClassLoader()); if (!FTPFileEntryParser.class.isAssignableFrom(parserClass)) { throw new ParserInitializationException( parserClass.getName() + " does not implement the interface " + FTPFileEntryParser.class.getCanonicalName()); diff --git a/src/test/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactoryTest.java b/src/test/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactoryTest.java index a72f35658..1068b87cb 100644 --- a/src/test/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactoryTest.java +++ b/src/test/java/org/apache/commons/net/ftp/parser/DefaultFTPFileEntryParserFactoryTest.java @@ -30,12 +30,17 @@ class DefaultFTPFileEntryParserFactoryTest { - /** A non-parser class whose constructor records that it was invoked. */ + /** Set from {@link ConstructorProbe}'s static initialiser; kept here so reading it does not initialise the probe. */ + static boolean probeInitialized; + + /** A non-parser class whose static initialiser records that the class was initialised. */ public static final class ConstructorProbe { - static boolean instantiated; + static { + probeInitialized = true; + } public ConstructorProbe() { - instantiated = true; + // empty } } @@ -166,9 +171,10 @@ void testDefaultParserFactoryConfig() throws Exception { @Test void testNonParserClassIsNotInstantiated() { final DefaultFTPFileEntryParserFactory factory = new DefaultFTPFileEntryParserFactory(); - ConstructorProbe.instantiated = false; + probeInitialized = false; // A qualified class name can arrive from the server's SYST reply during auto-detection. + // Referencing the .class literal does not initialise the probe, so probeInitialized stays false unless the factory triggers it. assertThrows(ParserInitializationException.class, () -> factory.createFileEntryParser(ConstructorProbe.class.getName())); - assertFalse(ConstructorProbe.instantiated, "a class that is not an FTPFileEntryParser must not be instantiated"); + assertFalse(probeInitialized, "a class that is not an FTPFileEntryParser must not be loaded with initialisation"); } }