Sanitizing payload filename

[citizenTwice]

For review: proposed mitigation of path traversal vulnerability, as per separate discussion vie email with developer Christopher Broderick.

[VboxNick]

Thanks for your contribution. Could you please add some units tests?

[citizenTwice]

Hi Nick, will look into it asap

[VboxNick]

I'm also not quite sure whether "path traversal" should be sneaked or a certain type of exception must be thrown. The exception might reveal the fact of an attack even keeping in mind that the training partner is known.

@uhurusurfa what do you think or prefer?

[citizenTwice]

The vulnerability is currently exploitable for certain configs of MessageFileModule in config.xml so, the exploitability should definitely be addressed. Personally, I do like the idea of the of ringing the bell with a suitable exception. The most likely attack scenario would be a compromised trading partner rather than a malicious one. Another possibility is a bogus filename generated because of bugs or wrong settings at the TP's end, non trivial heuristics would be required to distinguish between this and an actual attack... but warning everybody that something is not right feels like the right thing to do.

[igwtech]

I agree an exception should be thrown but the server should silently log it and not terminate from it but continue working. We definitely want the trace to ring the bell and act upon the error but we can't stop hackers and PTA actors from trying. Therefore uptime should not be affected by this neither feedback should be provided to the bad actor about success or failure of the attack attempt. My 2 cents. J

…

On Sat, Aug 22, 2020 at 5:02 AM LuigiG ***@***.***> wrote: The vulnerability is currently exploitable for certain configs of MessageFileModule in config.xml so, the exploitability should definitely be addressed. Personally, I do like the idea of the of ringing the bell with a suitable exception. The most likely attack scenario would be a compromised trading partner rather than a malicious one. Another possibility is a bogus filename generated because of bugs or wrong settings at the TP's end, non trivial heuristics would be required to distinguish between this and an actual attack... but warning everybody that something is not right feels like the right thing to do. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2QND2BQAQA5XLW7PY2KG3SB6CTJANCNFSM4QHONIJA> .

-- Javier Muñoz CTO Ingeniería | Greicodex Software C.A. (+58) 212-7629120 <(+58)+212-7629120> javier@greicodex.com greicodex.com Av. Francisco Solano con Calle La Iglesia. Torre Centro Solano Plaza I. Piso 9, Oficina PH-A., Urb. Sabana Grande. Municipio Libertador. Dtto. Capital. Código postal 1050. [image: facebook] <http://facebook.com/greicodex> [image: twitter] <https://twitter.com/greicodex>

[VboxNick]

@igwtech Why would you store a malicious file? Even its name has been "silently" sanitized. Somebody might want that an another system reads it or somebody will try to execute it somehow. To me, it makes no sense to such files.

Thus, I propose introduce kind of "filename sanitizer" interface which would be part of the configuration. For instance:

 <module enabled="$properties.module.MessageFileModule.enabled$"
              classname="org.openas2.processor.storage.MessageFileModule"
              filename="%home%/../data/$msg.sender.as2_id$-$msg.receiver.as2_id$/inbox/$msg.content-disposition.filename$-$msg.headers.message-id$"
              filename_sanitizer="org.openas2.support.RejectingFileNameSanitizer"
              header="%home%/../data/$msg.sender.as2_id$-$msg.receiver.as2_id$/msgheaders/$date.yyyy-MM-dd$/$msg.content-disposition.filename$-$msg.headers.message-id$"
              protocol="as2"
              tempdir="%home%/../data/temp"/>

or

 <module enabled="$properties.module.MessageFileModule.enabled$"
               classname="org.openas2.processor.storage.MessageFileModule"
               filename="%home%/../data/$msg.sender.as2_id$-$msg.receiver.as2_id$/inbox/$msg.content-disposition.filename$-$msg.headers.message-id$"
               filename_sanitizer="org.openas2.support.RenamingFileNameSanitizer"
               header="%home%/../data/$msg.sender.as2_id$-$msg.receiver.as2_id$/msgheaders/$date.yyyy-MM-dd$/$msg.content-disposition.filename$-$msg.headers.message-id$"
               protocol="as2"
               tempdir="%home%/../data/temp"/>

[uhurusurfa]

Having reflected on this, I have reviewed the IETF AS2 spec and the related ones that it relies on for guidance.
The received filename should always be strippied of any path information.
The IETF related spec is section 2.3 here:
https://tools.ietf.org/html/rfc2183

Specifically this line:
The receiving MUA SHOULD NOT respect any directory path informationthat may seem to be present in the filename parameter.

So th e method should be changed to ensure all path information is always stripped. from the filename parameter along with any other characters that are deemed unusable for the target system.
It should invoke this method to ensure the filename itself is acceptable after path stripping: org.openas2.utils.IOUtil.cleanFilename()

An exception should only be raised if the filename includes path information and not if the filename itself contains reserved characters since different file systems have different requirements regardin reserved characters.

[citizenTwice]

I think we can (mostly) leverage the JDK to implement the behavior outlined in Chris' comment.

Specifically

1. Use a combination of FileSystem.getPath + catching InvalidPathException to detect illegal characters in filenames.
2. Compare the proposed payload filename with the result of Path.getFileName() to detect directory paths embedded within it.

A quick look at OpenJDK suggests the existing implementations do a good job at dealing with OS-specific conventions&requirements, see the Windows and Unix getFileName implementations

as well as the Windows PathParser catching illegal chars:

// Reserved characters for window path name
    private static final String reservedChars = "<>:\"|?*";
    private static final boolean isInvalidPathChar(char ch) {
        return ch < '\u0020' || reservedChars.indexOf(ch) != -1;
    }

So, please review the below pseudocode:

// tmpFilename = filenamed obtained from payload
// Check for illegal chars
try tmpPath = fileSystem.getPath(tmpFilename)
if InvalidPathException thrown then
  log.warn("invalid chars in filename")
  tmpFilename = org.openas2.utils.IOUtil.cleanFilename()
endif
// Note the below could theoretically throw IPE again... Generate new filename in that case?
try tmpPath = fileSystem.getPath(tmpFilename) 

// No illegal chars, check for directory path in filename
baseName = tmpPath.getFileName()
if NOT baseName.equalsIgnoreCase(tmpFilename) then
    throw IllegalPayloadFilenameException("Payload filename contains directory path", tmpFilename)
endif
// fall-through, everything cool

[igwtech]

I would prefer sanitization checks to be done against whitelist charsets instead of checking for bad characters. There are too many Unicode and multibyte character combinations cases that can be used for transversal that seems simpler to check for only what's allowed instead of what's invalid. Regards, J

…

On Sun, Aug 23, 2020 at 8:18 AM LuigiG ***@***.***> wrote: I think we can (mostly) leverage the JDK to implement the behavior outlined in Chris' comment. Specifically 1. Use a combination of FileSystem.getPath + catching InvalidPathException to detect illegal characters in filenames. 2. Compare the proposed payload filename with the result of Path.getFileName() to detect directory paths embedded within it. A quick look at OpenJDK suggests the existing implementations do a good job at dealing with OS-specific conventions&requirements, see the Windows <https://github.com/openjdk/jdk/blob/39c023612dbdd1016ad24b60fdef464bf21d1016/src/java.base/windows/classes/sun/nio/fs/WindowsPath.java#L310> and Unix <https://github.com/openjdk/jdk/blob/270674ce1b1b8d44bbe92949c3f7db7b7c767cac/src/java.base/unix/classes/sun/nio/fs/UnixPath.java#L285> getFileName implementations as well as the Windows PathParser catching illegal chars: // Reserved characters for window path name private static final String reservedChars = "<>:\"|?*"; private static final boolean isInvalidPathChar(char ch) { return ch < '\u0020' || reservedChars.indexOf(ch) != -1; } So, please review the below pseudocode: // tmpFilename = filenamed obtained from payload // Check for illegal chars try tmpPath = fileSystem.getPath(tmpFilename) if InvalidPathException thrown then log.warn("invalid chars in filename") tmpFilename = org.openas2.utils.IOUtil.cleanFilename() endif // Note the below could theoretically throw IPE again... Generate new filename in that case? try tmpPath = fileSystem.getPath(tmpFilename) // No illegal chars, check for directory path in filename baseName = tmpPath.getFileName() if NOT baseName.equalsIgnoreCase(tmpFilename) throw IllegalPayloadFilenameException(?) endif // fall-through, everything cool — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2QNDYLRFTUGH2526HSYILSCECIPANCNFSM4QHONIJA> .

-- Javier Muñoz CTO Ingeniería | Greicodex Software C.A. (+58) 212-7629120 <(+58)+212-7629120> javier@greicodex.com greicodex.com Av. Francisco Solano con Calle La Iglesia. Torre Centro Solano Plaza I. Piso 9, Oficina PH-A., Urb. Sabana Grande. Municipio Libertador. Dtto. Capital. Código postal 1050. [image: facebook] <http://facebook.com/greicodex> [image: twitter] <https://twitter.com/greicodex>

[citizenTwice]

Hi Javier @igwtech

Yes, for 'own use', I always stick to that approach and restrict the name to [ A-Z 0-9 -_. ] only ,which is enough as far as I'm concerned, esp for EDI. But I'm not so sure about OpenAS2 as it is meant to be generally deployable/usable anywhere in the world. I think we can trust the JRE/JDK here: for path traversal protection I think Path.getFileName is good enough as well as InvalidPathException for catching any illegal characters for a given platform, given how wildly the requirements differ between the various OS, it is unlikely that we would do a better job than the JDK developers if we try to reimplement those checks...
Thanks

[igwtech]

Hi LuigiG, You have a very good point there. I stand corrected. J

…

On Mon, Aug 24, 2020, 12:22 PM LuigiG ***@***.***> wrote: Hi Javier @igwtech <https://github.com/igwtech> Yes, for 'own use', I always stick to that approach and restrict the name to [ A-Z 0-9 -_. ] only ,which is enough as far as I'm concerned, esp for EDI. But I'm not so sure about OpenAS2 as it is meant to be generally deployable/usable anywhere in the world. I think we can trust the JRE/JDK here: for path traversal protection I think Path.getFileName is good enough as well as InvalidPathException for catching any illegal characters for a given platform, given how wildly the requirements differ between the various OS, it is unlikely that we would do a better job than the JDK developers if we try to reimplement those checks... Thanks — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2QNDY3EZZGVBSEO7TROHTSCKHUHANCNFSM4QHONIJA> .

[uhurusurfa]

@citizenTwice - happy with that

[citizenTwice]

@citizenTwice - happy with that

will get coding and add a new commit to this PR

[citizenTwice]

@uhurusurfa @VboxNick @igwtech

Added new commits, please review.

Updates:

The filename sent by partner is now passed to IOUtil::getSafeFilename,
new function that works as follows:

1 - known, bad characters (e.g. 0x0d, 0xa, 0x00) are removed automatically
2 - if the platform-specific nio.file.Path still rejects the cleaned name
InvalidMessageException is thrown
3 - the name is checked for embedded directory paths
4 - if any are found
InvalidMessageException is thrown
5 - otherwise the (cleaned) filename is returned to the caller

I've also tried to add a few unit tests. This turned out to be a bit tricky for me because rejection of some filenames is platform-dependent and the tests need to be deterministic. I've opted to let the tests run only on the platforms where I was able to directly verify the test results myself, that's the three main desktop/server OS's (Mac,Linux,Windows) ...but still, I am unsure as to whether better solutions for handling such cases exist.

And sorry for the delay following-up on this.

[uhurusurfa]

Awesome - thank you for the contribution.

[adioss]

@uhurusurfa sorry to bother you with that but, shouldn't a CVE being created associated to this issue?
Thanks a lot,
Adrien

[uhurusurfa]

@adioss It was not a vulnerability

[adioss]

@uhurusurfa thanks a lot for the feedback. In fact, to be 100% transparent, I'm an AppSec engineer and Snyk reports this as a security issue.

• Please check https://security.snyk.io/vuln/SNYK-JAVA-NETSFOPENAS2-608851 I just asked them "why no CVE?"
• if I may, now I'm wondering: according to the PoC provided in the ticket, why don't you consider this as a security issue?

Adrien

[uhurusurfa]

@adioss - although that getSafeFielname() method was added to parse file names, there was already a filename check done in IOUtil.cleanFilename() that is called further downstream when the received file is actually persisted in the storage module handler and it is still called to this day so there was never a security vulnerability in OpenAS2.
What is claimed in Snyk is true that a filename could be contrived that in theory could do what the reporter "citizenTwice" claims but "citizenTwice" did not actually test that theory at all because if that claimant had, they would have realised that it was not and is not possible in OpenAS2.

[citizenTwice]

@adioss @uhurusurfa just jumping in to confirm what Christopher said about the filename filtering logic.

However, a correction: I did 100% test the vulnerability with that version of OpenAS2, dropping a an arbitrary, executable payload in the bin/ directory.
What, in practice, made exploitation highly unlikely is that it would first require establishing a trading partnership a trusted party that would later have to either turn malicious or get compromised in a very specific way.

[uhurusurfa]

@citizenTwice - Thanks for the clarification. I am surprised and will cross check that the exploit does work without the getSafeFilename() invocation.
@adioss - If true then I agree it should have been a CVE

[citizenTwice]

@uhurusurfa @adioss just LMK if there's anything I can do to help, this was a while ago but I think I could probably dig up my notes & screenshots.