Merge pull request #1433 from fl4via/UNDERTOW-2211

[UNDERTOW-2211] Fix deny uncovered methods handling
undertow-io · Jan 25, 2023 · 98bef0d · 98bef0d
2 parents 5aaa607 + 8785943
commit 98bef0d
Show file tree

Hide file tree

Showing 4 changed files with 358 additions and 5 deletions.
diff --git a/servlet/src/main/java/io/undertow/servlet/handlers/security/SecurityPathMatches.java b/servlet/src/main/java/io/undertow/servlet/handlers/security/SecurityPathMatches.java
@@ -75,17 +75,27 @@ private SecurityPathMatches(final boolean denyUncoveredHttpMethods, final PathSe
      * @return <code>true</code> If no security path information has been defined
      */
     public boolean isEmpty() {
-        return defaultPathSecurityInformation.excludedMethodRoles.isEmpty() &&
-                defaultPathSecurityInformation.perMethodRequiredRoles.isEmpty() &&
-                defaultPathSecurityInformation.defaultRequiredRoles.isEmpty() &&
+        return isDefaultPathSecurityEmpty() &&
                 exactPathRoleInformation.isEmpty() &&
                 prefixPathRoleInformation.isEmpty() &&
                 extensionRoleInformation.isEmpty();
     }
 
+    // the default security is applied to all http methods of the application
+    private boolean isDefaultPathSecurityEmpty() {
+        return defaultPathSecurityInformation.excludedMethodRoles.isEmpty() &&
+                defaultPathSecurityInformation.perMethodRequiredRoles.isEmpty() &&
+                defaultPathSecurityInformation.defaultRequiredRoles.isEmpty();
+    }
+
     public SecurityPathMatch getSecurityInfo(final String path, final String method) {
         RuntimeMatch currentMatch = new RuntimeMatch();
-        handleMatch(method, defaultPathSecurityInformation, currentMatch);
+        // skip the default path security if it is empty
+        // not only this saves cycles, it also prevents deny uncovered http methods algorithm
+        // from misinterpreting an empty default security as a forbids all
+        if (!isDefaultPathSecurityEmpty()) {
+            handleMatch(method, defaultPathSecurityInformation, currentMatch);
+        }
         PathSecurityInformation match = exactPathRoleInformation.get(path);
         PathSecurityInformation extensionMatch = null;
         if (match != null) {
@@ -184,6 +194,27 @@ private void handleMatch(final String method, final PathSecurityInformation exac
                 transport(currentMatch, role.transportGuaranteeType);
                 currentMatch.constraints.add(new SingleConstraintMatch(role.emptyRoleSemantic, role.roles));
             }
+        } else if (denyUncoveredHttpMethods) {
+            if (exact.perMethodRequiredRoles.size() == 0) {
+                if (exact.excludedMethodRoles.isEmpty()) {
+                    // 13.8.4. When HTTP methods are not enumerated within a security-constraint, the protections defined by the
+                    // constraint apply to the complete set of HTTP (extension) methods.
+                    currentMatch.uncovered = false;
+                } else {
+                    for (ExcludedMethodRoles excluded : exact.excludedMethodRoles) {
+                        if (!excluded.methods.contains(method)) {
+                            currentMatch.uncovered = false;
+                            break;
+                        }
+                    }
+                }
+            }
+            if (currentMatch.uncovered) {
+                //at this point method info is null, but there is match, above if will be triggered for default path, we need to flip it?
+                // keep currentMatch.uncovered value as true (this is the value that is initially set)
+                currentMatch.constraints.clear();
+                currentMatch.constraints.add(new SingleConstraintMatch(SecurityInfo.EmptyRoleSemantic.DENY, new HashSet<>()));
+            }
         }
         for (ExcludedMethodRoles excluded : exact.excludedMethodRoles) {
             if (!excluded.methods.contains(method)) {

diff --git a/...src/main/java/io/undertow/servlet/handlers/security/ServletSecurityConstraintHandler.java b/...src/main/java/io/undertow/servlet/handlers/security/ServletSecurityConstraintHandler.java
@@ -43,6 +43,7 @@ public ServletSecurityConstraintHandler(final SecurityPathMatches securityPathMa
     @Override
     public void handleRequest(final HttpServerExchange exchange) throws Exception {
         final String path = exchange.getRelativePath();
+
         SecurityPathMatch securityMatch = securityPathMatches.getSecurityInfo(path, exchange.getRequestMethod().toString());
         final ServletRequestContext servletRequestContext = exchange.getAttachment(ServletRequestContext.ATTACHMENT_KEY);
         List<SingleConstraintMatch> list = servletRequestContext.getRequiredConstrains();

diff --git a/.../test/java/io/undertow/servlet/test/security/constraint/AuthenticationMessageServlet.java b/.../test/java/io/undertow/servlet/test/security/constraint/AuthenticationMessageServlet.java
@@ -45,6 +45,26 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws S
         doGet(req, resp);
     }
 
+    @Override
+    protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+        doGet(req, resp);
+    }
+
+    @Override
+    protected void doDelete(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+        doGet(req, resp);
+    }
+
+    @Override
+    protected void doOptions(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+        doGet(req, resp);
+    }
+
+    @Override
+    protected void doTrace(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+        doGet(req, resp);
+    }
+
     private void checkExpectedMechanism(HttpServletRequest req) {
         String expectedMechanism = req.getHeader("ExpectedMechanism");
         if (expectedMechanism == null) {
@@ -56,7 +76,7 @@ private void checkExpectedMechanism(HttpServletRequest req) {
             }
         } else if (expectedMechanism.equals("BASIC")) {
             if (req.getAuthType() != HttpServletRequest.BASIC_AUTH) {
-                throw new IllegalStateException("Expected mechanism type not matched.");
+                throw new IllegalStateException("Expected mechanism type not matched: " + req.getAuthType());
             }
         } else {
             throw new IllegalStateException("ExpectedMechanism not recognised.");