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: use correct character encoding when parsing of MIME parameter value #66

Merged
merged 6 commits into from Nov 22, 2021

Conversation

t4nmoy
Copy link
Contributor

@t4nmoy t4nmoy commented Nov 5, 2021

Hello, I'm proposing fixes for MIME4J-109. As suggested pull64 is closed and this one is created based on the code review. please let me know if any clarification is needed. Thanks

Comment on lines +116 to +126
public void testNonAsciiFilename() throws MimeException {
ContentDispositionField f =
parse("Content-Disposition: attachment;"
+ "\nfilename*0=\"=?UTF-8?Q?3-2_FORPROSJEKT_2-Sheet_-_XXX_A_2_40_?="
+ "\n=?UTF-8?Q?\";"
+ "\nfilename*1=\"201_-_Fasader_nord=C3=B8st_og_nordvest.dwg?=\"");

assertEquals(getMessage(f),
"3-2 FORPROSJEKT 2-Sheet - XXX A 2 40 201 - Fasader nordøst og nordvest.dwg",
f.getFilename());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get this test too for the lenient version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing, but are suggesting to move this test to LenientContentDispositionFieldTest or just replicate the same test in LenientContentDispositionFieldTest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replicate in my opinion.

The best way to go in the future is to have a common "contract" for both the lenient and non lenient implementations but this clearly looks like another topic to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I agree with you

@chibenwa
Copy link
Contributor

chibenwa commented Nov 5, 2021

👍 sorry for messing up.

I was about to fix it....

@chibenwa
Copy link
Contributor

chibenwa commented Nov 5, 2021

Fuzzing:

== Java Exception: java.lang.NullPointerException
	at org.apache.james.mime4j.util.MimeParameterMapping.decodeParameterValue(MimeParameterMapping.java:48)
	at org.apache.james.mime4j.util.MimeParameterMapping.getParameters(MimeParameterMapping.java:33)
	at org.apache.james.mime4j.field.ContentDispositionFieldLenientImpl.parse(ContentDispositionFieldLenientImpl.java:176)
	at org.apache.james.mime4j.field.ContentDispositionFieldLenientImpl.getParameter(ContentDispositionFieldLenientImpl.java:87)
	at org.apache.james.mime4j.field.ContentDispositionFieldLenientImpl.getFilename(ContentDispositionFieldLenientImpl.java:121)
	at DJazzer.fuzzerTestOneInput(DJazzer.java:31)

Input: (in base64)

Owo8PA==

Not encoded:

;
<<

Code of the Fuzzer:

import java.util.Base64;

import org.apache.james.mime4j.codec.DecodeMonitor;
import org.apache.james.mime4j.dom.field.ContentDispositionField;
import org.apache.james.mime4j.field.ContentDispositionFieldLenientImpl;
import org.apache.james.mime4j.stream.RawField;

import com.code_intelligence.jazzer.api.FuzzedDataProvider;

public class DJazzer {
    public static void fuzzerTestOneInput(FuzzedDataProvider data) {
        final String body = data.consumeAsciiString(1024);
        try {
            final ContentDispositionField parse = ContentDispositionFieldLenientImpl.PARSER.parse(
                new RawField("Content-Disposition", body), DecodeMonitor.SILENT);
            parse.getFilename();
        } catch (Exception e) {
            e.printStackTrace();
            System.out.println(Base64.getEncoder().encodeToString(body.getBytes()));
            throw e;
        }
    }
}

Runned with jazzer and a big fat jar with dependencies.

…apping.java


don't parse value if it's null

Co-authored-by: Benoit TELLIER <btellier@linagora.com>
@andreak
Copy link

andreak commented Nov 22, 2021

Does this need any further reviewing before merging?

@chibenwa chibenwa merged commit d6a2db2 into apache:master Nov 22, 2021
@chibenwa
Copy link
Contributor

Does this need any further reviewing before merging?

No we are good. Letting a little more time for people to review (~a week) is generally seen as a good practice within the ASF. Sorry for loosing track of this topic ;-)

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