Check parser type before instantiating class in DefaultFTPFileEntryParserFactory.createFileEntryParser()#403
Conversation
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.
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
@dxbjavid
The class is still loaded and initialized, this doesn't change anything. You'll want to avoid initializing the class if you really want to bullet-proof this. How can this actually be an issue?
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 <dxbjavid@gmail.com>
|
you're right, good catch. the single-arg Class.forName still initialises the class, so the static initialiser ran before isAssignableFrom ever looked at the type. moving the check earlier only stopped the constructor, not the static init. i've switched to the three-arg Class.forName(key, false, loader) so the class is loaded without being initialised. it only gets initialised when a genuine FTPFileEntryParser is actually constructed. the test now asserts the static initialiser doesn't run, and it fails against the initialising forName (expected false, was true), so it guards the real behaviour now. on how much this actually matters: it's narrow. the class has to already be on the client's classpath and have a visible side effect in its static block, and the trigger is the SYST reply during listFiles auto-detection, so a malicious or intercepted server picks which existing class gets initialised. no arbitrary code beyond what's already shipped. more defence-in-depth than a sharp vuln, but loading without init is basically free and keeps remote input from initialising unrelated classes, so it seemed worth tightening. |
|
@dxbjavid
All of the above call |
|
right, that null is the parserKey argument, and it's actually the null that makes this reachable. when createParser gets a null key it falls through to the autodetect branch (the else at ~1236 in FTPClient): it calls getSystemTypeOverride(), which with no FTP_SYSTEM_TYPE property set just returns getSystemType(), and that's the server's SYST reply, _replyLines.get(last).substring(4). that string is then passed straight into createFileEntryParser(systemType). so the value that reaches the factory isn't null, it's whatever the server put in its SYST response. normal servers answer UNIX/WINDOWS/etc and hit the alias path, but if a server replies to SYST with something shaped like a qualified class name (say some.pkg.Foo) it matches JAVA_QUALIFIED_NAME_PATTERN and goes down the Class.forName branch instead. nothing constrains the SYST text to the known aliases. full chain for listFiles(path): initiateListParsing(null, path) -> createParser(null) -> else/autodetect -> getSystemTypeOverride() -> getSystemType() -> SYST reply substring(4) -> createFileEntryParser(systemType). as noted it's narrow (needs a class already on the client classpath with a side effect in its static block), so it's more defence-in-depth than a sharp vuln, but loading without init keeps a remote SYST reply from initialising unrelated classes. |
garydgregory
changed the title
garydgregory
changed the title
|
Ok, I see it now, thank you for the clarification @dxbjavid , merged 🚀 |
DefaultFTPFileEntryParserFactory.createFileEntryParser() (#403).
|
@dxbjavid |
|
sure, happy to take a look. from a quick read it looks like a Net-side gap rather than pure VFS: our autodetect only flips the control encoding to UTF-8 when the server advertises it in FEAT (the hasFeature("UTF8") check in connectAction), and IIS notably doesn't list UTF8 in FEAT even though it accepts OPTS UTF8 ON. so setAutodetectUTF8 quietly does nothing there and the filenames come back mis-decoded. i'll dig into it properly on the VFS issue and see whether the right fix is on the Net side (optionally sending OPTS UTF8 ON) or in how VFS drives the client, and follow up there. |
2 participants
Untrusted class name from the SYST reply is instantiated during listing auto-detection
When
FTPClient.listFiles()runs without an explicit parser key or config it auto-detects the parser from the server'sSYSTreply, andDefaultFTPFileEntryParserFactorytreats a reply that looks like a qualified class name as a class to load. It callsClass.forNameand thengetConstructor().newInstance(), only catching theClassCastExceptionafterwards, so the constructor and static initialiser of any class on the client's classpath run before the type is ever checked. A malicious or intercepted server can reply toSYSTwith the name of an arbitrary classpath class and have it built for its side effects (CWE-470). The impact is limited to classes already present, but it is still an unintended instantiation driven by remote input.The type check is moved ahead of construction with
isAssignableFrom, so unrelated classes are rejected before their constructor runs. The documented behaviour of passing your own parser class name is unchanged, since real parser classes still load and construct as before.mvn; that'smvnon the command line by itself.