From 66f79ea74731abd0a2427a42ed7dc6a360afcab6 Mon Sep 17 00:00:00 2001 From: Yannick Marcon Date: Sat, 11 Jan 2025 20:44:32 +0100 Subject: [PATCH] feat: improved csrf check (#585) * fix: ensure Referer header is provided or check allowed user agent * feat: allow java user agent --- .../src/main/resources/config/application.yml | 3 ++ .../web/rest/config/JerseyConfiguration.java | 2 +- .../web/rest/security/CSRFInterceptor.java | 41 +++++++++++++------ .../agate/web/controller/AdminController.java | 2 +- 4 files changed, 33 insertions(+), 15 deletions(-) diff --git a/agate-core/src/main/resources/config/application.yml b/agate-core/src/main/resources/config/application.yml index 11648206..491e19d6 100644 --- a/agate-core/src/main/resources/config/application.yml +++ b/agate-core/src/main/resources/config/application.yml @@ -45,3 +45,6 @@ login: trialTime: 300 banTime: 300 otpTimeout: 600 + +csrf: + allowed-agents: curl,python,java \ No newline at end of file diff --git a/agate-rest/src/main/java/org/obiba/agate/web/rest/config/JerseyConfiguration.java b/agate-rest/src/main/java/org/obiba/agate/web/rest/config/JerseyConfiguration.java index b1dba3c1..adcda3d4 100644 --- a/agate-rest/src/main/java/org/obiba/agate/web/rest/config/JerseyConfiguration.java +++ b/agate-rest/src/main/java/org/obiba/agate/web/rest/config/JerseyConfiguration.java @@ -37,7 +37,7 @@ public JerseyConfiguration(Environment environment) { ///register(LoggingFilter.class); register(AuthenticationInterceptor.class); register(AuditInterceptor.class); - register(new CSRFInterceptor(environment.acceptsProfiles(Profiles.of(Constants.SPRING_PROFILE_PRODUCTION)), environment.getProperty("csrf.allowed", ""))); + register(new CSRFInterceptor(environment.acceptsProfiles(Profiles.of(Constants.SPRING_PROFILE_PRODUCTION)), environment.getProperty("csrf.allowed", ""), environment.getProperty("csrf.allowed-agents", ""))); // validation errors will be sent to the client property(ServerProperties.BV_SEND_ERROR_IN_RESPONSE, true); } diff --git a/agate-rest/src/main/java/org/obiba/agate/web/rest/security/CSRFInterceptor.java b/agate-rest/src/main/java/org/obiba/agate/web/rest/security/CSRFInterceptor.java index dc11682e..8cdda80f 100644 --- a/agate-rest/src/main/java/org/obiba/agate/web/rest/security/CSRFInterceptor.java +++ b/agate-rest/src/main/java/org/obiba/agate/web/rest/security/CSRFInterceptor.java @@ -32,24 +32,32 @@ public class CSRFInterceptor implements ContainerRequestFilter { private static final String REFERER_HEADER = "Referer"; + private static final String USER_AGENT_HEADER = "User-Agent"; + private static final Pattern localhostPattern = Pattern.compile("^http[s]?://localhost:"); private static final Pattern loopbackhostPattern = Pattern.compile("^http[s]?://127\\.0\\.0\\.1:"); private final boolean productionMode; - private final List csrfAllowed; + private final List csrfAllowedHosts; + + private final List csrfAllowedAgents; - public CSRFInterceptor(boolean productionMode, String csrfAllowed) { + public CSRFInterceptor(boolean productionMode, String csrfAllowedHosts, String csrfAllowedAgents) { this.productionMode = productionMode; - this.csrfAllowed = Strings.isNullOrEmpty(csrfAllowed) ? Lists.newArrayList() : Splitter.on(",").splitToList(csrfAllowed.trim()); + this.csrfAllowedHosts = Strings.isNullOrEmpty(csrfAllowedHosts) ? Lists.newArrayList() : Splitter.on(",").splitToList(csrfAllowedHosts.trim()); + this.csrfAllowedAgents = Strings.isNullOrEmpty(csrfAllowedAgents) ? Lists.newArrayList() : Splitter.on(",").splitToList(csrfAllowedAgents.trim()); } @Override - public void filter(ContainerRequestContext requestContext) throws IOException { - if (!productionMode || csrfAllowed.contains("*")) return; + public void filter(ContainerRequestContext requestContext) + throws IOException { + if (!productionMode || csrfAllowedHosts.contains("*")) return; String host = requestContext.getHeaderString(HOST_HEADER); + if (matchesLocalhost(host)) return; + String referer = requestContext.getHeaderString(REFERER_HEADER); if (referer != null) { String refererHostPort = ""; @@ -60,26 +68,33 @@ public void filter(ContainerRequestContext requestContext) throws IOException { // malformed url } // explicitly ok - if (csrfAllowed.contains(refererHostPort)) return; - - boolean forbidden = false; - if (!matchesLocalhost(host) && !referer.startsWith(String.format("https://%s/", host))) { - forbidden = true; - } + if (csrfAllowedHosts.contains(refererHostPort)) return; + boolean forbidden = !referer.startsWith(String.format("https://%s/", host)); if (forbidden) { log.warn("CSRF detection: Host={}, Referer={}", host, referer); log.info(">> You can add {} to csrf.allowed setting", refererHostPort); throw new ForbiddenException(); } + } else { + String userAgent = requestContext.getHeaderString(USER_AGENT_HEADER); + if (Strings.isNullOrEmpty(userAgent) || !matchesUserAgent(userAgent)) { + log.warn("CSRF detection: Host={}, User-Agent={}", host, userAgent); + log.info(">> Ensure 'Referer' HTTP header is set or allow this 'User-Agent' with 'csrf.allowed-agents' setting"); + throw new ForbiddenException("CSRF error"); + } } } - private boolean matchesLocalhost(String host) { return localhostPattern.matcher(host).matches() || loopbackhostPattern.matcher(host).matches() || host.startsWith("localhost:") || host.startsWith("127.0.0.1:"); } -} + + private boolean matchesUserAgent(String userAgent) { + return csrfAllowedAgents.stream().anyMatch(ua -> userAgent.toLowerCase().contains(ua.toLowerCase())); + } + +} \ No newline at end of file diff --git a/agate-webapp/src/main/java/org/obiba/agate/web/controller/AdminController.java b/agate-webapp/src/main/java/org/obiba/agate/web/controller/AdminController.java index 5eb50257..e5ea01da 100644 --- a/agate-webapp/src/main/java/org/obiba/agate/web/controller/AdminController.java +++ b/agate-webapp/src/main/java/org/obiba/agate/web/controller/AdminController.java @@ -51,7 +51,7 @@ private void includeEntryPoints(ModelAndView mv) throws IOException { for (Resource resource : resources) { String fileName = resource.getFilename(); if (fileName != null && fileName.startsWith("index-")) { - log.info("Quasar entrypoint: {}", fileName); + log.debug("Quasar entrypoint: {}", fileName); if (fileName.endsWith(".js")) { mv.getModel().put("entryPointJS", fileName); } else if (fileName.endsWith(".css")) {