Merge branch 'master' into 'dev'

bpm/bonita-core/bonita-login/src/main/java/org/bonitasoft/engine/core/login/SecuredLoginServiceImpl.java

@@ -41,6 +41,7 @@
 import org.bonitasoft.engine.session.model.SSession;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.stereotype.Service;
 
 /**
@@ -58,7 +59,8 @@ public class SecuredLoginServiceImpl implements LoginService {
     private final ProfileService profileService;
     private final PermissionsBuilder permissionsBuilder;
 
-    public SecuredLoginServiceImpl(final GenericAuthenticationService authenticationService,
+    public SecuredLoginServiceImpl(
+            @Qualifier("entryPointAuthenticationService") final GenericAuthenticationService authenticationService,
             final SessionService sessionService,
             final IdentityService identityService,
             TechnicalUser technicalUser, ProfileService profileService, PermissionsBuilder permissionsBuilder) {

bpm/bonita-core/bonita-process-engine/src/main/resources/bonita-community.xml

@@ -1906,6 +1906,9 @@
         <constructor-arg ref="${authentication.service.ref.name:authenticationService}" />
     </bean>
 
+    <!--  Used to inject the bean referenced by property 'authentication.service.ref.name' in SecuredLoginServiceImpl: -->
+    <alias name="${authentication.service.ref.name:authenticationService}" alias="entryPointAuthenticationService"  />
+
     <bean id="externalIdentityMappingService"
           class="org.bonitasoft.engine.external.identity.mapping.impl.ExternalIdentityMappingServiceImpl">
         <constructor-arg name="persistenceService" ref="persistenceService" />

bpm/bonita-web-server/src/main/java/org/bonitasoft/console/common/server/filter/SanitizerFilter.java

@@ -15,6 +15,7 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -39,6 +40,7 @@
 import org.bonitasoft.console.common.server.preferences.properties.PropertiesFactory;
 import org.owasp.html.PolicyFactory;
 import org.owasp.html.Sanitizers;
+import org.springframework.web.util.HtmlUtils;
 
 /**
  * This class is used to filter malicious payload (e.g. XSS injection) by
@@ -160,11 +162,15 @@ protected Optional<JsonNode> sanitize(JsonNode node) {
         } else if (node.isObject()) {
             AtomicBoolean changed = new AtomicBoolean(false);
             ObjectNode object = (ObjectNode) node;
+            List<Runnable> operationsToPerformAfterIteration = new ArrayList<>();
             object.fields().forEachRemaining(entry -> {
                 var key = entry.getKey();
                 var newKey = sanitizeValueAndPerformAction(key, s -> {
-                    object.remove(key);
-                    object.set(s, entry.getValue());
+                    // can't remove key while iterating, or we get a ConcurrentModificationException
+                    operationsToPerformAfterIteration.add(() -> {
+                        object.remove(key);
+                        object.set(s, entry.getValue());
+                    });
                     changed.set(true);
                 }).orElse(key);
 
@@ -176,6 +182,7 @@ protected Optional<JsonNode> sanitize(JsonNode node) {
                     });
                 }
             });
+            operationsToPerformAfterIteration.forEach(Runnable::run);
             return changed.get() ? Optional.of(object) : Optional.empty();
         } else if (node.isArray()) {
             AtomicBoolean changed = new AtomicBoolean(false);
@@ -227,7 +234,25 @@ private JsonNode getJsonBody(HttpServletRequest request) throws ServletException
      * @return the sanitized value if it has changed
      */
     private Optional<String> sanitizeValueAndPerformAction(String value, Consumer<String> action) {
-        var sanitized = sanitizer.sanitize(value);
+        /*
+         * Sanitize the value.
+         * It's not just about applying the sanitizer...
+         * We want the value to contain unescaped characters, but no script.
+         * To avoid values with multiple escaping, we unescape untill the value does not
+         * change.
+         * Then we apply the sanitizer.
+         * And finally, we unescape once again to get values that the frontend can
+         * easily handle.
+         */
+        var previous = value;
+        var unescaped = HtmlUtils.htmlUnescape(previous);
+        while (!unescaped.equals(previous)) {
+            previous = unescaped;
+            unescaped = HtmlUtils.htmlUnescape(previous);
+        }
+        var sanitized = sanitizer.sanitize(unescaped);
+        sanitized = HtmlUtils.htmlUnescape(sanitized);
+        // check whether value has effectively changed before doing anything
         if (!sanitized.equals(value)) {
             action.accept(sanitized);
             return Optional.of(sanitized);

bpm/bonita-web-server/src/test/java/org/bonitasoft/console/common/server/filter/SanitizerFilterTest.java

@@ -180,15 +180,58 @@ public void setReadListener(ReadListener readListener) {
     public void shouldSanitizeAttackFromBody() throws Exception {
         when(httpRequest.getContentType()).thenReturn("application/JSON");
         when(sanitizerFilter.getAttributesExcluded()).thenReturn(List.of("email", "password"));
-        final String attName = "test";
-        final String saneValue = "Hello <b>World</b>";
-        final String attValue = saneValue
+        // Classic XSS attack in value
+        final String attName1 = "test1";
+        final String saneValue1 = "Hello <b>World</b>";
+        final String attValue1 = saneValue1
                 + "<style>@keyframes slidein {}</style><xss style=\"animation-duration:1s;animation-name:slidein;animation-iteration-count:2\" onwebkitanimationiteration=\"alert(1)\"></xss>";
+        // Another XSS attack in value
+        final String attName2 = "test2";
+        final String saneValue2 = "<div>test</div>";
+        final String attValue2 = "<div onclick=\"alert('test')\">test</div><script>alert('test')</script>";
+        // XSS attack in name
+        final String saneAttName3 = "test3";
+        final String attName3 = saneAttName3 + "<script>alert('test')</script>";
+        final String attValue3 = "value3";
+        // XSS attack as exploited in IE (as seen on
+        // https://github.com/OWASP/java-html-sanitizer/blob/master/docs/html-validation.md#valid-according-to-policy)
+        final String attName4 = "test4";
+        final String saneValue4 = "v4";
+        final String attValue4 = saneValue4 + "<!--if[true]> <script>alert(1337)</script> -->";
+        // XSS attack as exploited in foreign content context (as seen on
+        // https://github.com/OWASP/java-html-sanitizer/blob/master/docs/html-validation.md#valid-according-to-policy)
+        final String attName5 = "test5";
+        final String saneValue5 = "v5";
+        final String attValue5 = saneValue5 + "<![CDATA[ <!-- ]]><script>alert(1337)</script><!-- -->";
+        // Don't break my heart (as seen on
+        // https://github.com/OWASP/java-html-sanitizer/blob/master/docs/html-validation.md#dont-break-my-heart)
+        final String attName6 = "test6";
+        final String saneValue6 = "I <3 Poniez!";
+        // but escaped chars are unescaped anyway...
+        final String attName7 = "test7";
+        final String saneValue7 = "You <3 Poniez 2!";
+        final String attValue7 = "You &lt;3 Poniez 2!";
+        // XSS attack escaped in value
+        final String attName8 = "test8";
+        final String saneValue8 = "value8";
+        final String attValue8 = saneValue8 + "&lt;script>alert('test')</script>";
+
         final String body = String.format("{%n" +
                 "  \"key1\": \"value1\",%n" +
                 "  \"%s\": \"%s\",%n" +
+                "  \"%s\": \"%s\",%n" +
+                "  \"%s\": \"%s\",%n" +
+                "  \"%s\": \"%s\",%n" +
+                "  \"%s\": \"%s\",%n" +
+                "  \"%s\": \"%s\",%n" +
+                "  \"%s\": \"%s\",%n" +
+                "  \"%s\": \"%s\",%n" +
                 "  \"email\": \"[email protected]\"%n" +
-                "}", attName, JSonUtil.escape(attValue));
+                "}",
+                attName1, JSonUtil.escape(attValue1), attName2, JSonUtil.escape(attValue2),
+                attName3, JSonUtil.escape(attValue3), attName4, JSonUtil.escape(attValue4),
+                attName5, JSonUtil.escape(attValue5), attName6, JSonUtil.escape(saneValue6),
+                attName7, JSonUtil.escape(attValue7), attName8, JSonUtil.escape(attValue8));
         var is = new ByteArrayInputStream(body.getBytes());
 
         when(httpRequest.getInputStream()).thenReturn(new ServletInputStream() {
@@ -225,10 +268,19 @@ public void setReadListener(ReadListener readListener) {
             var stringBody = IOUtils.toString(inputStream, r.getCharacterEncoding());
             ObjectMapper mapper = new ObjectMapper();
             var json = mapper.readTree(stringBody);
+            // check normal values
             assertThat(json.get("key1").asText()).isEqualTo("value1");
             assertThat(json.get("email").asText()).isEqualTo("[email protected]");
-            var hackValue = json.get(attName).asText();
-            assertThat(hackValue).isEqualTo(saneValue);
+            // check sanitized values
+            assertThat(json.get(attName1).asText()).isEqualTo(saneValue1);
+            assertThat(json.get(attName2).asText()).isEqualTo(saneValue2);
+            assertThat(json.get(attName3)).isNull();
+            assertThat(json.get(saneAttName3).asText()).isEqualTo(attValue3);
+            assertThat(json.get(attName4).asText()).isEqualTo(saneValue4);
+            assertThat(json.get(attName5).asText()).isEqualTo(saneValue5);
+            assertThat(json.get(attName6).asText()).isEqualTo(saneValue6);
+            assertThat(json.get(attName7).asText()).isEqualTo(saneValue7);
+            assertThat(json.get(attName8).asText()).isEqualTo(saneValue8);
         } catch (IOException e) {
             throw new AssertionError(e);
         }

platform/platform-resources/src/main/resources/platform_portal/security-config.properties

@@ -3,7 +3,7 @@ security.csrf.enabled true
 #Enable/disable the Sanitizer protection activation. This sanitizer protects against multiple attacks such as XSS, but may restrict the use of some character sequences.
 security.sanitizer.enabled true
 #Name of the Attributes excluded from sanitizer protection (comma separated)
-security.sanitizer.exclude userName,email,password,password_confirm
+security.sanitizer.exclude email,password,password_confirm
 #Add  or not the secure flag to the CSRF token cookie (HTTPS only)
 security.csrf.cookie.secure false
 #X-Frame-Options response header value

services/bonita-authentication/src/main/java/org/bonitasoft/engine/authentication/impl/AuthenticationServiceImpl.java

@@ -24,7 +24,7 @@
 import org.bonitasoft.engine.identity.model.SUser;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
+import org.springframework.context.annotation.Lazy;
 import org.springframework.stereotype.Component;
 
 /**
@@ -34,9 +34,8 @@
  * @author Julien Reboul
  * @author Celine Souchet
  */
-
-@ConditionalOnProperty(value = "authentication.service.ref.name", havingValue = "authenticationService", matchIfMissing = true)
 @Component("authenticationService")
+@Lazy
 public class AuthenticationServiceImpl implements GenericAuthenticationService {
 
     private Logger logger = LoggerFactory.getLogger(AuthenticationServiceImpl.class);