Merge pull request #38414 from gsmet/jaxrs-security-fixes

Assorted JAX-RS security fixes - CVE-2023-6267 and CVE-2023-5675

extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/RestPathAnnotationProcessor.java

@@ -182,7 +182,7 @@ static Optional<AnnotationInstance> searchPathAnnotationOnInterfaces(CombinedInd
      * @param resultAcc accumulator for tail-recursion
      * @return Collection of all interfaces und their parents. Never null.
      */
-    private static Collection<ClassInfo> getAllClassInterfaces(
+    static Collection<ClassInfo> getAllClassInterfaces(
             CombinedIndexBuildItem index,
             Collection<ClassInfo> classInfos,
             List<ClassInfo> resultAcc) {

extensions/resteasy-classic/resteasy/deployment/src/main/java/io/quarkus/resteasy/deployment/ResteasyBuiltinsProcessor.java

@@ -1,18 +1,21 @@
 package io.quarkus.resteasy.deployment;
 
 import static io.quarkus.deployment.annotations.ExecutionTime.STATIC_INIT;
+import static io.quarkus.resteasy.deployment.RestPathAnnotationProcessor.getAllClassInterfaces;
 import static io.quarkus.resteasy.deployment.RestPathAnnotationProcessor.isRestEndpointMethod;
 import static io.quarkus.security.spi.SecurityTransformerUtils.hasSecurityAnnotation;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import java.util.Objects;
-import java.util.Optional;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 import org.jboss.jandex.ClassInfo;
 import org.jboss.jandex.DotName;
 import org.jboss.jandex.MethodInfo;
+import org.jboss.logging.Logger;
 
 import io.quarkus.arc.deployment.AdditionalBeanBuildItem;
 import io.quarkus.deployment.Capabilities;
@@ -34,14 +37,14 @@
 import io.quarkus.resteasy.runtime.JaxRsSecurityConfig;
 import io.quarkus.resteasy.runtime.NotFoundExceptionMapper;
 import io.quarkus.resteasy.runtime.SecurityContextFilter;
+import io.quarkus.resteasy.runtime.StandardSecurityCheckInterceptor;
 import io.quarkus.resteasy.runtime.UnauthorizedExceptionMapper;
 import io.quarkus.resteasy.runtime.vertx.JsonArrayReader;
 import io.quarkus.resteasy.runtime.vertx.JsonArrayWriter;
 import io.quarkus.resteasy.runtime.vertx.JsonObjectReader;
 import io.quarkus.resteasy.runtime.vertx.JsonObjectWriter;
 import io.quarkus.resteasy.server.common.deployment.ResteasyDeploymentBuildItem;
 import io.quarkus.security.spi.AdditionalSecuredMethodsBuildItem;
-import io.quarkus.vertx.http.deployment.EagerSecurityInterceptorBuildItem;
 import io.quarkus.vertx.http.deployment.HttpRootPathBuildItem;
 import io.quarkus.vertx.http.deployment.devmode.NotFoundPageDisplayableEndpointBuildItem;
 import io.quarkus.vertx.http.deployment.devmode.RouteDescriptionBuildItem;
@@ -51,6 +54,7 @@
 public class ResteasyBuiltinsProcessor {
 
     protected static final String META_INF_RESOURCES = "META-INF/resources";
+    private static final Logger LOG = Logger.getLogger(ResteasyBuiltinsProcessor.class);
 
     @BuildStep
     void setUpDenyAllJaxRs(CombinedIndexBuildItem index,
@@ -66,10 +70,42 @@ void setUpDenyAllJaxRs(CombinedIndexBuildItem index,
                 ClassInfo classInfo = index.getIndex().getClassByName(DotName.createSimple(className));
                 if (classInfo == null)
                     throw new IllegalStateException("Unable to find class info for " + className);
-                if (!hasSecurityAnnotation(classInfo)) {
-                    for (MethodInfo methodInfo : classInfo.methods()) {
-                        if (isRestEndpointMethod(index, methodInfo) && !hasSecurityAnnotation(methodInfo)) {
-                            methods.add(methodInfo);
+                // add unannotated class endpoints as well as parent class unannotated endpoints
+                addAllUnannotatedEndpoints(index, classInfo, methods);
+
+                // interface endpoints implemented on resources are already in, now we need to resolve default interface
+                // methods as there, CDI interceptors won't work, therefore neither will our additional secured methods
+                Collection<ClassInfo> interfaces = getAllClassInterfaces(index, List.of(classInfo), new ArrayList<>());
+                if (!interfaces.isEmpty()) {
+                    final List<MethodInfo> interfaceEndpoints = new ArrayList<>();
+                    for (ClassInfo anInterface : interfaces) {
+                        addUnannotatedEndpoints(index, anInterface, interfaceEndpoints);
+                    }
+                    // look for implementors as implementors on resource classes are secured by CDI interceptors
+                    if (!interfaceEndpoints.isEmpty()) {
+                        interfaceBlock: for (MethodInfo interfaceEndpoint : interfaceEndpoints) {
+                            if (interfaceEndpoint.isDefault()) {
+                                for (MethodInfo endpoint : methods) {
+                                    boolean nameParamsMatch = endpoint.name().equals(interfaceEndpoint.name())
+                                            && (interfaceEndpoint.parameterTypes().equals(endpoint.parameterTypes()));
+                                    if (nameParamsMatch) {
+                                        // whether matched method is declared on class that implements interface endpoint
+                                        Predicate<DotName> isEndpointInterface = interfaceEndpoint.declaringClass()
+                                                .name()::equals;
+                                        if (endpoint.declaringClass().interfaceNames().stream().anyMatch(isEndpointInterface)) {
+                                            continue interfaceBlock;
+                                        }
+                                    }
+                                }
+                                String configProperty = config.denyJaxRs ? "quarkus.security.jaxrs.deny-unannotated-endpoints"
+                                        : "quarkus.security.jaxrs.default-roles-allowed";
+                                // this is logging only as I'm a bit worried about false positives and breaking things
+                                // for what is very much edge case
+                                LOG.warn("Default interface method '" + interfaceEndpoint
+                                        + "' cannot be secured with the '" + configProperty
+                                        + "' configuration property. Please implement this method for CDI "
+                                        + "interceptor binding to work");
+                            }
                         }
                     }
                 }
@@ -86,13 +122,33 @@ void setUpDenyAllJaxRs(CombinedIndexBuildItem index,
         }
     }
 
+    private static void addAllUnannotatedEndpoints(CombinedIndexBuildItem index, ClassInfo classInfo,
+            List<MethodInfo> methods) {
+        if (classInfo == null) {
+            return;
+        }
+        addUnannotatedEndpoints(index, classInfo, methods);
+        if (classInfo.superClassType() != null && !classInfo.superClassType().name().equals(DotName.OBJECT_NAME)) {
+            addAllUnannotatedEndpoints(index, index.getIndex().getClassByName(classInfo.superClassType().name()), methods);
+        }
+    }
+
+    private static void addUnannotatedEndpoints(CombinedIndexBuildItem index, ClassInfo classInfo, List<MethodInfo> methods) {
+        if (!hasSecurityAnnotation(classInfo)) {
+            for (MethodInfo methodInfo : classInfo.methods()) {
+                if (isRestEndpointMethod(index, methodInfo) && !hasSecurityAnnotation(methodInfo)) {
+                    methods.add(methodInfo);
+                }
+            }
+        }
+    }
+
     /**
      * Install the JAX-RS security provider.
      */
     @BuildStep
     void setUpSecurity(BuildProducer<ResteasyJaxrsProviderBuildItem> providers,
-            BuildProducer<AdditionalBeanBuildItem> additionalBeanBuildItem, Capabilities capabilities,
-            Optional<EagerSecurityInterceptorBuildItem> eagerSecurityInterceptors) {
+            BuildProducer<AdditionalBeanBuildItem> additionalBeanBuildItem, Capabilities capabilities) {
         providers.produce(new ResteasyJaxrsProviderBuildItem(UnauthorizedExceptionMapper.class.getName()));
         providers.produce(new ResteasyJaxrsProviderBuildItem(ForbiddenExceptionMapper.class.getName()));
         providers.produce(new ResteasyJaxrsProviderBuildItem(AuthenticationFailedExceptionMapper.class.getName()));
@@ -102,10 +158,16 @@ void setUpSecurity(BuildProducer<ResteasyJaxrsProviderBuildItem> providers,
         if (capabilities.isPresent(Capability.SECURITY)) {
             providers.produce(new ResteasyJaxrsProviderBuildItem(SecurityContextFilter.class.getName()));
             additionalBeanBuildItem.produce(AdditionalBeanBuildItem.unremovableOf(SecurityContextFilter.class));
-            if (eagerSecurityInterceptors.isPresent()) {
-                providers.produce(new ResteasyJaxrsProviderBuildItem(EagerSecurityFilter.class.getName()));
-                additionalBeanBuildItem.produce(AdditionalBeanBuildItem.unremovableOf(EagerSecurityFilter.class));
-            }
+            providers.produce(new ResteasyJaxrsProviderBuildItem(EagerSecurityFilter.class.getName()));
+            additionalBeanBuildItem.produce(AdditionalBeanBuildItem.unremovableOf(EagerSecurityFilter.class));
+            additionalBeanBuildItem.produce(
+                    AdditionalBeanBuildItem.unremovableOf(StandardSecurityCheckInterceptor.RolesAllowedInterceptor.class));
+            additionalBeanBuildItem.produce(AdditionalBeanBuildItem
+                    .unremovableOf(StandardSecurityCheckInterceptor.PermissionsAllowedInterceptor.class));
+            additionalBeanBuildItem.produce(
+                    AdditionalBeanBuildItem.unremovableOf(StandardSecurityCheckInterceptor.PermitAllInterceptor.class));
+            additionalBeanBuildItem.produce(
+                    AdditionalBeanBuildItem.unremovableOf(StandardSecurityCheckInterceptor.AuthenticatedInterceptor.class));
         }
     }
 

extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/AbstractSecurityEventTest.java

@@ -37,7 +37,8 @@ public abstract class AbstractSecurityEventTest {
 
     protected static final Class<?>[] TEST_CLASSES = {
             RolesAllowedResource.class, TestIdentityProvider.class, TestIdentityController.class,
-            UnsecuredResource.class, UnsecuredSubResource.class, EventObserver.class
+            UnsecuredResource.class, UnsecuredSubResource.class, EventObserver.class, UnsecuredResourceInterface.class,
+            UnsecuredParentResource.class
     };
 
     @Inject

extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedJaxRsTest.java

@@ -22,8 +22,8 @@ public class DefaultRolesAllowedJaxRsTest {
     static QuarkusUnitTest runner = new QuarkusUnitTest()
             .withApplicationRoot((jar) -> jar
                     .addClasses(PermitAllResource.class, UnsecuredResource.class,
-                            TestIdentityProvider.class,
-                            TestIdentityController.class,
+                            TestIdentityProvider.class, UnsecuredResourceInterface.class,
+                            TestIdentityController.class, UnsecuredParentResource.class,
                             UnsecuredSubResource.class, HelloResource.class)
                     .addAsResource(new StringAsset("quarkus.security.jaxrs.default-roles-allowed = admin\n"),
                             "application.properties"));
@@ -41,6 +41,18 @@ public void shouldDenyUnannotated() {
         assertStatus(path, 200, 403, 401);
     }
 
+    @Test
+    public void shouldDenyUnannotatedOnParentClass() {
+        String path = "/unsecured/defaultSecurityParent";
+        assertStatus(path, 200, 403, 401);
+    }
+
+    @Test
+    public void shouldDenyUnannotatedOnInterface() {
+        String path = "/unsecured/defaultSecurityInterface";
+        assertStatus(path, 200, 403, 401);
+    }
+
     @Test
     public void shouldDenyDenyAllMethod() {
         String path = "/unsecured/denyAll";

extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DefaultRolesAllowedStarJaxRsTest.java

@@ -17,8 +17,8 @@ public class DefaultRolesAllowedStarJaxRsTest {
     static QuarkusUnitTest runner = new QuarkusUnitTest()
             .withApplicationRoot((jar) -> jar
                     .addClasses(PermitAllResource.class, UnsecuredResource.class,
-                            TestIdentityProvider.class,
-                            TestIdentityController.class,
+                            TestIdentityProvider.class, UnsecuredParentResource.class,
+                            TestIdentityController.class, UnsecuredResourceInterface.class,
                             UnsecuredSubResource.class)
                     .addAsResource(new StringAsset("quarkus.security.jaxrs.default-roles-allowed = **\n"),
                             "application.properties"));

extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DenyAllJaxRsTest.java

@@ -26,8 +26,8 @@ public class DenyAllJaxRsTest {
     static QuarkusUnitTest runner = new QuarkusUnitTest()
             .withApplicationRoot((jar) -> jar
                     .addClasses(PermitAllResource.class, UnsecuredResource.class,
-                            TestIdentityProvider.class,
-                            TestIdentityController.class,
+                            TestIdentityProvider.class, UnsecuredParentResource.class,
+                            TestIdentityController.class, UnsecuredResourceInterface.class,
                             UnsecuredSubResource.class, HelloResource.class)
                     .addAsResource(new StringAsset("quarkus.security.jaxrs.deny-unannotated-endpoints = true\n"),
                             "application.properties"));
@@ -58,6 +58,18 @@ public void shouldDenyUnannotated() {
         assertStatus(path, 403, 401);
     }
 
+    @Test
+    public void shouldDenyUnannotatedOnParentClass() {
+        String path = "/unsecured/defaultSecurityParent";
+        assertStatus(path, 403, 401);
+    }
+
+    @Test
+    public void shouldDenyUnannotatedOnInterface() {
+        String path = "/unsecured/defaultSecurityInterface";
+        assertStatus(path, 403, 401);
+    }
+
     @Test
     public void shouldDenyDenyAllMethod() {
         String path = "/unsecured/denyAll";