Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Prevent header injection with MIME4J DOM #91

Merged
merged 1 commit into from Jan 5, 2024

Conversation

chibenwa
Copy link
Contributor

No description provided.

@chibenwa chibenwa self-assigned this Dec 28, 2023
@jochenw
Copy link
Contributor

jochenw commented Dec 30, 2023

Is it really necessary, to pull in a new dependency (assert4j) to do stuff, that JUnit does as well? I suggest, that you remove assert4j.

@chibenwa
Copy link
Contributor Author

Hi!
Thanks for the feedback.
As it only is a test dependency, library users are non impacted.
I pulled the dependency in mime4j core for 3 reason:

  • asserting exceptions with junit alone is painful
  • we are using already assertj in mime4j dom
  • and this library is used everywhere in james code base as well.
    As such using it in this context seems reasonbable to me.

@chibenwa chibenwa merged commit 9dec5df into apache:master Jan 5, 2024
1 check passed
@carnil
Copy link

carnil commented Feb 28, 2024

This should be CVE-2024-21742

@jim-krueger
Copy link

jim-krueger commented Mar 8, 2024

Hi,
I'm questioning if this fix is actually complete. In this PR you've added validation code the the RawField public constructor. And while the public constructor does invoke the default constructor there is no validation in that constructor. So any code that directly invokes the default constructor could still experience the problem this PR addresses.
Here is an example stack demonstrating my point:

java.lang.Throwable
    at org.apache.james.mime4j.stream.RawField.<init>(RawField.java:47)
    at org.apache.james.mime4j.stream.RawFieldParser.parseField(RawFieldParser.java:71)
    at org.apache.james.mime4j.stream.DefaultFieldBuilder.build(DefaultFieldBuilder.java:95)
    at org.apache.james.mime4j.stream.MimeEntity.nextField(MimeEntity.java:276)
    at org.apache.james.mime4j.stream.MimeEntity.advance(MimeEntity.java:312)
    at org.apache.james.mime4j.stream.MimeTokenStream.next(MimeTokenStream.java:375)
    at org.apache.james.mime4j.parser.MimeStreamParser.parse(MimeStreamParser.java:177)

@chibenwa
Copy link
Contributor Author

chibenwa commented Mar 8, 2024

Thanks for raising the point.

And while the public constructor does invoke the default constructor there is no validation in that constructor.

I protected the constructors called by the DOM builders.

The others are called by the parsers that would naturally consider newlines as new header.

I must confess I did not do fuzzing on this and only wrote simple unit tests. If you happen to find one faulty value please provide (ideally to private@james.apache.org) a reproduction unit test (and a fix?)

I've been voluntarily limiting the scope of the change in order to avoid a regression on "mime4j core" as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants