Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

public class XssModuleImpl extends SinkModuleBase implements XssModule {

private static final int MAX_LENGTH = 500;

public XssModuleImpl(final Dependencies dependencies) {
super(dependencies);
}
Expand Down Expand Up @@ -61,6 +63,13 @@ public void onXss(@Nonnull CharSequence s, @Nullable String file, int line) {
checkInjection(VulnerabilityType.XSS, s, new FileAndLineLocationSupplier(file, line));
}

private static String truncate(final String s) {

Choose a reason for hiding this comment

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

Should we do it for all vulns, in case this happens in other places?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I recall correctly, we only use the LocationSupplier approach for XSS and Unvalidated redirect, checking the codo for the unvalidated redirects seems that only is used by this advice

public static class SpringAdvice {

  @Advice.OnMethodExit(suppress = Throwable.class)
   @Sink(VulnerabilityTypes.SPRING_RESPONSE)
   public static void checkReturnedObject(
       @Advice.Return HandlerMethodReturnValueHandler handler,
       @Advice.Argument(0) final Object value,
       @Advice.Argument(1) MethodParameter returnType) {
     final UnvalidatedRedirectModule unvalidatedRedirectModule =
         InstrumentationBridge.UNVALIDATED_REDIRECT;
     final XssModule xssModule = InstrumentationBridge.XSS;
     if (handler != null && value != null && returnType != null) {
       String clazz = returnType.getMethod().getDeclaringClass().getName();
       String method = returnType.getMethod().getName();
       if (unvalidatedRedirectModule != null && value instanceof AbstractUrlBasedView) {
         unvalidatedRedirectModule.onRedirect(
             ((AbstractUrlBasedView) value).getUrl(), clazz, method);
       } else if (unvalidatedRedirectModule != null && value instanceof ModelAndView) {
         unvalidatedRedirectModule.onRedirect(((ModelAndView) value).getViewName(), clazz, method);
       } else if (value instanceof String) {
         if (xssModule != null && handler instanceof RequestResponseBodyMethodProcessor) {
           xssModule.onXss((String) value, clazz, method);
         } else if (unvalidatedRedirectModule != null) {
           unvalidatedRedirectModule.onRedirect((String) value, clazz, method);
         }
       }
     }
   }
 }

So IMHO we are safe with this but we can add some default behavior or comment to warn this in the LocationSuplier just to be aware in future implementations

if (s == null || s.length() <= MAX_LENGTH) {
return s;
}
return s.substring(0, MAX_LENGTH);
}

private static class ClassMethodLocationSupplier implements LocationSupplier {
private final String clazz;
private final String method;
Expand All @@ -72,7 +81,7 @@ private ClassMethodLocationSupplier(final String clazz, final String method) {

@Override
public Location build(final @Nullable AgentSpan span) {
return Location.forSpanAndClassAndMethod(span, clazz, method);
return Location.forSpanAndClassAndMethod(span, truncate(clazz), truncate(method));
}
}

Expand All @@ -87,7 +96,7 @@ private FileAndLineLocationSupplier(final String file, final int line) {

@Override
public Location build(@Nullable final AgentSpan span) {
return Location.forSpanAndFileAndLine(span, file, line);
return Location.forSpanAndFileAndLine(span, truncate(file), line);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,45 @@ class XssModuleTest extends IastModuleImplTestBase {
'/==>var<==' | VulnerabilityMarks.SQL_INJECTION_MARK| "/==>var<=="
}

void 'class and method names are truncated when exceeding max length'() {
setup:
final param = mapTainted('/==>value<==', NOT_MARKED)
final clazz = 'c' * 600
final method = 'm' * 600

when:
module.onXss(param, clazz, method)

then:
1 * reporter.report(_, _) >> { args ->
final vuln = args[1] as Vulnerability
assertEvidence(vuln, '/==>value<==')
assert vuln.location.path.length() == 500
assert vuln.location.method.length() == 500
assert vuln.location.path == clazz.substring(0, 500)
assert vuln.location.method == method.substring(0, 500)
}
}

void 'file name is truncated when exceeding max length'() {
setup:
final param = mapTainted('/==>value<==', NOT_MARKED)
final file = 'f' * 600
final line = 42

when:
module.onXss(param as CharSequence, file, line)

then:
1 * reporter.report(_, _) >> { args ->
final vuln = args[1] as Vulnerability
assertEvidence(vuln, '/==>value<==')
assert vuln.location.path.length() == 500
assert vuln.location.line == line
assert vuln.location.path == file.substring(0, 500)
}
}


private String mapTainted(final String value, final int mark) {
final result = addFromTaintFormat(ctx.taintedObjects, value, mark)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,43 @@
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;
import org.thymeleaf.context.Context;
import org.thymeleaf.spring5.SpringTemplateEngine;
import org.thymeleaf.templateresolver.StringTemplateResolver;

@Controller
@RequestMapping("/xss")
public class XssController {

private static final String TEMPLATE = "<p th:utext=\"${xss}\">Test!</p>";

private static final String BIG_TEMPLATE =
new String(new char[500]).replace('\0', 'A') + "<p th:utext=\"${xss}\">Test!</p>";

@GetMapping("/utext")
public String utext(@RequestParam(name = "string") String name, Model model) {
model.addAttribute("xss", name);
return "utext";
}

@GetMapping(value = "/string-template", produces = "text/html")
@ResponseBody
public String stringTemplate(@RequestParam(name = "string") String name) {
SpringTemplateEngine engine = new SpringTemplateEngine();
engine.setTemplateResolver(new StringTemplateResolver());
Context context = new Context();
context.setVariable("xss", name);
return engine.process(TEMPLATE, context);
}

@GetMapping(value = "/big-string-template", produces = "text/html")
@ResponseBody
public String bigStringTemplate(@RequestParam(name = "string") String name) {
SpringTemplateEngine engine = new SpringTemplateEngine();
engine.setTemplateResolver(new StringTemplateResolver());
Context context = new Context();
context.setVariable("xss", name);
return engine.process(BIG_TEMPLATE, context);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,50 @@ class IastSpringBootThymeleafSmokeTest extends AbstractIastServerSmokeTest {
final request = new Request.Builder().url(url).get().build()

when:
client.newCall(request).execute()
def response = client.newCall(request).execute()

then:
response.code() == 200
hasVulnerability { vul -> vul.type == 'XSS' && vul.location.path == templateName && vul.location.line == line }

where:
method | param |templateName| line
'utext' | 'test' | 'utext' | 12
}

void 'xss with string template returns html as template name'() {
setup:
final param = '<script>'
final encoded = URLEncoder.encode(param, 'UTF-8')
final url = "http://localhost:${httpPort}/xss/string-template?string=${encoded}"
final request = new Request.Builder().url(url).get().build()

when:
client.newCall(request).execute()

then:
hasVulnerability { vul ->
vul.type == 'XSS' &&
vul.location.path == '<p th:utext="${xss}">Test!</p>' &&
vul.location.line == 1
}
}

void 'xss with string template returns html as template name - truncated'() {
setup:
final param = '<script>'
final encoded = URLEncoder.encode(param, 'UTF-8')
final url = "http://localhost:${httpPort}/xss/big-string-template?string=${encoded}"
final request = new Request.Builder().url(url).get().build()

when:
client.newCall(request).execute()

then:
hasVulnerability { vul ->
vul.type == 'XSS' &&
vul.location.path == 'A'*500 &&
vul.location.line == 1
}
}
}