From 948555a44fa19787c0fec9c0cd0ab4bd739f8ae9 Mon Sep 17 00:00:00 2001 From: bbottema Date: Sun, 12 Jul 2020 17:14:59 +0200 Subject: [PATCH] #271: don't encode filenames anymore, but do make sure all attachment/image file names are tested for injection attacks. Update the master demo --- .../mimemessage/MimeMessageHelper.java | 8 ++--- .../simplejavamail/mailer/MailerHelper.java | 2 ++ .../src/test/java/demo/FullEmailDemoApp.java | 5 +-- .../mimemessage/MimeMessageHelperTest.java | 32 +++++++++---------- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/modules/simple-java-mail/src/main/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageHelper.java b/modules/simple-java-mail/src/main/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageHelper.java index 5bd620f8e..e026cc509 100644 --- a/modules/simple-java-mail/src/main/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageHelper.java +++ b/modules/simple-java-mail/src/main/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageHelper.java @@ -231,8 +231,8 @@ private static BodyPart getBodyPartFromDatasource(final AttachmentResource attac throws MessagingException { final BodyPart attachmentPart = new MimeBodyPart(); // setting headers isn't working nicely using the javax mail API, so let's do that manually - final String resourceName = determineResourceName(attachmentResource, false); - final String fileName = determineResourceName(attachmentResource, true); + final String resourceName = determineResourceName(attachmentResource, false, true); + final String fileName = determineResourceName(attachmentResource, true, false); attachmentPart.setDataHandler(new DataHandler(new NamedDataSource(fileName, attachmentResource.getDataSource()))); attachmentPart.setFileName(fileName); final String contentType = attachmentResource.getDataSource().getContentType(); @@ -248,7 +248,7 @@ private static BodyPart getBodyPartFromDatasource(final AttachmentResource attac /** * Determines the right resource name and optionally attaches the correct extension to the name. The result is mime encoded. */ - static String determineResourceName(final AttachmentResource attachmentResource, final boolean includeExtension) { + static String determineResourceName(final AttachmentResource attachmentResource, final boolean includeExtension, final boolean encodeResourceName) { final String datasourceName = attachmentResource.getDataSource().getName(); String resourceName; @@ -265,7 +265,7 @@ static String determineResourceName(final AttachmentResource attachmentResource, } else if (!includeExtension && resourceName.contains(".") && resourceName.equals(datasourceName)) { resourceName = removeExtension(resourceName); } - return MiscUtil.encodeText(resourceName); + return encodeResourceName ? MiscUtil.encodeText(resourceName) : resourceName; } @NotNull diff --git a/modules/simple-java-mail/src/main/java/org/simplejavamail/mailer/MailerHelper.java b/modules/simple-java-mail/src/main/java/org/simplejavamail/mailer/MailerHelper.java index ac038985e..620c44dbd 100644 --- a/modules/simple-java-mail/src/main/java/org/simplejavamail/mailer/MailerHelper.java +++ b/modules/simple-java-mail/src/main/java/org/simplejavamail/mailer/MailerHelper.java @@ -84,9 +84,11 @@ public static boolean validate(@NotNull final Email email, @NotNull final EnumSe } for (final AttachmentResource attachment : email.getAttachments()) { scanForInjectionAttack(attachment.getName(), "email.attachment.name"); + scanForInjectionAttack(attachment.getDataSource().getName(), "email.attachment.datasource.name"); } for (final AttachmentResource embeddedImage : email.getEmbeddedImages()) { scanForInjectionAttack(embeddedImage.getName(), "email.embeddedImage.name"); + scanForInjectionAttack(embeddedImage.getDataSource().getName(), "email.embeddedImage.datasource.name"); } scanForInjectionAttack(email.getFromRecipient().getName(), "email.fromRecipient.name"); scanForInjectionAttack(email.getFromRecipient().getAddress(), "email.fromRecipient.address"); diff --git a/modules/simple-java-mail/src/test/java/demo/FullEmailDemoApp.java b/modules/simple-java-mail/src/test/java/demo/FullEmailDemoApp.java index 668929cc2..ae353cd0a 100644 --- a/modules/simple-java-mail/src/test/java/demo/FullEmailDemoApp.java +++ b/modules/simple-java-mail/src/test/java/demo/FullEmailDemoApp.java @@ -10,7 +10,7 @@ import javax.mail.internet.MimeMessage; import javax.mail.util.ByteArrayDataSource; import java.io.IOException; -import java.nio.charset.Charset; +import static java.nio.charset.Charset.defaultCharset; import static demo.ResourceFolderHelper.determineResourceFolder; @@ -63,7 +63,8 @@ private static void testMixedRelatedAlternativeIncludingCalendarAndMessageParsin // add two text files in different ways and a black thumbs up embedded image -> emailPopulatingBuilderNormal.withAttachment("dresscode.txt", new ByteArrayDataSource("Black Tie Optional", "text/plain")); - emailPopulatingBuilderNormal.withAttachment("location.txt", "On the moon!".getBytes(Charset.defaultCharset()), "text/plain"); + emailPopulatingBuilderNormal.withAttachment("location.txt", "On the moon!".getBytes(defaultCharset()), "text/plain"); + emailPopulatingBuilderNormal.withAttachment("special_łąąśćńółęĄŻŹĆŃÓŁĘ.txt", "doorcode: Ken sent me".getBytes(defaultCharset()), "text/plain"); emailPopulatingBuilderNormal.withEmbeddedImage("thumbsup", produceThumbsUpImage(), "image/png"); emailPopulatingBuilderNormal.withCalendarText(CalendarMethod.REQUEST, CalendarHelper.createCalendarEvent()); diff --git a/modules/simple-java-mail/src/test/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageHelperTest.java b/modules/simple-java-mail/src/test/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageHelperTest.java index 534a8caed..c90d255f0 100644 --- a/modules/simple-java-mail/src/test/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageHelperTest.java +++ b/modules/simple-java-mail/src/test/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageHelperTest.java @@ -45,64 +45,64 @@ public void setup() { public void determineResourceName1() throws IOException { AttachmentResource resource1 = new AttachmentResource(null, getDataSource("blahblah")); - assertThat(MimeMessageHelper.determineResourceName(resource1, false)).isEqualTo("blahblah"); - assertThat(MimeMessageHelper.determineResourceName(resource1, true)).isEqualTo("blahblah"); + assertThat(MimeMessageHelper.determineResourceName(resource1, false, true)).isEqualTo("blahblah"); + assertThat(MimeMessageHelper.determineResourceName(resource1, true, true)).isEqualTo("blahblah"); } @Test public void determineResourceName2() throws IOException { AttachmentResource resource2 = new AttachmentResource(null, getDataSource("blahblah.txt")); - assertThat(MimeMessageHelper.determineResourceName(resource2, false)).isEqualTo("blahblah"); - assertThat(MimeMessageHelper.determineResourceName(resource2, true)).isEqualTo("blahblah.txt"); + assertThat(MimeMessageHelper.determineResourceName(resource2, false, true)).isEqualTo("blahblah"); + assertThat(MimeMessageHelper.determineResourceName(resource2, true, true)).isEqualTo("blahblah.txt"); } @Test public void determineResourceName3() throws IOException { AttachmentResource resource3 = new AttachmentResource("the resource", getDataSource(null)); - assertThat(MimeMessageHelper.determineResourceName(resource3, false)).isEqualTo("the resource"); - assertThat(MimeMessageHelper.determineResourceName(resource3, true)).isEqualTo("the resource"); + assertThat(MimeMessageHelper.determineResourceName(resource3, false, true)).isEqualTo("the resource"); + assertThat(MimeMessageHelper.determineResourceName(resource3, true, true)).isEqualTo("the resource"); } @Test public void determineResourceName4() throws IOException { AttachmentResource resource4 = new AttachmentResource("the resource", getDataSource("blahblah.txt")); - assertThat(MimeMessageHelper.determineResourceName(resource4, false)).isEqualTo("the resource"); - assertThat(MimeMessageHelper.determineResourceName(resource4, true)).isEqualTo("the resource.txt"); + assertThat(MimeMessageHelper.determineResourceName(resource4, false, true)).isEqualTo("the resource"); + assertThat(MimeMessageHelper.determineResourceName(resource4, true, true)).isEqualTo("the resource.txt"); } @Test public void determineResourceName5() throws IOException { AttachmentResource resource5 = new AttachmentResource("the resource", getDataSource("blahblah")); - assertThat(MimeMessageHelper.determineResourceName(resource5, false)).isEqualTo("the resource"); - assertThat(MimeMessageHelper.determineResourceName(resource5, true)).isEqualTo("the resource"); + assertThat(MimeMessageHelper.determineResourceName(resource5, false, true)).isEqualTo("the resource"); + assertThat(MimeMessageHelper.determineResourceName(resource5, true, true)).isEqualTo("the resource"); } @Test public void determineResourceName6() throws IOException { AttachmentResource resource6 = new AttachmentResource("the resource.txt", getDataSource("blahblah.txt")); - assertThat(MimeMessageHelper.determineResourceName(resource6, false)).isEqualTo("the resource.txt"); - assertThat(MimeMessageHelper.determineResourceName(resource6, true)).isEqualTo("the resource.txt"); + assertThat(MimeMessageHelper.determineResourceName(resource6, false, true)).isEqualTo("the resource.txt"); + assertThat(MimeMessageHelper.determineResourceName(resource6, true, true)).isEqualTo("the resource.txt"); } @Test public void determineResourceName7() throws IOException { AttachmentResource resource7 = new AttachmentResource("the resource.txt", getDataSource("blahblah")); - assertThat(MimeMessageHelper.determineResourceName(resource7, false)).isEqualTo("the resource.txt"); - assertThat(MimeMessageHelper.determineResourceName(resource7, true)).isEqualTo("the resource.txt"); + assertThat(MimeMessageHelper.determineResourceName(resource7, false, true)).isEqualTo("the resource.txt"); + assertThat(MimeMessageHelper.determineResourceName(resource7, true, true)).isEqualTo("the resource.txt"); } @Test public void determineResourceName_ignoreExtensionFromResource() throws IOException { AttachmentResource resource7 = new AttachmentResource("the resource.txt", getDataSource("blahblah.1/www/get?id=3")); - assertThat(MimeMessageHelper.determineResourceName(resource7, false)).isEqualTo("the resource.txt"); - assertThat(MimeMessageHelper.determineResourceName(resource7, true)).isEqualTo("the resource.txt"); + assertThat(MimeMessageHelper.determineResourceName(resource7, false, true)).isEqualTo("the resource.txt"); + assertThat(MimeMessageHelper.determineResourceName(resource7, true, true)).isEqualTo("the resource.txt"); } private ByteArrayDataSource getDataSource(@Nullable String name)