[KEYCLOAK-12192] - Missing Input Validation in IDP Authorization URLs

keycloak · Mar 5, 2020 · 2f489a4 · 2f489a4
1 parent 0cf0955
commit 2f489a4
Show file tree

Hide file tree

Showing 41 changed files with 401 additions and 34 deletions.
diff --git a/common/src/main/java/org/keycloak/common/util/UriUtils.java b/common/src/main/java/org/keycloak/common/util/UriUtils.java
@@ -18,10 +18,14 @@
 package org.keycloak.common.util;
 
 import java.io.UnsupportedEncodingException;
+import java.net.MalformedURLException;
 import java.net.URI;
+import java.net.URL;
 import java.net.URLDecoder;
 import java.util.regex.Pattern;
 
+import org.keycloak.common.enums.SslRequired;
+
 /**
  * @author <a href="mailto:sthorger@redhat.com">Stian Thorgersen</a>
  */
@@ -84,4 +88,28 @@ public static MultivaluedHashMap<String, String> decodeQueryString(String queryS
     public static String stripQueryParam(String url, String name){
         return url.replaceFirst("[\\?&]"+name+"=[^&]*$|"+name+"=[^&]*&", "");
     }
+
+    public static void checkUrl(SslRequired sslRequired, String url, String name) throws IllegalArgumentException{
+        if (url == null) {
+            return;
+        }
+
+        URL parsed;
+
+        try {
+            parsed = new URL(url);
+        } catch (MalformedURLException e) {
+            throw new IllegalArgumentException("The url [" + name + "] is malformed", e);
+        }
+
+        String protocol = parsed.getProtocol().toLowerCase();
+
+        if (!("http".equalsIgnoreCase(protocol) || "https".equalsIgnoreCase(protocol))) {
+            throw new IllegalArgumentException("Invalid protocol/scheme for url [" + name + "]");
+        }
+
+        if (!"https".equals(protocol) && sslRequired.isRequired(url)) {
+            throw new IllegalArgumentException("The url [" + name + "] requires secure connections");
+        }
+    }
 }
diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/IdentityProviderFactory.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/IdentityProviderFactory.java
@@ -54,4 +54,14 @@ public interface IdentityProviderFactory<T extends IdentityProvider> extends Pro
      * @return
      */
     Map<String, String> parseConfig(KeycloakSession session, InputStream inputStream);
+
+    /**
+     * <p>Creates a provider specific {@link IdentityProviderModel} instance.
+     * 
+     * <p>Providers may want to implement their own {@link IdentityProviderModel} type so that validations
+     * can be performed when managing the provider configuration
+     * 
+     * @return the provider specific instance
+     */
+    <C extends IdentityProviderModel> C createConfig();
 }
diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java b/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java
@@ -49,6 +49,9 @@
 import org.keycloak.authorization.store.ResourceStore;
 import org.keycloak.authorization.store.ScopeStore;
 import org.keycloak.authorization.store.StoreFactory;
+import org.keycloak.broker.provider.IdentityProvider;
+import org.keycloak.broker.provider.IdentityProviderFactory;
+import org.keycloak.broker.social.SocialIdentityProvider;
 import org.keycloak.common.enums.SslRequired;
 import org.keycloak.common.util.MultivaluedHashMap;
 import org.keycloak.common.util.UriUtils;
@@ -285,7 +288,7 @@ public static void importRealm(KeycloakSession session, RealmRepresentation rep,
             DefaultRequiredActions.addActions(newRealm);
         }
 
-        importIdentityProviders(rep, newRealm);
+        importIdentityProviders(rep, newRealm, session);
         importIdentityProviderMappers(rep, newRealm);
 
         Map<String, ClientScopeModel> clientScopes = new HashMap<>();
@@ -1856,10 +1859,10 @@ public static void createClientRoleMappings(ClientModel clientModel, UserModel u
         }
     }
 
-    private static void importIdentityProviders(RealmRepresentation rep, RealmModel newRealm) {
+    private static void importIdentityProviders(RealmRepresentation rep, RealmModel newRealm, KeycloakSession session) {
         if (rep.getIdentityProviders() != null) {
             for (IdentityProviderRepresentation representation : rep.getIdentityProviders()) {
-                newRealm.addIdentityProvider(toModel(newRealm, representation));
+                newRealm.addIdentityProvider(toModel(newRealm, representation, session));
             }
         }
     }
@@ -1872,8 +1875,20 @@ private static void importIdentityProviderMappers(RealmRepresentation rep, Realm
         }
     }
 
-    public static IdentityProviderModel toModel(RealmModel realm, IdentityProviderRepresentation representation) {
-        IdentityProviderModel identityProviderModel = new IdentityProviderModel();
+    public static IdentityProviderModel toModel(RealmModel realm, IdentityProviderRepresentation representation, KeycloakSession session) {
+        IdentityProviderFactory providerFactory = (IdentityProviderFactory) session.getKeycloakSessionFactory().getProviderFactory(
+                IdentityProvider.class, representation.getProviderId());
+
+        if (providerFactory == null) {
+            providerFactory = (IdentityProviderFactory) session.getKeycloakSessionFactory().getProviderFactory(
+                    SocialIdentityProvider.class, representation.getProviderId());
+        }
+
+        if (providerFactory == null) {
+            throw new IllegalArgumentException("Invalid identity provider id [" + representation.getProviderId() + "]");
+        }
+
+        IdentityProviderModel identityProviderModel = providerFactory.createConfig();
 
         identityProviderModel.setInternalId(representation.getInternalId());
         identityProviderModel.setAlias(representation.getAlias());
@@ -1908,6 +1923,8 @@ public static IdentityProviderModel toModel(RealmModel realm, IdentityProviderRe
             }
             identityProviderModel.setPostBrokerLoginFlowId(flowModel.getId());
         }
+
+        identityProviderModel.validate(realm);
 
         return identityProviderModel;
     }

diff --git a/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java b/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java
@@ -196,5 +196,15 @@ public String getDisplayName() {
     public void setDisplayName(String displayName) {
         this.displayName = displayName;
     }
-
+
+    /**
+     * <p>Validates this configuration.
+     * 
+     * <p>Sub-classes can override this method in order to enforce provider specific validations.
+     * 
+     * @param realm the realm
+     */
+    public void validate(RealmModel realm) {
+
+    }
 }
diff --git a/services/src/main/java/org/keycloak/broker/oidc/KeycloakOIDCIdentityProviderFactory.java b/services/src/main/java/org/keycloak/broker/oidc/KeycloakOIDCIdentityProviderFactory.java
@@ -48,7 +48,10 @@ public String getId() {
     @Override
     public Map<String, String> parseConfig(KeycloakSession session, InputStream inputStream) {
         return OIDCIdentityProviderFactory.parseOIDCConfig(session, inputStream);
-
     }
 
- }
+    @Override
+    public OIDCIdentityProviderConfig createConfig() {
+        return new OIDCIdentityProviderConfig();
+    }
+}
diff --git a/services/src/main/java/org/keycloak/broker/oidc/OAuth2IdentityProviderConfig.java b/services/src/main/java/org/keycloak/broker/oidc/OAuth2IdentityProviderConfig.java
@@ -16,7 +16,12 @@
  */
 package org.keycloak.broker.oidc;
 
+import static org.keycloak.common.util.UriUtils.checkUrl;
+
+import org.keycloak.common.enums.SslRequired;
 import org.keycloak.models.IdentityProviderModel;
+import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.RealmModel;
 import org.keycloak.protocol.oidc.OIDCLoginProtocol;
 
 /**
@@ -28,6 +33,10 @@ public OAuth2IdentityProviderConfig(IdentityProviderModel model) {
         super(model);
     }
 
+    public OAuth2IdentityProviderConfig() {
+        super();
+    }
+
     public String getAuthorizationUrl() {
         return getConfig().get("authorizationUrl");
     }
@@ -123,4 +132,13 @@ public String getForwardParameters() {
     public void setForwardParameters(String forwardParameters) {
        getConfig().put("forwardParameters", forwardParameters);
     }
+
+    @Override
+    public void validate(RealmModel realm) {
+        SslRequired sslRequired = realm.getSslRequired();
+
+        checkUrl(sslRequired, getAuthorizationUrl(), "authorization_url");
+        checkUrl(sslRequired, getTokenUrl(), "token_url");
+        checkUrl(sslRequired, getUserInfoUrl(), "userinfo_url");
+    }
 }
diff --git a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderConfig.java b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderConfig.java
@@ -16,7 +16,12 @@
  */
 package org.keycloak.broker.oidc;
 
+import static org.keycloak.common.util.UriUtils.checkUrl;
+
+import org.keycloak.common.enums.SslRequired;
 import org.keycloak.models.IdentityProviderModel;
+import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.RealmModel;
 
 /**
  * @author Pedro Igor
@@ -33,6 +38,10 @@ public OIDCIdentityProviderConfig(IdentityProviderModel identityProviderModel) {
         super(identityProviderModel);
     }
 
+    public OIDCIdentityProviderConfig() {
+        super();
+    }
+
     public String getPrompt() {
         return getConfig().get("prompt");
     }
@@ -122,4 +131,12 @@ public int getAllowedClockSkew() {
             return 0;
         }
     }
+
+    @Override 
+    public void validate(RealmModel realm) {
+        super.validate(realm);
+        SslRequired sslRequired = realm.getSslRequired();
+        checkUrl(sslRequired, getJwksUrl(), "jwks_url");
+        checkUrl(sslRequired, getLogoutUrl(), "logout_url");
+    }
 }
diff --git a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderFactory.java b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderFactory.java
@@ -43,6 +43,11 @@ public OIDCIdentityProvider create(KeycloakSession session, IdentityProviderMode
         return new OIDCIdentityProvider(session, new OIDCIdentityProviderConfig(model));
     }
 
+    @Override
+    public OIDCIdentityProviderConfig createConfig() {
+        return new OIDCIdentityProviderConfig();
+    }
+
     @Override
     public String getId() {
         return PROVIDER_ID;
@@ -60,7 +65,7 @@ protected static Map<String, String> parseOIDCConfig(KeycloakSession session, In
         } catch (IOException e) {
             throw new RuntimeException("failed to load openid connect metadata", e);
         }
-        OIDCIdentityProviderConfig config = new OIDCIdentityProviderConfig(new IdentityProviderModel());
+        OIDCIdentityProviderConfig config = new OIDCIdentityProviderConfig();
         config.setIssuer(rep.getIssuer());
         config.setLogoutUrl(rep.getLogoutEndpoint());
         config.setAuthorizationUrl(rep.getAuthorizationEndpoint());

diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java
@@ -16,8 +16,13 @@
  */
 package org.keycloak.broker.saml;
 
+import static org.keycloak.common.util.UriUtils.checkUrl;
+
+import org.keycloak.common.enums.SslRequired;
 import org.keycloak.models.IdentityProviderModel;
 
+import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.RealmModel;
 import org.keycloak.protocol.saml.SamlPrincipalType;
 import org.keycloak.saml.common.util.XmlKeyInfoKeyNameTransformer;
 
@@ -276,4 +281,11 @@ public void setPrincipalAttribute(String principalAttribute) {
         getConfig().put(PRINCIPAL_ATTRIBUTE, principalAttribute);
     }
 
+    @Override
+    public void validate(RealmModel realm) {
+        SslRequired sslRequired = realm.getSslRequired();
+
+        checkUrl(sslRequired, getSingleLogoutServiceUrl(), SINGLE_LOGOUT_SERVICE_URL);
+        checkUrl(sslRequired, getSingleSignOnServiceUrl(), SINGLE_SIGN_ON_SERVICE_URL);
+    }
 }
diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java
@@ -58,6 +58,11 @@ public SAMLIdentityProvider create(KeycloakSession session, IdentityProviderMode
         return new SAMLIdentityProvider(session, new SAMLIdentityProviderConfig(model), destinationValidator);
     }
 
+    @Override
+    public SAMLIdentityProviderConfig createConfig() {
+        return new SAMLIdentityProviderConfig();
+    }
+
     @Override
     public Map<String, String> parseConfig(KeycloakSession session, InputStream inputStream) {
         try {

diff --git a/services/src/main/java/org/keycloak/partialimport/IdentityProvidersPartialImport.java b/services/src/main/java/org/keycloak/partialimport/IdentityProvidersPartialImport.java
@@ -72,7 +72,7 @@ public void remove(RealmModel realm, KeycloakSession session, IdentityProviderRe
     @Override
     public void create(RealmModel realm, KeycloakSession session, IdentityProviderRepresentation idpRep) {
         idpRep.setInternalId(KeycloakModelUtils.generateId());
-        IdentityProviderModel identityProvider = RepresentationToModel.toModel(realm, idpRep);
+        IdentityProviderModel identityProvider = RepresentationToModel.toModel(realm, idpRep, session);
         realm.addIdentityProvider(identityProvider);
     }
 

diff --git a/services/src/main/java/org/keycloak/services/resources/admin/IdentityProviderResource.java b/services/src/main/java/org/keycloak/services/resources/admin/IdentityProviderResource.java
@@ -16,6 +16,8 @@
  */
 package org.keycloak.services.resources.admin;
 
+import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
+
 import org.jboss.logging.Logger;
 import org.jboss.resteasy.annotations.cache.NoCache;
 import javax.ws.rs.NotFoundException;
@@ -162,6 +164,8 @@ public Response update(IdentityProviderRepresentation providerRep) {
             adminEvent.operation(OperationType.UPDATE).resourcePath(session.getContext().getUri()).representation(providerRep).success();
 
             return Response.noContent().build();
+        } catch (IllegalArgumentException e) {
+            return ErrorResponse.error("Invalid request", BAD_REQUEST);
         } catch (ModelDuplicateException e) {
             return ErrorResponse.exists("Identity Provider " + providerRep.getAlias() + " already exists");
         }
@@ -176,7 +180,7 @@ private void updateIdpFromRep(IdentityProviderRepresentation providerRep, RealmM
             lookUpProviderIdByAlias(realm, providerRep);
         }
 
-        IdentityProviderModel updated = RepresentationToModel.toModel(realm, providerRep);
+        IdentityProviderModel updated = RepresentationToModel.toModel(realm, providerRep, session);
 
         if (updated.getConfig() != null && ComponentRepresentation.SECRET_VALUE.equals(updated.getConfig().get("clientSecret"))) {
             updated.getConfig().put("clientSecret", identityProviderModel.getConfig() != null ? identityProviderModel.getConfig().get("clientSecret") : null);

diff --git a/services/src/main/java/org/keycloak/services/resources/admin/IdentityProvidersResource.java b/services/src/main/java/org/keycloak/services/resources/admin/IdentityProvidersResource.java
@@ -183,14 +183,16 @@ public Response create(IdentityProviderRepresentation representation) {
         this.auth.realm().requireManageIdentityProviders();
 
         try {
-            IdentityProviderModel identityProvider = RepresentationToModel.toModel(realm, representation);
+            IdentityProviderModel identityProvider = RepresentationToModel.toModel(realm, representation, session);
             this.realm.addIdentityProvider(identityProvider);
 
             representation.setInternalId(identityProvider.getInternalId());
             adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri(), identityProvider.getAlias())
                     .representation(StripSecretsUtils.strip(representation)).success();
 
             return Response.created(session.getContext().getUri().getAbsolutePathBuilder().path(representation.getAlias()).build()).build();
+        } catch (IllegalArgumentException e) {
+            return ErrorResponse.error("Invalid request", BAD_REQUEST);
         } catch (ModelDuplicateException e) {
             return ErrorResponse.exists("Identity Provider " + representation.getAlias() + " already exists");
         }

diff --git a/services/src/main/java/org/keycloak/social/bitbucket/BitbucketIdentityProviderFactory.java b/services/src/main/java/org/keycloak/social/bitbucket/BitbucketIdentityProviderFactory.java
@@ -39,6 +39,11 @@ public BitbucketIdentityProvider create(KeycloakSession session, IdentityProvide
         return new BitbucketIdentityProvider(session, new OAuth2IdentityProviderConfig(model));
     }
 
+    @Override
+    public OAuth2IdentityProviderConfig createConfig() {
+        return new OAuth2IdentityProviderConfig();
+    }
+
     @Override
     public String getId() {
         return PROVIDER_ID;

diff --git a/services/src/main/java/org/keycloak/social/facebook/FacebookIdentityProviderFactory.java b/services/src/main/java/org/keycloak/social/facebook/FacebookIdentityProviderFactory.java
@@ -39,6 +39,11 @@ public FacebookIdentityProvider create(KeycloakSession session, IdentityProvider
         return new FacebookIdentityProvider(session, new OAuth2IdentityProviderConfig(model));
     }
 
+    @Override
+    public OAuth2IdentityProviderConfig createConfig() {
+        return new OAuth2IdentityProviderConfig();
+    }
+
     @Override
     public String getId() {
         return PROVIDER_ID;

diff --git a/services/src/main/java/org/keycloak/social/github/GitHubIdentityProviderFactory.java b/services/src/main/java/org/keycloak/social/github/GitHubIdentityProviderFactory.java
@@ -39,6 +39,11 @@ public GitHubIdentityProvider create(KeycloakSession session, IdentityProviderMo
         return new GitHubIdentityProvider(session, new OAuth2IdentityProviderConfig(model));
     }
 
+    @Override
+    public OAuth2IdentityProviderConfig createConfig() {
+        return new OAuth2IdentityProviderConfig();
+    }
+
     @Override
     public String getId() {
         return PROVIDER_ID;

diff --git a/services/src/main/java/org/keycloak/social/gitlab/GitLabIdentityProviderFactory.java b/services/src/main/java/org/keycloak/social/gitlab/GitLabIdentityProviderFactory.java
@@ -16,7 +16,6 @@
  */
 package org.keycloak.social.gitlab;
 
-import org.keycloak.broker.oidc.OAuth2IdentityProviderConfig;
 import org.keycloak.broker.oidc.OIDCIdentityProviderConfig;
 import org.keycloak.broker.provider.AbstractIdentityProviderFactory;
 import org.keycloak.broker.social.SocialIdentityProviderFactory;
@@ -40,6 +39,11 @@ public GitLabIdentityProvider create(KeycloakSession session, IdentityProviderMo
         return new GitLabIdentityProvider(session, new OIDCIdentityProviderConfig(model));
     }
 
+    @Override
+    public OIDCIdentityProviderConfig createConfig() {
+        return new OIDCIdentityProviderConfig();
+    }
+
     @Override
     public String getId() {
         return PROVIDER_ID;