-
Notifications
You must be signed in to change notification settings - Fork 324
Email HTML Injection detection in IAST #8205
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
Changes from 44 commits
508a671
a0f62f4
59ea624
14df382
700dd63
b4225d2
34ac9fb
bcca415
4b9c23c
14458ed
b548721
4182134
791a5fa
cd9f249
74cebb5
ea76961
e98038d
2111e77
b814fd0
460737d
be50dc3
fba4788
5fb78f1
a1ab334
d64327a
6f8e74f
49291c1
bbf5486
ee69fd5
ac708fe
ba2da19
553c7d8
e3eaf20
95248a2
4993aec
4883918
56b3521
cb9a54f
482a231
69044c0
bccb5ae
990bbb7
6727eff
6337ba5
b8595c7
3384382
c9895be
3f78b54
7950096
f6da333
f9b7617
2ff2278
a8b13ca
c54f206
4fdb7f7
28a67ba
2e7468c
59e42e0
845b2d0
34796da
631b775
86321f6
42b355d
aedcb1e
f0aa378
457e9f6
aac7112
8b17208
3f9815d
73b3cb1
bc708c5
943227d
a37d29a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package com.datadog.iast.sink; | ||
|
|
||
| import com.datadog.iast.Dependencies; | ||
| import com.datadog.iast.model.VulnerabilityType; | ||
| import datadog.trace.api.iast.sink.EmailInjectionModule; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| public class EmailInjectionModuleImpl extends SinkModuleBase implements EmailInjectionModule { | ||
| public EmailInjectionModuleImpl(final Dependencies dependencies) { | ||
| super(dependencies); | ||
| } | ||
|
|
||
| @Override | ||
| public void onSendEmail(@Nullable final Object messageContent) { | ||
| if (messageContent == null) { | ||
| return; | ||
| } | ||
| checkInjection(VulnerabilityType.EMAIL_HTML_INJECTION, messageContent); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| package com.datadog.iast.sink | ||
|
|
||
| import com.datadog.iast.IastModuleImplTestBase | ||
| import com.datadog.iast.Reporter | ||
| import com.datadog.iast.model.Vulnerability | ||
| import com.datadog.iast.model.VulnerabilityType | ||
| import com.datadog.iast.propagation.PropagationModuleImpl | ||
| import datadog.trace.api.iast.SourceTypes | ||
|
|
||
| class EmailInjectionModuleTest extends IastModuleImplTestBase{ | ||
|
|
||
| private EmailInjectionModuleImpl module | ||
|
|
||
| def setup() { | ||
| module = new EmailInjectionModuleImpl(dependencies) | ||
| } | ||
|
|
||
| @Override | ||
| protected Reporter buildReporter() { | ||
| return Mock(Reporter) | ||
| } | ||
|
|
||
| def "test onSendEmail with null messageContent"() { | ||
| when: | ||
| module.onSendEmail(null) | ||
|
|
||
| then: | ||
| noExceptionThrown() | ||
| } | ||
|
|
||
| def "test onSendEmail with non-null messageContent"() { | ||
| given: | ||
| def messageContent = "test message" | ||
| def propagationModule = new PropagationModuleImpl() | ||
| propagationModule.taintObject(messageContent, SourceTypes.NONE) | ||
|
|
||
| when: | ||
| module.onSendEmail(messageContent) | ||
|
|
||
| then: | ||
| 1 * reporter.report(_, _) >> { args -> | ||
| def vulnerability = args[1] as Vulnerability | ||
| vulnerability.type == VulnerabilityType.EMAIL_HTML_INJECTION && | ||
| vulnerability.evidence == messageContent | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,11 @@ public static String afterEscape( | |
| final PropagationModule module = InstrumentationBridge.PROPAGATION; | ||
| if (module != null) { | ||
| try { | ||
| module.taintStringIfTainted(result, input, false, VulnerabilityMarks.XSS_MARK); | ||
| module.taintStringIfTainted( | ||
| result, | ||
| input, | ||
| false, | ||
| VulnerabilityMarks.XSS_MARK | VulnerabilityMarks.EMAIL_HTML_INJECTION_MARK); | ||
|
||
| } catch (final Throwable e) { | ||
| module.onUnexpectedException("afterEscape threw", e); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import groovy.transform.CompileDynamic | |
|
|
||
| import static datadog.trace.api.iast.VulnerabilityMarks.SQL_INJECTION_MARK | ||
| import static datadog.trace.api.iast.VulnerabilityMarks.XSS_MARK | ||
| import static datadog.trace.api.iast.VulnerabilityMarks.EMAIL_HTML_INJECTION_MARK | ||
|
|
||
| @CompileDynamic | ||
| class StringEscapeUtilsCallSiteTest extends AgentTestRunner { | ||
|
|
@@ -27,15 +28,32 @@ class StringEscapeUtilsCallSiteTest extends AgentTestRunner { | |
|
|
||
| then: | ||
| result == expected | ||
| 1 * module.taintStringIfTainted(_ as String, args[0], false, mark) | ||
| 1 * module.taintStringIfTainted(_ as String, args[0], false, XSS_MARK | EMAIL_HTML_INJECTION_MARK) | ||
| 0 * _ | ||
|
|
||
| where: | ||
| method | args | mark | expected | ||
| 'escapeHtml' | ['Ø-This is a quote'] | XSS_MARK | 'Ø-This is a quote' | ||
| 'escapeJava' | ['Ø-This is a quote'] | XSS_MARK | '\\u00D8-This is a quote' | ||
| 'escapeJavaScript' | ['Ø-This is a quote'] | XSS_MARK | '\\u00D8-This is a quote' | ||
| 'escapeXml' | ['Ø-This is a quote'] | XSS_MARK | 'Ø-This is a quote' | ||
| 'escapeSql' | ['Ø-This is a quote'] | SQL_INJECTION_MARK | 'Ø-This is a quote' | ||
| method | args | expected | ||
| 'escapeHtml' | ['Ø-This is a quote'] | 'Ø-This is a quote' | ||
| 'escapeJava' | ['Ø-This is a quote'] | '\\u00D8-This is a quote' | ||
| 'escapeJavaScript' | ['Ø-This is a quote'] | '\\u00D8-This is a quote' | ||
| 'escapeXml' | ['Ø-This is a quote'] | 'Ø-This is a quote' | ||
| } | ||
|
|
||
| void 'test #method sql'() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we cannot reuse the previous test and just add a new mark?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because the mark being tested is EMAIL | XSS and I could not figure out how to escape the pipe character for the test lol
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can do a workaround with this:
As we want to do a bitwise operation we can pass an array and do the bitwise operation inside the test. In my opinion in this way the tests are easier to follow than having them splitted. Nonetheless, if you disagree, you can leave it as it is as it is not a real issue :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might as well do separate tests for separate marks, i think |
||
| given: | ||
| final module = Mock(PropagationModule) | ||
| InstrumentationBridge.registerIastModule(module) | ||
|
|
||
| when: | ||
| final result = TestStringEscapeUtilsSuite.&"$method".call(args) | ||
|
|
||
| then: | ||
| result == expected | ||
| 1 * module.taintStringIfTainted(_ as String, args[0], false, SQL_INJECTION_MARK) | ||
| 0 * _ | ||
|
|
||
| where: | ||
| method | args | expected | ||
| 'escapeSql' | ['Ø-This is a quote'] | 'Ø-This is a quote' | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| muzzle { | ||
| pass { | ||
| coreJdk() | ||
| } | ||
| } | ||
|
|
||
| apply from: "$rootDir/gradle/java.gradle" | ||
| apply plugin: 'call-site-instrumentation' | ||
|
||
|
|
||
| addTestSuiteForDir('latestDepTest', 'test') | ||
|
|
||
| dependencies { | ||
| testRuntimeOnly project(':dd-java-agent:instrumentation:iast-instrumenter') | ||
| implementation 'javax.mail:javax.mail-api:1.4.4' | ||
sezen-datadog marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| latestDepTestImplementation 'javax.mail:javax.mail-api:+' | ||
| testImplementation 'de.saly:javamail-mock2:0.4-beta3' | ||
smola marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| package datadog.trace.instrumentation.javax.mail; | ||
|
|
||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; | ||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; | ||
|
|
||
| import com.google.auto.service.AutoService; | ||
| import datadog.trace.agent.tooling.Instrumenter; | ||
| import datadog.trace.agent.tooling.InstrumenterModule; | ||
| import datadog.trace.api.iast.InstrumentationBridge; | ||
| import datadog.trace.api.iast.Propagation; | ||
| import datadog.trace.api.iast.propagation.PropagationModule; | ||
| import javax.mail.Part; | ||
| import net.bytebuddy.asm.Advice; | ||
| import net.bytebuddy.description.type.TypeDescription; | ||
| import net.bytebuddy.matcher.ElementMatcher; | ||
|
|
||
| @AutoService(InstrumenterModule.class) | ||
| public class JavaxMailBodyInstrumentation extends InstrumenterModule.Iast | ||
| implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice { | ||
|
|
||
| public JavaxMailBodyInstrumentation() { | ||
| super("javax-mail", "javax-mail-body"); | ||
| } | ||
|
|
||
| @Override | ||
| public void methodAdvice(MethodTransformer transformer) { | ||
| transformer.applyAdvice( | ||
| named("setContent"), | ||
| JavaxMailBodyInstrumentation.class.getName() + "$ContentInjectionAdvice"); | ||
| transformer.applyAdvice( | ||
| named("setText"), JavaxMailBodyInstrumentation.class.getName() + "$TextInjectionAdvice"); | ||
| } | ||
|
|
||
| @Override | ||
| public String hierarchyMarkerType() { | ||
| return "javax.mail.Message"; | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher<TypeDescription> hierarchyMatcher() { | ||
| return implementsInterface(named(hierarchyMarkerType())); | ||
| } | ||
|
|
||
| public static class ContentInjectionAdvice { | ||
| @Propagation | ||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| private static void onSetContent( | ||
| @Advice.This Part part, @Advice.Argument(0) final Object content) { | ||
| PropagationModule propagationModule = InstrumentationBridge.PROPAGATION; | ||
| if (propagationModule != null && content != null) { | ||
| propagationModule.taintObjectIfTainted(part, content); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public static class TextInjectionAdvice { | ||
| @Propagation | ||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| private static void onSetText(@Advice.This Part part, @Advice.Argument(0) final String text) { | ||
| PropagationModule propagationModule = InstrumentationBridge.PROPAGATION; | ||
| if (propagationModule != null && text != null) { | ||
| propagationModule.taintObjectIfTainted(part, text); | ||
| } | ||
| } | ||
| } | ||
| } |
sezen-datadog marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| package datadog.trace.instrumentation.javax.mail; | ||
|
|
||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; | ||
|
|
||
| import com.google.auto.service.AutoService; | ||
| import datadog.trace.agent.tooling.Instrumenter; | ||
| import datadog.trace.agent.tooling.InstrumenterModule; | ||
| import datadog.trace.api.iast.InstrumentationBridge; | ||
| import datadog.trace.api.iast.Sink; | ||
| import datadog.trace.api.iast.VulnerabilityTypes; | ||
| import datadog.trace.api.iast.sink.EmailInjectionModule; | ||
| import java.io.IOException; | ||
| import javax.mail.MessagingException; | ||
| import javax.mail.Multipart; | ||
| import javax.mail.Part; | ||
| import net.bytebuddy.asm.Advice; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| @AutoService(InstrumenterModule.class) | ||
| public class JavaxMailInstrumentation extends InstrumenterModule.Iast | ||
| implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
|
|
||
| private static Logger LOGGER = LoggerFactory.getLogger(JavaxMailInstrumentation.class); | ||
|
|
||
| public JavaxMailInstrumentation() { | ||
| super("javax-mail", "javax-mail-transport"); | ||
| } | ||
|
|
||
| @Override | ||
| public void methodAdvice(MethodTransformer transformer) { | ||
| transformer.applyAdvice( | ||
| named("send0"), JavaxMailInstrumentation.class.getName() + "$MailInjectionAdvice"); | ||
Mariovido marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| @Override | ||
| public String instrumentedType() { | ||
| return "javax.mail.Transport"; | ||
| } | ||
|
|
||
| public static class MailInjectionAdvice { | ||
sezen-datadog marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| @Sink(VulnerabilityTypes.EMAIL_HTML_INJECTION) | ||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void onSend(@Advice.Argument(0) final Part message) | ||
| throws MessagingException, IOException { | ||
| EmailInjectionModule emailInjectionModule = InstrumentationBridge.EMAIL_INJECTION; | ||
| if (message != null && message.getContent() != null) { | ||
| if (message.isMimeType("text/html")) { | ||
| emailInjectionModule.onSendEmail(message.getContent()); | ||
| } else if (message.isMimeType("multipart/*")) { | ||
| Multipart parts = (Multipart) message.getContent(); | ||
| for (int i = 0; i < parts.getCount(); i++) { | ||
| if (parts.getBodyPart(i).isMimeType("text/html")) { | ||
| emailInjectionModule.onSendEmail(parts.getBodyPart(i).getContent()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.