From a4b3473f4b0dc9b9ffbd4e879995802ce8d12d41 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 14 Sep 2026 13:15:34 +0300 Subject: [PATCH 1/5] Validate ID-FF forward targets, FilesRepo identity names and SAML1 POST target - FSSingleLogoutServlet (/liberty-logout, unauthenticated) and FSReturnLogoutServlet appended the metaAlias request parameter to the "/ProcessLogout/metaAlias" dispatcher path unchecked, and FSUtils.forwardRequest forwarded to any same-host path taken from goto/LRURL. A RequestDispatcher can reach /WEB-INF and skips the web.xml filters. Reject aliases with path traversal (400) and redirect instead of forwarding when the target is not a plain in-app path (ForwardPathValidator). - FilesRepo built new File(typeDir, name) from the identity name; with a Files data store an identity could be created, read or deleted outside the repository. Names with path separators, NUL, "." or ".." now raise IdRepoException. The search filter quoted nothing but "*", so regex metacharacters were interpreted; literal parts are now Pattern.quote()d. - SAMLUtils.postToTarget wrote the target URL into FORM ACTION unescaped; postYN() checks host, port and path but not the query string. - The OAuth module's CSRF state token id (also the NONCE_TOKEN_ID cookie) came from RandomStringUtils.randomAlphanumeric (java.util.Random); use the module's SecureRandom. Closes CodeQL alerts #120, #164, #173, #174, #196, #197, #198, #208, #209 --- .../authentication/modules/oauth2/OAuth.java | 13 ++- .../modules/oauth2/OAuthTest.java | 25 ++++++ .../identity/idm/plugins/files/FilesRepo.java | 33 +++++-- .../idm/plugins/files/FilesRepoTest.java | Bin 0 -> 3250 bytes .../identity/federation/common/FSUtils.java | 10 +++ .../common/ForwardPathValidator.java | 84 ++++++++++++++++++ .../logout/FSReturnLogoutServlet.java | 10 +++ .../logout/FSSingleLogoutServlet.java | 12 ++- .../sun/identity/saml/common/SAMLUtils.java | 5 +- .../common/ForwardPathValidatorTest.java | Bin 0 -> 3383 bytes .../identity/saml/common/SAMLUtilsTest.java | 44 +++++++++ 11 files changed, 227 insertions(+), 9 deletions(-) create mode 100644 openam-core/src/test/java/com/sun/identity/idm/plugins/files/FilesRepoTest.java create mode 100644 openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java create mode 100644 openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java create mode 100644 openam-federation/openam-federation-library/src/test/java/com/sun/identity/saml/common/SAMLUtilsTest.java diff --git a/openam-authentication/openam-auth-oauth2/src/main/java/org/forgerock/openam/authentication/modules/oauth2/OAuth.java b/openam-authentication/openam-auth-oauth2/src/main/java/org/forgerock/openam/authentication/modules/oauth2/OAuth.java index a2952659f0..b785a39bd3 100644 --- a/openam-authentication/openam-auth-oauth2/src/main/java/org/forgerock/openam/authentication/modules/oauth2/OAuth.java +++ b/openam-authentication/openam-auth-oauth2/src/main/java/org/forgerock/openam/authentication/modules/oauth2/OAuth.java @@ -23,7 +23,7 @@  * "Portions Copyrighted [year] [name of copyright owner]" * * Portions Copyrighted 2015 Nomura Research Institute, Ltd. - * Portions Copyrighted 2018-2025 3A Systems, LLC. + * Portions Copyrighted 2018-2026 3A Systems, LLC. */ package org.forgerock.openam.authentication.modules.oauth2; @@ -224,7 +224,7 @@ public int process2(Callback[] callbacks, int state) throws LoginException { String ProviderLogoutURL = config.getLogoutServiceUrl(); - String csrfStateTokenId = RandomStringUtils.randomAlphanumeric(32); + String csrfStateTokenId = newCsrfStateTokenId(); String csrfState = createAuthorizationState(); Token csrfStateToken = new Token(csrfStateTokenId, TokenType.GENERIC); csrfStateToken.setAttribute(CoreTokenField.STRING_ONE, csrfState); @@ -521,6 +521,15 @@ public int process2(Callback[] callbacks, int state) throws LoginException { throw new AuthLoginException(BUNDLE_NAME, "unknownState", null); } + /** + * Id of the CTS record holding the CSRF state; it also travels in the + * NONCE_TOKEN_ID cookie, so it is drawn from a cryptographic generator rather + * than the java.util.Random behind RandomStringUtils.randomAlphanumeric(). + */ + static String newCsrfStateTokenId() { + return RandomStringUtils.random(32, 0, 0, true, true, null, random); + } + private String createAuthorizationState() { return UUID.randomUUID().toString(); //new BigInteger(160, new SecureRandom()).toString(Character.MAX_RADIX); } diff --git a/openam-authentication/openam-auth-oauth2/src/test/java/org/forgerock/openam/authentication/modules/oauth2/OAuthTest.java b/openam-authentication/openam-auth-oauth2/src/test/java/org/forgerock/openam/authentication/modules/oauth2/OAuthTest.java index 6bc55aacab..c8f6b78232 100644 --- a/openam-authentication/openam-auth-oauth2/src/test/java/org/forgerock/openam/authentication/modules/oauth2/OAuthTest.java +++ b/openam-authentication/openam-auth-oauth2/src/test/java/org/forgerock/openam/authentication/modules/oauth2/OAuthTest.java @@ -1,7 +1,24 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ package org.forgerock.openam.authentication.modules.oauth2; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotEquals; +import static org.testng.Assert.assertTrue; import java.util.Collections; import java.util.HashMap; @@ -29,5 +46,13 @@ public void shouldRemoveCredentialAndAccountStatusAttributesFromUpdates() { assertFalse(attributes.containsKey("userPassword")); assertFalse(attributes.containsKey("inetuserstatus")); } + + @Test + public void csrfStateTokenIdIsThirtyTwoAlphanumericCharacters() { + String id = OAuth.newCsrfStateTokenId(); + + assertTrue(id.matches("[A-Za-z0-9]{32}"), id); + assertNotEquals(id, OAuth.newCsrfStateTokenId()); + } } diff --git a/openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java b/openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java index f48ed80b2f..e37ef28050 100755 --- a/openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java +++ b/openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java @@ -25,6 +25,7 @@ * $Id: FilesRepo.java,v 1.22 2008/07/02 17:21:21 kenwho Exp $ * * Portions Copyrighted 2011-2016 ForgeRock AS. + * Portions Copyrighted 2026 3A Systems LLC. */ package com.sun.identity.idm.plugins.files; @@ -1510,7 +1511,18 @@ void initDir(String rootDir) throws IdRepoException { } - File constructFile(String rootDir, IdType type, String name) { + File constructFile(String rootDir, IdType type, String name) + throws IdRepoException { + // The identity name becomes the file name: it must stay a single path + // component, or an identity could be created, read or deleted anywhere + // the server can write. + if (name == null || name.isEmpty() || name.equals(".") || name.equals("..") + || name.indexOf('/') != -1 || name.indexOf('\\') != -1 + || name.indexOf('\0') != -1) { + debug.error("FilesRepo.constructFile: invalid identity name: " + name); + throw new IdRepoException(IdRepoBundle.getString(IdRepoErrorCode.ILLEGAL_ARGUMENTS), + IdRepoErrorCode.ILLEGAL_ARGUMENTS); + } // Construct file name File root = new File(rootDir); File subDir = new File(root, type.getName()); @@ -1709,13 +1721,24 @@ class FileRepoFileFilter implements FilenameFilter { // Default constructor FileRepoFileFilter(String p) { if (p != null && p.length() != 0 && !p.equals("*")) { - // Replace "*" with ".*" + // "*" is the only wildcard; everything between wildcards is a + // literal, so quote it rather than letting the search pattern + // be interpreted as a regular expression. + StringBuilder regex = new StringBuilder(); + int from = 0; int idx = p.indexOf('*'); while (idx != -1) { - p = p.substring(0, idx) + ".*" + p.substring(idx + 1); - idx = p.indexOf('*', idx + 2); + if (idx > from) { + regex.append(Pattern.quote(p.substring(from, idx))); + } + regex.append(".*"); + from = idx + 1; + idx = p.indexOf('*', from); + } + if (from < p.length()) { + regex.append(Pattern.quote(p.substring(from))); } - pattern = Pattern.compile(p.toLowerCase()); + pattern = Pattern.compile(regex.toString(), Pattern.CASE_INSENSITIVE); } } diff --git a/openam-core/src/test/java/com/sun/identity/idm/plugins/files/FilesRepoTest.java b/openam-core/src/test/java/com/sun/identity/idm/plugins/files/FilesRepoTest.java new file mode 100644 index 0000000000000000000000000000000000000000..99ca62478d14fd2e438b1d8a6c96451b39cc3ae8 GIT binary patch literal 3250 zcmb_e+in{-5bd+RVz5suL!pus=tGSZO(h2vs>Fe%ASfI^P`e|EDVN-mTuF<-|K6Fs zXk|%p0ylzf$Ry`>=A0pi7d^Ou`5GayCZGu(>=MFSc~~lqARU4)7N0Q*5G-vV1aw(* z3Snfk%o-Tu2DQyIasbk#>~pMqaB5KmMf+2gpz#PXJ81st3;5d>kjWB^4N%Z_7Z-O4 z<^hzUys}&?X%d933TsY*-8W7IedmAKQz8x6MUZ49$t*AHQuYBs2GF>YdN>-7r<)hz zxd^);V1^vIJGQj0jpB1m)KVp&rCAkng|M<4bVivKL90v!$<=x&Op29{ z<1|06(LlOKhP;f}AEq#~%djQ12UK&Si?jiQGlf4mL_1`N=Wa~h0h|f4u#B>cwH@U!<)VNT87IfTh7;xiu$AM zf`679DU=m&D5q}qZ2FV*26KB=sfIGzXYrN8^D-xoO^sY!Kpza%Xh@1RiN#n3`QF(L z!Q{Smc24G$VO}hVQ;=xF3OH-eS@9-(>GgmR34%VAhp!pEyl0d)Pxo znQ;vMu%lX5ud0No)dwNsXtaNj_6*tJ8*uB#b-dq>J-G5IT> z5Qoi*5aJ$r)xguC;V=}=R4q9$?)EwAmq>_V{evMH=2Yf3jqV3~NZUha)=rmpjjQr> zi|(Lds@&;@5T8Er@9uO9H%k9>x_q)?3at-PzJCZPb1zO0v_LeCz7Ap3KSceMs5i)W zsjn!?beN*t;0aAHJ)~w5pSS-V8a8!ej*~u%l zE0!nOP!D)wh*mof55<*K#A6C1A3`Sl#)!H7Bq~-Ho3J$Ju0WnXEg~K6tlsmsyT^VtWJ6Iovj{Q^z@LM&|}->G-4-)=K<1fLh5+X^5GDoID)M$ zG(E29DNdB9GvG@TTI89jcQ56kF(3QNX|NA9&# TWmM;%A}L96*zQP9_r3oBXe1b< literal 0 HcmV?d00001 diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java index 8502f5d2a7..1dce98bef7 100755 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java @@ -358,6 +358,16 @@ public static void forwardRequest( } else { String resource = newUrl.substring( index + deploymentURI.length()); + // The target comes from the request (goto/RelayState). A forward + // bypasses the web.xml filters and can reach /WEB-INF, so hand + // anything that is not a plain in-app path back to the container + // as a redirect instead. + if (!ForwardPathValidator.isSafeForwardPath(resource)) { + FSUtils.debug.warning("FSUtils.forwardRequest: refusing to " + + "forward to " + resource + "; redirecting instead"); + response.sendRedirect(newUrl); + return; + } if (FSUtils.debug.messageEnabled()) { FSUtils.debug.message( "FSUtils.forwardRequest: Forwarding to :" + resource); diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java new file mode 100644 index 0000000000..d11d65a4bb --- /dev/null +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java @@ -0,0 +1,84 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package com.sun.identity.federation.common; + +import java.util.Locale; + +/** + * Sanity checks for request-derived values before they are handed to a + * {@code RequestDispatcher}. A forward is served from inside the web application: + * it can reach {@code /WEB-INF} and {@code /META-INF}, and it does not run the + * filters declared in {@code web.xml}, so a path an end user controls must never + * be allowed to climb out of the location the code intends to dispatch to. + */ +public final class ForwardPathValidator { + + private ForwardPathValidator() { + } + + /** + * Whether {@code path} may be forwarded to as-is: absolute, without {@code ..} + * segments (path parameters stripped, as the container does), without + * backslashes or control characters, and not under a reserved directory. + * + * @param path a context-relative path, optionally with a query string + * @return {@code true} if the path is safe to pass to a request dispatcher + */ + public static boolean isSafeForwardPath(String path) { + if (path == null || !path.startsWith("/")) { + return false; + } + int query = path.indexOf('?'); + String uri = query == -1 ? path : path.substring(0, query); + if (containsTraversal(uri)) { + return false; + } + String lower = uri.toLowerCase(Locale.ROOT); + return !(lower.startsWith("/web-inf/") || lower.equals("/web-inf") + || lower.startsWith("/meta-inf/") || lower.equals("/meta-inf")); + } + + /** + * Whether {@code metaAlias} can be appended to a fixed handler path without + * changing which resource is dispatched to. + * + * @param metaAlias the provider meta alias taken from the request + * @return {@code true} if the alias contains no traversal + */ + public static boolean isSafeMetaAlias(String metaAlias) { + return metaAlias != null && !metaAlias.isEmpty() && !containsTraversal(metaAlias); + } + + private static boolean containsTraversal(String value) { + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if (c == '\\' || c < ' ' || c == 0x7f) { + return true; + } + } + for (String segment : value.split("/", -1)) { + // The container strips ";param" from each segment before normalising. + int semicolon = segment.indexOf(';'); + if (semicolon != -1) { + segment = segment.substring(0, semicolon); + } + if (segment.equals("..")) { + return true; + } + } + return false; + } +} diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSReturnLogoutServlet.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSReturnLogoutServlet.java index d348854052..83b0130a19 100644 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSReturnLogoutServlet.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSReturnLogoutServlet.java @@ -37,6 +37,7 @@ import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import com.sun.identity.federation.common.ForwardPathValidator; import com.sun.identity.federation.common.FSUtils; import com.sun.identity.federation.common.IFSConstants; import com.sun.identity.federation.common.LogUtil; @@ -144,6 +145,15 @@ private void doGetPost(HttpServletRequest request, FSUtils.bundle.getString("aliasNotFound")); return; } + // The alias is appended to the dispatcher path below; a traversal in it + // would forward to an arbitrary resource of the web application. + if (!ForwardPathValidator.isSafeMetaAlias(providerAlias)) { + FSUtils.debug.error("FSReturnLogoutServlet: rejecting metaAlias " + + "with path traversal"); + response.sendError(response.SC_BAD_REQUEST, + FSUtils.bundle.getString("aliasNotFound")); + return; + } Object ssoToken = null; try { diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSSingleLogoutServlet.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSSingleLogoutServlet.java index b0969a81ab..c339bf222c 100755 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSSingleLogoutServlet.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSSingleLogoutServlet.java @@ -24,7 +24,7 @@ * * $Id: FSSingleLogoutServlet.java,v 1.5 2008/12/19 06:50:47 exu Exp $ * - * Portions Copyrighted 2025 3A Systems LLC. + * Portions Copyrighted 2025-2026 3A Systems LLC. */ @@ -37,6 +37,7 @@ import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import com.sun.identity.federation.common.ForwardPathValidator; import com.sun.identity.federation.common.FSUtils; import com.sun.identity.federation.common.IFSConstants; import com.sun.identity.federation.services.util.FSServiceUtils; @@ -124,6 +125,15 @@ private void doGetPost(HttpServletRequest request, FSUtils.bundle.getString("aliasNotFound")); return; } + // The alias is appended to the dispatcher path below; a traversal in it + // would forward to an arbitrary resource of the web application. + if (!ForwardPathValidator.isSafeMetaAlias(providerAlias)) { + FSUtils.debug.error("FSSingleLogoutServlet: rejecting metaAlias " + + "with path traversal"); + response.sendError(response.SC_BAD_REQUEST, + FSUtils.bundle.getString("aliasNotFound")); + return; + } request.setAttribute("logoutSource", "local"); StringBuffer processLogout = new StringBuffer(); diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java index d4283dd4b7..bdb15b1f3f 100644 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java @@ -742,7 +742,10 @@ public static void postToTarget(HttpServletResponse response, PrintWriter out, } out.println("

\n"); } - out.println("
"); + // postYN() only validates host, port and path of the target: the query + // string is caller-supplied, so it must not be able to close the attribute. + out.println(""); if (assertion != null) { it = assertion.iterator(); while (it.hasNext()) { diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java new file mode 100644 index 0000000000000000000000000000000000000000..4ebed81af7d0224f63a3ac12e3dcdd5b64ceaa0a GIT binary patch literal 3383 zcmcgu+iu%N5Y4l`VxWgY4(w&(0DY)oq^e~j27E&$G$<^iSdk-X-Q_N^yOc}>{`bz1 zG(}5vt0HLmL~>@&+|C>_df$WhFj*qR)&w-cgPlWIDi3p|5u`)#`Rozn0Kt+0A)rfZ zFN8Cjrq;j+SEy~45&%e(Fy=`4;M6P+itJY^M&l6%3{XED2>9LRkjes#4Um(06BnOi z%mOGwd8L_F(!>aB6_%U=!`C2!?)j0CVrjt6f}}%|j%7tvN)r%d0F4W&qqEWIYV}ck z5aB5Z7$ZmCxXrD5iQ+Si)m+7(rCH>1fv~VEbViv4WYh=cJ(ou%gI1{ulB@NQn*>XY z?KHnG(LmC4hEUEJ4-**Md011o52@x@=SkfR&J@0&WV{<2sEyncl<~5j4@IsIa2sBg zQO>WsKoaWfj{x_rkOFm#o~VKW4Zk2>*yl&JQ=S>WsajUPj6B2#Ha`=SEb?ghI@b z+FNEJ$VPlV>Gf2aQ99sxq%l~x5Yl^e;gMV%3YjS}EMM@zYujWg!{#WT3Fw2NqlOq> zaYKw`kay0mD7rg&3G;^UgpLznmd|KZ5Nk>SF03P-BzH0_f0J4zGFZoc@U7Pax_)j4 zDBsk_Z_6#df1ufTImZ@`9~{tWKFpWHkUIkg*U&n$v9r&&ex#CYJifg&Du_p)^`8$L zfZD;!R3ArV+j9M!Vvs{xX@AO=AAELBoCw9`==7-rhIGkWjnPbcjE2|4Bvt0Z5g$5y z*El<3PDL5bxlW6-sjnykYjjc4+M(3&_57Ek%bN?@NJj!^T>+$f4r$ISBoS*}RIrR@ zLc~!*aa)GwmcaW$^oi4+MoS9haZ&0aL9#HpL>WmDn92?G^GSL8y#MHO^Zmm!xz;=L z+HL2~gqvEc8>-l$CR(pH1+j&CRW~-GfrzJ6?_#rsM-`Q%Mjt(i%vqAupwM@?U!PBg zJgVq=cyoDiKAwo{iw?KBwUH2egs&aR7eh>R@qW#NZ3a`~fNMHDH;cAxiG;|{`f>T+7J_NCDq z#-qxb7Jv9mykg-N)%GZ?n%j4kwx?F{*6+3WBx?KmR=e8T*T36s!hP2<0eVf#+)AzO zv(?Z}>htcl8rZ@6t<`Ngzi>T$;Z~bi`z>&THrRlaJG^=P_lw+slGN`?QN714Csvtr ZAIl6UOZ-AV+Zr8<)BjfA)?z>R{sJZJ1#18R literal 0 HcmV?d00001 diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/saml/common/SAMLUtilsTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/saml/common/SAMLUtilsTest.java new file mode 100644 index 0000000000..ce36c8c430 --- /dev/null +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/saml/common/SAMLUtilsTest.java @@ -0,0 +1,44 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package com.sun.identity.saml.common; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.Collections; + +import org.testng.annotations.Test; + +public class SAMLUtilsTest { + + /** + * postYN() only checks host, port and path of the target, so the query string + * is attacker-influenced and must not be able to close the ACTION attribute. + */ + @Test + public void postToTargetEscapesTargetUrlInFormAction() throws Exception { + String target = "https://sp.example.com/acs?next=\">"); + assertThat(out).contains("ACTION=\"https://sp.example.com/acs?next="><script>"); + } +} From 76ef05a89c2e9ea03079f0abbe0be31b24c04b25 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 14 Sep 2026 13:44:40 +0300 Subject: [PATCH 2/5] Address CodeQL follow-ups on the ID-FF forward guard and SAML1 POST target - FSUtils.forwardRequest: answer 400 instead of redirecting when the forward target is rejected; the redirect reused the request-supplied URL. - SAMLUtils.postToTarget: escape the FORM ACTION with StringEscapeUtils.escapeHtml4, which CodeQL recognises as a sanitizer. --- .../com/sun/identity/federation/common/FSUtils.java | 10 +++++----- .../java/com/sun/identity/saml/common/SAMLUtils.java | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java index 1dce98bef7..ce281b783a 100755 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java @@ -359,13 +359,13 @@ public static void forwardRequest( String resource = newUrl.substring( index + deploymentURI.length()); // The target comes from the request (goto/RelayState). A forward - // bypasses the web.xml filters and can reach /WEB-INF, so hand - // anything that is not a plain in-app path back to the container - // as a redirect instead. + // bypasses the web.xml filters and can reach /WEB-INF, so refuse + // anything that is not a plain in-app path. if (!ForwardPathValidator.isSafeForwardPath(resource)) { FSUtils.debug.warning("FSUtils.forwardRequest: refusing to " - + "forward to " + resource + "; redirecting instead"); - response.sendRedirect(newUrl); + + "forward to a path with traversal or a reserved " + + "directory"); + response.sendError(HttpServletResponse.SC_BAD_REQUEST); return; } if (FSUtils.debug.messageEnabled()) { diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java index bdb15b1f3f..95b0fea539 100644 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java @@ -63,6 +63,7 @@ import com.sun.identity.common.TaskRunnable; import com.sun.identity.common.TimerPool; import com.sun.identity.shared.xml.XMLUtils; +import org.apache.commons.lang3.StringEscapeUtils; import com.sun.identity.shared.encode.URLEncDec; import com.sun.identity.shared.encode.Base64; @@ -745,7 +746,7 @@ public static void postToTarget(HttpServletResponse response, PrintWriter out, // postYN() only validates host, port and path of the target: the query // string is caller-supplied, so it must not be able to close the attribute. out.println(""); + + StringEscapeUtils.escapeHtml4(targeturl) + "\">"); if (assertion != null) { it = assertion.iterator(); while (it.hasNext()) { From 5cbbeb72d90cf54827b654c661c64693f0f8a8ce Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 17 Sep 2026 07:06:24 +0300 Subject: [PATCH 3/5] Decode the ID-FF forward target before validating it; FilesRepo review follow-ups Review round 2 on #1128: - ForwardPathValidator checked the raw string only, while the container percent-decodes a dispatcher path once more before normalising it (Tomcat >= 11.0.3 / 10.1.35: decode, then normalize, no rejection of an encoded ".."). metaAlias=/%252e%252e/%252e%252e/WEB-INF/web.xml passed the guard and reached /WEB-INF/web.xml. The validator now decodes once (two ASCII hex digits, "+" untouched), runs every check on both the raw and the decoded form, and refuses a malformed escape or one that survives the single decode. - Entry-point tests: FSSingleLogoutServletTest drives /liberty-logout and asserts 400 with no dispatcher for literal, single- and double-encoded aliases; FSUtilsForwardPathTest does the same for forwardRequest. - FilesRepo: identity names with any control character are refused (not only NUL) and the rejected name is no longer logged. FileRepoFileFilter folds case beyond ASCII again (UNICODE_CASE): accept() lowercases the file name with String.toLowerCase(), which the ASCII-only CASE_INSENSITIVE pattern did not match. - Test sources carry "\0" escapes instead of raw NUL bytes, so git diffs them as text. --- .../identity/idm/plugins/files/FilesRepo.java | 32 ++++-- .../idm/plugins/files/FilesRepoTest.java | Bin 3250 -> 3709 bytes .../common/ForwardPathValidator.java | 90 +++++++++++++++- .../common/FSUtilsForwardPathTest.java | 98 ++++++++++++++++++ .../common/ForwardPathValidatorTest.java | Bin 3383 -> 4897 bytes .../logout/FSSingleLogoutServletTest.java | 90 ++++++++++++++++ 6 files changed, 299 insertions(+), 11 deletions(-) create mode 100644 openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java create mode 100644 openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/services/logout/FSSingleLogoutServletTest.java diff --git a/openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java b/openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java index e37ef28050..3f96a56bd5 100755 --- a/openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java +++ b/openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java @@ -1515,11 +1515,11 @@ File constructFile(String rootDir, IdType type, String name) throws IdRepoException { // The identity name becomes the file name: it must stay a single path // component, or an identity could be created, read or deleted anywhere - // the server can write. - if (name == null || name.isEmpty() || name.equals(".") || name.equals("..") - || name.indexOf('/') != -1 || name.indexOf('\\') != -1 - || name.indexOf('\0') != -1) { - debug.error("FilesRepo.constructFile: invalid identity name: " + name); + // the server can write. Control characters are refused too, so a name + // cannot forge a log line. The name is caller data and is not logged. + if (!isSingleFileName(name)) { + debug.error("FilesRepo.constructFile: invalid identity name of type " + + type.getName() + ", length " + (name == null ? 0 : name.length())); throw new IdRepoException(IdRepoBundle.getString(IdRepoErrorCode.ILLEGAL_ARGUMENTS), IdRepoErrorCode.ILLEGAL_ARGUMENTS); } @@ -1713,6 +1713,25 @@ boolean containsAttrValue(Set attrValues, Set patterns) { return (false); } + /** + * Whether an identity name is usable as one file name under the type + * directory: not empty, not a dot directory, and free of path separators + * (both kinds, so a repository copied between platforms stays valid) and + * of control characters. + */ + static boolean isSingleFileName(String name) { + if (name == null || name.isEmpty() || name.equals(".") || name.equals("..")) { + return false; + } + for (int i = 0; i < name.length(); i++) { + char c = name.charAt(i); + if (c == '/' || c == '\\' || c < ' ' || c == 0x7f) { + return false; + } + } + return true; + } + // File name filter inner class class FileRepoFileFilter implements FilenameFilter { // Pattern to match @@ -1738,7 +1757,8 @@ class FileRepoFileFilter implements FilenameFilter { if (from < p.length()) { regex.append(Pattern.quote(p.substring(from))); } - pattern = Pattern.compile(regex.toString(), Pattern.CASE_INSENSITIVE); + pattern = Pattern.compile(regex.toString(), + Pattern.CASE_INSENSITIVE | Pattern.UNICODE_CASE); } } diff --git a/openam-core/src/test/java/com/sun/identity/idm/plugins/files/FilesRepoTest.java b/openam-core/src/test/java/com/sun/identity/idm/plugins/files/FilesRepoTest.java index 99ca62478d14fd2e438b1d8a6c96451b39cc3ae8..22e70032171516e1f09ba7ded3db49492093febd 100644 GIT binary patch delta 219 zcmdla`B!Gc1Gf4Y1EpFWE(Ih|t(01pnG;hKlb4oXl%ARblt;+w>nk`1>HD}@Ddgm* z>*i$Ur7D0_W#*+TB<7_kV!Z delta 19 bcmew>vq^Hp12#s6%@OPqST@`6Ix+$POpOL$ diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java index d11d65a4bb..6a53630cff 100644 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java @@ -15,6 +15,8 @@ */ package com.sun.identity.federation.common; +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; import java.util.Locale; /** @@ -23,6 +25,12 @@ * it can reach {@code /WEB-INF} and {@code /META-INF}, and it does not run the * filters declared in {@code web.xml}, so a path an end user controls must never * be allowed to climb out of the location the code intends to dispatch to. + *

+ * The container percent-decodes a dispatcher path once more before it normalises + * it (Tomcat: {@code dispatchersUseEncodedPaths}, on by default), so every check + * runs on the decoded form as well as on the raw one, and a value that would + * still carry an escape after that single decode - a second encoding layer - is + * refused outright: no in-app path of the product needs one. */ public final class ForwardPathValidator { @@ -31,8 +39,9 @@ private ForwardPathValidator() { /** * Whether {@code path} may be forwarded to as-is: absolute, without {@code ..} - * segments (path parameters stripped, as the container does), without - * backslashes or control characters, and not under a reserved directory. + * segments in its raw or decoded form (path parameters stripped, as the + * container does), without backslashes, control characters, malformed or + * double escapes, and not under a reserved directory. * * @param path a context-relative path, optionally with a query string * @return {@code true} if the path is safe to pass to a request dispatcher @@ -43,10 +52,11 @@ public static boolean isSafeForwardPath(String path) { } int query = path.indexOf('?'); String uri = query == -1 ? path : path.substring(0, query); - if (containsTraversal(uri)) { + String resolved = resolve(uri); + if (resolved == null) { return false; } - String lower = uri.toLowerCase(Locale.ROOT); + String lower = resolved.toLowerCase(Locale.ROOT); return !(lower.startsWith("/web-inf/") || lower.equals("/web-inf") || lower.startsWith("/meta-inf/") || lower.equals("/meta-inf")); } @@ -59,7 +69,24 @@ public static boolean isSafeForwardPath(String path) { * @return {@code true} if the alias contains no traversal */ public static boolean isSafeMetaAlias(String metaAlias) { - return metaAlias != null && !metaAlias.isEmpty() && !containsTraversal(metaAlias); + return metaAlias != null && !metaAlias.isEmpty() && resolve(metaAlias) != null; + } + + /** + * The path as the container will resolve it, or {@code null} when it must not + * be dispatched to: traversal, a backslash or a control character in either + * the raw or the decoded form, or an escape that is malformed or survives the + * container's single decode. + */ + private static String resolve(String value) { + if (containsTraversal(value)) { + return null; + } + String decoded = decodeOnce(value); + if (decoded == null || decoded.indexOf('%') != -1 || containsTraversal(decoded)) { + return null; + } + return decoded; } private static boolean containsTraversal(String value) { @@ -81,4 +108,57 @@ private static boolean containsTraversal(String value) { } return false; } + + /** + * Percent-decodes {@code value} exactly once, the way the container does for + * a path: {@code %XX} with two ASCII hex digits only, {@code +} left alone. + * + * @return the decoded value, or {@code null} if an escape is malformed + */ + private static String decodeOnce(String value) { + if (value.indexOf('%') == -1) { + return value; + } + StringBuilder decoded = new StringBuilder(value.length()); + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + int i = 0; + while (i < value.length()) { + char c = value.charAt(i); + if (c != '%') { + decoded.append(c); + i++; + continue; + } + // A run of escapes is one byte sequence: a multi-byte character has + // to be decoded as a whole or its bytes turn into replacement characters. + bytes.reset(); + while (i < value.length() && value.charAt(i) == '%') { + if (i + 2 >= value.length()) { + return null; + } + int hi = hexDigit(value.charAt(i + 1)); + int lo = hexDigit(value.charAt(i + 2)); + if (hi < 0 || lo < 0) { + return null; + } + bytes.write((hi << 4) | lo); + i += 3; + } + decoded.append(new String(bytes.toByteArray(), StandardCharsets.UTF_8)); + } + return decoded.toString(); + } + + private static int hexDigit(char c) { + if (c >= '0' && c <= '9') { + return c - '0'; + } + if (c >= 'a' && c <= 'f') { + return c - 'a' + 10; + } + if (c >= 'A' && c <= 'F') { + return c - 'A' + 10; + } + return -1; + } } diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java new file mode 100644 index 0000000000..571b6a86d6 --- /dev/null +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java @@ -0,0 +1,98 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package com.sun.identity.federation.common; + +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import jakarta.servlet.RequestDispatcher; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +/** + * Drives {@link FSUtils#forwardRequest} with a same-host target, the branch + * that ends in a {@code RequestDispatcher} rather than a redirect. + */ +public class FSUtilsForwardPathTest { + + private static final String BASE = "http://sp.example.com:8080/openam"; + + private String savedDeploymentURI; + private HttpServletRequest request; + private HttpServletResponse response; + + @BeforeClass + public void pinDeploymentUri() { + savedDeploymentURI = FSUtils.deploymentURI; + FSUtils.deploymentURI = "/openam"; + } + + @AfterClass(alwaysRun = true) + public void restoreDeploymentUri() { + FSUtils.deploymentURI = savedDeploymentURI; + } + + @BeforeMethod + public void setUp() { + request = mock(HttpServletRequest.class); + when(request.getServerName()).thenReturn("sp.example.com"); + when(request.getServerPort()).thenReturn(8080); + response = mock(HttpServletResponse.class); + } + + @Test + public void forwardsAnInAppTargetOnTheSameHost() throws Exception { + RequestDispatcher dispatcher = mock(RequestDispatcher.class); + when(request.getRequestDispatcher("/UI/Login?goto=%2Fopenam%2Fconsole")).thenReturn(dispatcher); + + FSUtils.forwardRequest(request, response, BASE + "/UI/Login?goto=%2Fopenam%2Fconsole"); + + verify(dispatcher).forward(request, response); + verify(response, never()).sendError(anyInt()); + } + + @DataProvider + public Object[][] traversingTargets() { + return new Object[][] { + {BASE + "/x/../WEB-INF/web.xml"}, + {BASE + "/WEB-INF/web.xml"}, + // The container decodes the dispatcher path before it normalises it. + {BASE + "/x/%2e%2e/WEB-INF/web.xml"}, + {BASE + "/x/%252e%252e/WEB-INF/web.xml"}, + {BASE + "/x/..%2fWEB-INF/web.xml"}, + {BASE + "/%57EB-INF/web.xml"}, + {BASE + "/x/%zz"}, + }; + } + + @Test(dataProvider = "traversingTargets") + public void answers400WithoutDispatchingForATraversingTarget(String url) throws Exception { + FSUtils.forwardRequest(request, response, url); + + verify(response).sendError(HttpServletResponse.SC_BAD_REQUEST); + verify(request, never()).getRequestDispatcher(anyString()); + verify(response, never()).sendRedirect(anyString()); + } +} diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java index 4ebed81af7d0224f63a3ac12e3dcdd5b64ceaa0a..28497885b62cc66404053579076259fe70d2a447 100644 GIT binary patch delta 1122 zcmZ`&%Wl&^6cuftt;@2oD4R44{(g|)2We$!O1+Fm35Vcp%Z8x3q$ zdE820-MBL*2zRMk6GCoayX*=v4?&TDaj@WKmJg)f$&!NQJg%}N$;7RYP{E}8ac(_% z!wSym4%2gc`kcJzi*6W@h7`1(aRW5zJy&_Dnx4^1tv?nu#pkeh7ybrWbw%Gxq!qfz0{_Gj_WLqIp@|7_fI zUHjtG+WAWQXJg}-?wLduMo#gbO>sy}$@)Zv?kh#_`O_0H{6Vy!Om~zv^2)S#{V)xz w-z%qX`pdeV`kUKZ284JlMsa_=n7msBLFZG7+3qUE$zZM8qtV6L=GSZg01YTXQvd(} delta 54 zcmZ3ewq0t&B9_TqtU8;|vp!;CWY}E6Q_VEFgNJ`|Kbz*{X8sq943iHEh)r%5kOGQo KZ0;AF#0UTf+!A2` diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/services/logout/FSSingleLogoutServletTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/services/logout/FSSingleLogoutServletTest.java new file mode 100644 index 0000000000..45996a8ab9 --- /dev/null +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/services/logout/FSSingleLogoutServletTest.java @@ -0,0 +1,90 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package com.sun.identity.federation.services.logout; + +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import jakarta.servlet.RequestDispatcher; +import jakarta.servlet.ServletConfig; +import jakarta.servlet.ServletContext; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +/** + * Drives the unauthenticated {@code /liberty-logout} endpoint end to end: the + * request parameter goes in, the dispatcher path (or the error status) comes out. + */ +public class FSSingleLogoutServletTest { + + private FSSingleLogoutServlet servlet; + private ServletContext servletContext; + private HttpServletRequest request; + private HttpServletResponse response; + + @BeforeMethod + public void setUp() throws Exception { + servletContext = mock(ServletContext.class); + ServletConfig config = mock(ServletConfig.class); + when(config.getServletContext()).thenReturn(servletContext); + servlet = new FSSingleLogoutServlet(); + servlet.init(config); + request = mock(HttpServletRequest.class); + response = mock(HttpServletResponse.class); + } + + @Test + public void forwardsAnOrdinaryAliasToTheProcessLogoutHandler() throws Exception { + when(request.getParameter("metaAlias")).thenReturn("/idp"); + RequestDispatcher dispatcher = mock(RequestDispatcher.class); + when(servletContext.getRequestDispatcher("/ProcessLogout/metaAlias/idp")).thenReturn(dispatcher); + + servlet.doGet(request, response); + + verify(dispatcher).forward(request, response); + verify(response, never()).sendError(anyInt(), anyString()); + } + + @DataProvider + public Object[][] traversingAliases() { + return new Object[][] { + {"/../../WEB-INF/web.xml"}, + // getParameter() has already decoded one layer; the container decodes + // the dispatcher path once more before normalising it. + {"/%2e%2e/%2e%2e/WEB-INF/web.xml"}, + {"/%252e%252e/%252e%252e/WEB-INF/web.xml"}, + {"/idp/..%2f..%2fWEB-INF/web.xml"}, + }; + } + + @Test(dataProvider = "traversingAliases") + public void answers400WithoutDispatchingForATraversingAlias(String metaAlias) throws Exception { + when(request.getParameter("metaAlias")).thenReturn(metaAlias); + + servlet.doGet(request, response); + + verify(response).sendError(eq(HttpServletResponse.SC_BAD_REQUEST), anyString()); + verify(servletContext, never()).getRequestDispatcher(anyString()); + } +} From e415dc54850f486a4abff93b28a8db3693bf5feb Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 17 Sep 2026 10:14:00 +0300 Subject: [PATCH 4/5] Collapse the forward path before the reserved-directory check Review round 3 on #1128: - ForwardPathValidator ran the /WEB-INF check on the decoded but uncollapsed path, while the container collapses "//", "/./" and ";params" before mapping: "//WEB-INF/web.xml", "/./WEB-INF/web.xml", "/WEB-INF;x/web.xml" and "/%2e/WEB-INF/web.xml" passed and were served. resolve() now returns the collapsed path and the check reads that; the fragment-stripped form is checked too, since HttpServletRequest.getRequestDispatcher() drops "#..." before mapping. - An escape that decodes to a delimiter (%25, %3F, %23) or to invalid UTF-8 (overlong %C0%AE) is refused; the container would map the first literally and replace the second, neither of which is a path this code means to reach. - FSReturnLogoutServletTest drives the prefix-mapped servlet through the raw getRequestURI() alias fallback; FSUtilsForwardPathTest pins the collapsed forms and restores FSUtils.deploymentURI per test. --- .../common/ForwardPathValidator.java | 89 ++++++++++++++---- .../common/FSUtilsForwardPathTest.java | 29 +++--- .../common/ForwardPathValidatorTest.java | 25 +++++ .../logout/FSReturnLogoutServletTest.java | 91 +++++++++++++++++++ 4 files changed, 201 insertions(+), 33 deletions(-) create mode 100644 openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/services/logout/FSReturnLogoutServletTest.java diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java index 6a53630cff..8ea68a44fa 100644 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java @@ -16,6 +16,9 @@ package com.sun.identity.federation.common; import java.io.ByteArrayOutputStream; +import java.nio.ByteBuffer; +import java.nio.charset.CharacterCodingException; +import java.nio.charset.CodingErrorAction; import java.nio.charset.StandardCharsets; import java.util.Locale; @@ -26,11 +29,13 @@ * filters declared in {@code web.xml}, so a path an end user controls must never * be allowed to climb out of the location the code intends to dispatch to. *

- * The container percent-decodes a dispatcher path once more before it normalises - * it (Tomcat: {@code dispatchersUseEncodedPaths}, on by default), so every check - * runs on the decoded form as well as on the raw one, and a value that would - * still carry an escape after that single decode - a second encoding layer - is - * refused outright: no in-app path of the product needs one. + * The container percent-decodes a dispatcher path once more and then collapses + * it ({@code //}, {@code /./}, {@code ;params}) before it maps it (Tomcat: + * {@code dispatchersUseEncodedPaths}, on by default), so every check runs on the + * decoded form as well as on the raw one, and the reserved-directory check reads + * the collapsed path. An escape that decodes to a URL delimiter ({@code %25}, + * {@code %3F}, {@code %23}) or to something that is not UTF-8 is refused + * outright: no in-app path of the product carries one. */ public final class ForwardPathValidator { @@ -41,7 +46,7 @@ private ForwardPathValidator() { * Whether {@code path} may be forwarded to as-is: absolute, without {@code ..} * segments in its raw or decoded form (path parameters stripped, as the * container does), without backslashes, control characters, malformed or - * double escapes, and not under a reserved directory. + * delimiter escapes, and not under a reserved directory once collapsed. * * @param path a context-relative path, optionally with a query string * @return {@code true} if the path is safe to pass to a request dispatcher @@ -53,12 +58,21 @@ public static boolean isSafeForwardPath(String path) { int query = path.indexOf('?'); String uri = query == -1 ? path : path.substring(0, query); String resolved = resolve(uri); - if (resolved == null) { + if (resolved == null || isReserved(resolved)) { return false; } - String lower = resolved.toLowerCase(Locale.ROOT); - return !(lower.startsWith("/web-inf/") || lower.equals("/web-inf") - || lower.startsWith("/meta-inf/") || lower.equals("/meta-inf")); + // HttpServletRequest.getRequestDispatcher() drops a fragment before it + // maps the path, so the reserved-directory check has to see that form + // too; a ServletContext dispatcher keeps it, which the traversal checks + // above already cover. + int fragment = resolved.indexOf('#'); + return fragment == -1 || !isReserved(collapse(resolved.substring(0, fragment))); + } + + private static boolean isReserved(String collapsedPath) { + String lower = collapsedPath.toLowerCase(Locale.ROOT); + return lower.startsWith("/web-inf/") || lower.equals("/web-inf") + || lower.startsWith("/meta-inf/") || lower.equals("/meta-inf"); } /** @@ -73,20 +87,39 @@ public static boolean isSafeMetaAlias(String metaAlias) { } /** - * The path as the container will resolve it, or {@code null} when it must not - * be dispatched to: traversal, a backslash or a control character in either - * the raw or the decoded form, or an escape that is malformed or survives the - * container's single decode. + * The path as the container will map it - decoded once and collapsed - or + * {@code null} when it must not be dispatched to: traversal, a backslash or a + * control character in either the raw or the decoded form, or an escape that + * is malformed, not UTF-8, or decodes to a URL delimiter. */ private static String resolve(String value) { if (containsTraversal(value)) { return null; } String decoded = decodeOnce(value); - if (decoded == null || decoded.indexOf('%') != -1 || containsTraversal(decoded)) { + if (decoded == null || containsTraversal(decoded)) { return null; } - return decoded; + return collapse(decoded); + } + + /** + * Collapses a path the way the container does before mapping it: {@code ;params} + * stripped from each segment, empty and {@code .} segments dropped. {@code ..} + * never reaches here. + */ + private static String collapse(String path) { + StringBuilder collapsed = new StringBuilder(path.length()); + for (String segment : path.split("/", -1)) { + int semicolon = segment.indexOf(';'); + if (semicolon != -1) { + segment = segment.substring(0, semicolon); + } + if (!segment.isEmpty() && !segment.equals(".")) { + collapsed.append('/').append(segment); + } + } + return collapsed.length() == 0 ? "/" : collapsed.toString(); } private static boolean containsTraversal(String value) { @@ -113,7 +146,8 @@ private static boolean containsTraversal(String value) { * Percent-decodes {@code value} exactly once, the way the container does for * a path: {@code %XX} with two ASCII hex digits only, {@code +} left alone. * - * @return the decoded value, or {@code null} if an escape is malformed + * @return the decoded value, or {@code null} if an escape is malformed, is + * not valid UTF-8, or decodes to {@code %}, {@code ?} or {@code #} */ private static String decodeOnce(String value) { if (value.indexOf('%') == -1) { @@ -130,7 +164,7 @@ private static String decodeOnce(String value) { continue; } // A run of escapes is one byte sequence: a multi-byte character has - // to be decoded as a whole or its bytes turn into replacement characters. + // to be decoded as a whole. bytes.reset(); while (i < value.length() && value.charAt(i) == '%') { if (i + 2 >= value.length()) { @@ -141,10 +175,25 @@ private static String decodeOnce(String value) { if (hi < 0 || lo < 0) { return null; } - bytes.write((hi << 4) | lo); + int b = (hi << 4) | lo; + if (b == '%' || b == '?' || b == '#') { + // A second encoding layer, or a delimiter the container would + // map literally: never a plain in-app path. + return null; + } + bytes.write(b); i += 3; } - decoded.append(new String(bytes.toByteArray(), StandardCharsets.UTF_8)); + try { + decoded.append(StandardCharsets.UTF_8.newDecoder() + .onMalformedInput(CodingErrorAction.REPORT) + .onUnmappableCharacter(CodingErrorAction.REPORT) + .decode(ByteBuffer.wrap(bytes.toByteArray()))); + } catch (CharacterCodingException e) { + // Overlong or truncated sequences: the container would turn them + // into replacement characters, never into a path this code means. + return null; + } } return decoded.toString(); } diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java index 571b6a86d6..57216f76ab 100644 --- a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java @@ -25,8 +25,7 @@ import jakarta.servlet.RequestDispatcher; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import org.testng.annotations.AfterClass; -import org.testng.annotations.BeforeClass; +import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -43,25 +42,23 @@ public class FSUtilsForwardPathTest { private HttpServletRequest request; private HttpServletResponse response; - @BeforeClass - public void pinDeploymentUri() { - savedDeploymentURI = FSUtils.deploymentURI; - FSUtils.deploymentURI = "/openam"; - } - - @AfterClass(alwaysRun = true) - public void restoreDeploymentUri() { - FSUtils.deploymentURI = savedDeploymentURI; - } - @BeforeMethod public void setUp() { + // forwardRequest reads the static deployment URI; pinned per test and + // restored whatever the outcome. + savedDeploymentURI = FSUtils.deploymentURI; + FSUtils.deploymentURI = "/openam"; request = mock(HttpServletRequest.class); when(request.getServerName()).thenReturn("sp.example.com"); when(request.getServerPort()).thenReturn(8080); response = mock(HttpServletResponse.class); } + @AfterMethod(alwaysRun = true) + public void restoreDeploymentUri() { + FSUtils.deploymentURI = savedDeploymentURI; + } + @Test public void forwardsAnInAppTargetOnTheSameHost() throws Exception { RequestDispatcher dispatcher = mock(RequestDispatcher.class); @@ -84,6 +81,12 @@ public Object[][] traversingTargets() { {BASE + "/x/..%2fWEB-INF/web.xml"}, {BASE + "/%57EB-INF/web.xml"}, {BASE + "/x/%zz"}, + // Forms the container collapses to /WEB-INF/web.xml before mapping. + {BASE + "//WEB-INF/web.xml"}, + {BASE + "/./WEB-INF/web.xml"}, + {BASE + "/WEB-INF;x/web.xml"}, + {BASE + "/%2e/WEB-INF/web.xml"}, + {BASE + "/WEB-INF#/x"}, }; } diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java index 28497885b6..fee68b6d5a 100644 --- a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java @@ -33,6 +33,10 @@ public Object[][] safeForwardPaths() { {"/console/a+b.jsp"}, // '+' is literal in a path {"/web-info/page.jsp"}, // only the real WEB-INF directory is reserved {"/a/b..c/d"}, // ".." inside a segment is an ordinary name + {"/console//base/AMAdminFrame"}, // collapses to an ordinary path + {"/./console/base/AMAdminFrame"}, + {"/console/base;jsessionid=1/AMAdminFrame"}, + {"/XUI/#login/"}, // the container drops the fragment: /XUI/ }; } @@ -72,6 +76,25 @@ public Object[][] unsafeForwardPaths() { {"/x/%zz.jsp"}, {"/x/%2"}, {"/x/100%.jsp"}, + // The container collapses "//", "/./" and ";params" before mapping, + // so the reserved-directory check has to see the collapsed path. + {"//WEB-INF/web.xml"}, + {"/./WEB-INF/web.xml"}, + {"/WEB-INF;x/web.xml"}, + {"/;/WEB-INF/web.xml"}, + {"/%2e/WEB-INF/web.xml"}, + {"/WEB-INF/"}, + {"/WEB-INF/./web.xml"}, + // request.getRequestDispatcher() drops a fragment before mapping. + {"/WEB-INF#/x"}, + {"/WEB-INF/web.xml#x"}, + {"/x#/../WEB-INF/web.xml"}, // a ServletContext dispatcher keeps the fragment + // An escape that decodes to a URL delimiter is never a plain in-app path. + {"/WEB-INF%3fx/web.xml"}, + {"/WEB-INF%23/web.xml"}, + {"/x/\u007f.jsp"}, + {"/x/%7f.jsp"}, + {"/x/%c0%ae%c0%ae/WEB-INF/web.xml"}, // overlong UTF-8 is not a path }; } @@ -111,6 +134,8 @@ public Object[][] unsafeMetaAliases() { {"/%252e%252e/%252e%252e/WEB-INF/web.xml"}, {"/idp%00"}, {"/idp%zz"}, + {"/idp#/../../WEB-INF/web.xml"}, + {"/idp/%c0%ae%c0%ae/WEB-INF/web.xml"}, }; } diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/services/logout/FSReturnLogoutServletTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/services/logout/FSReturnLogoutServletTest.java new file mode 100644 index 0000000000..c1ff15dc21 --- /dev/null +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/services/logout/FSReturnLogoutServletTest.java @@ -0,0 +1,91 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package com.sun.identity.federation.services.logout; + +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import jakarta.servlet.ServletConfig; +import jakarta.servlet.ServletContext; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +/** + * Drives {@code /ReturnLogout/*} through its second alias source: the servlet is + * prefix-mapped, so with no {@code metaAlias} parameter the alias is cut out of + * the raw, still-encoded {@code getRequestURI()}. + */ +public class FSReturnLogoutServletTest { + + private FSReturnLogoutServlet servlet; + private ServletContext servletContext; + private HttpServletRequest request; + private HttpServletResponse response; + + @BeforeMethod + public void setUp() throws Exception { + servletContext = mock(ServletContext.class); + ServletConfig config = mock(ServletConfig.class); + when(config.getServletContext()).thenReturn(servletContext); + servlet = new FSReturnLogoutServlet(); + servlet.init(config); + request = mock(HttpServletRequest.class); + when(request.getParameter("metaAlias")).thenReturn(null); + response = mock(HttpServletResponse.class); + } + + @Test + public void anOrdinaryAliasInTheRequestUriPassesTheGuard() throws Exception { + when(request.getRequestURI()).thenReturn("/openam/ReturnLogout/metaAlias/idp"); + + servlet.doGet(request, response); + + // No session in a unit test: the servlet fails later, but not on the alias. + verify(response, never()).sendError(eq(HttpServletResponse.SC_BAD_REQUEST), anyString()); + verify(response).sendError(eq(HttpServletResponse.SC_INTERNAL_SERVER_ERROR), anyString()); + } + + @DataProvider + public Object[][] traversingRequestUris() { + return new Object[][] { + {"/openam/ReturnLogout/metaAlias/../../WEB-INF/web.xml"}, + {"/openam/ReturnLogout/metaAlias/idp/..%2f..%2fWEB-INF/web.xml"}, + {"/openam/ReturnLogout/metaAlias/%2e%2e/%2e%2e/WEB-INF/web.xml"}, + {"/openam/ReturnLogout/metaAlias/%252e%252e/%252e%252e/WEB-INF/web.xml"}, + }; + } + + @Test(dataProvider = "traversingRequestUris") + public void answers400WithoutDispatchingForATraversingAliasInTheRequestUri(String uri) throws Exception { + when(request.getRequestURI()).thenReturn(uri); + + servlet.doGet(request, response); + + verify(response).sendError(eq(HttpServletResponse.SC_BAD_REQUEST), anyString()); + verify(response, never()).sendError(eq(HttpServletResponse.SC_INTERNAL_SERVER_ERROR), anyString()); + verify(servletContext, never()).getRequestDispatcher(anyString()); + verify(request, never()).getRequestDispatcher(anyString()); + verify(response, never()).sendError(anyInt()); + } +} From 53397347b8f553cc9974d962696e6a1593540e7b Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 17 Sep 2026 15:13:28 +0300 Subject: [PATCH 5/5] Strip path parameters from the raw forward path before decoding it Tomcat's ApplicationContext.getRequestDispatcher strips ";param" on the raw string, from each ';' up to the next raw '/', and only then decodes and normalises. The validator decoded first, so an encoded slash inside a parameter had already become a separator when the parameter was cut per segment: "/;%2Fjunk/WEB-INF/web.xml" read as "/junk/WEB-INF/web.xml" here and as "/WEB-INF/web.xml" in the container. resolve() now strips path parameters on the raw string in the container's order; a ';' produced by an escape (%3B) is refused like the other delimiter escapes, since the container maps it literally. The fragment-stripped form that HttpServletRequest.getRequestDispatcher() maps goes through resolve() as a whole, not only through the reserved-directory check, so a trailing "..#" segment is refused. A '?' in a meta alias is refused: the container cuts the dispatcher path there, which turned "/..?" into a whole ".." segment one level up. Pinned in ForwardPathValidatorTest (87 rows) and FSUtilsForwardPathTest (18): the ";%2F" forms, "/x/..#", "/..#", "/x%3by/", "/..?", plus the two round-2 forms that had no row ("/;x/", "/.;x/"). Twelve rows are red with the previous validator. --- .../common/ForwardPathValidator.java | 93 ++++++++++++------- .../common/FSUtilsForwardPathTest.java | 7 ++ .../common/ForwardPathValidatorTest.java | 16 ++++ 3 files changed, 85 insertions(+), 31 deletions(-) diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java index 8ea68a44fa..5f23cd896a 100644 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java @@ -29,12 +29,16 @@ * filters declared in {@code web.xml}, so a path an end user controls must never * be allowed to climb out of the location the code intends to dispatch to. *

- * The container percent-decodes a dispatcher path once more and then collapses - * it ({@code //}, {@code /./}, {@code ;params}) before it maps it (Tomcat: - * {@code dispatchersUseEncodedPaths}, on by default), so every check runs on the - * decoded form as well as on the raw one, and the reserved-directory check reads - * the collapsed path. An escape that decodes to a URL delimiter ({@code %25}, - * {@code %3F}, {@code %23}) or to something that is not UTF-8 is refused + * The container processes a dispatcher path in a fixed order before it maps it + * (Tomcat {@code ApplicationContext.getRequestDispatcher}, with + * {@code dispatchersUseEncodedPaths} on by default): the query string is cut, + * {@code ;params} are stripped from the raw string (each {@code ;} up to the + * next raw {@code /}), the rest is percent-decoded once, and {@code //} and + * {@code /./} are collapsed. The checks here follow that order: traversal is + * refused in the raw form and again in the decoded form, and the + * reserved-directory check reads the stripped, decoded, collapsed path. An + * escape that decodes to a URL delimiter ({@code %25}, {@code %3F}, + * {@code %23}, {@code %3B}) or to something that is not UTF-8 is refused * outright: no in-app path of the product carries one. */ public final class ForwardPathValidator { @@ -62,11 +66,16 @@ public static boolean isSafeForwardPath(String path) { return false; } // HttpServletRequest.getRequestDispatcher() drops a fragment before it - // maps the path, so the reserved-directory check has to see that form - // too; a ServletContext dispatcher keeps it, which the traversal checks - // above already cover. - int fragment = resolved.indexOf('#'); - return fragment == -1 || !isReserved(collapse(resolved.substring(0, fragment))); + // maps the path, so that form has to pass every check as well (a + // trailing "..#" segment is ".." once the fragment is gone); a + // ServletContext dispatcher keeps the fragment, which the checks + // above cover. + int fragment = uri.indexOf('#'); + if (fragment != -1) { + String withoutFragment = resolve(uri.substring(0, fragment)); + return withoutFragment != null && !isReserved(withoutFragment); + } + return true; } private static boolean isReserved(String collapsedPath) { @@ -83,20 +92,28 @@ private static boolean isReserved(String collapsedPath) { * @return {@code true} if the alias contains no traversal */ public static boolean isSafeMetaAlias(String metaAlias) { - return metaAlias != null && !metaAlias.isEmpty() && resolve(metaAlias) != null; + // The container cuts the dispatcher path at the first '?', so a '?' + // in the alias would end the path early and turn a trailing ".." that + // precedes it into a whole segment; an alias never carries one. + return metaAlias != null && !metaAlias.isEmpty() && metaAlias.indexOf('?') == -1 + && resolve(metaAlias) != null; } /** - * The path as the container will map it - decoded once and collapsed - or - * {@code null} when it must not be dispatched to: traversal, a backslash or a - * control character in either the raw or the decoded form, or an escape that - * is malformed, not UTF-8, or decodes to a URL delimiter. + * The path as the container will map it - path parameters stripped, decoded + * once, collapsed - or {@code null} when it must not be dispatched to: + * traversal, a backslash or a control character in either the raw or the + * decoded form, or an escape that is malformed, not UTF-8, or decodes to a + * URL delimiter. */ private static String resolve(String value) { if (containsTraversal(value)) { return null; } - String decoded = decodeOnce(value); + // The container strips ";param" on the raw string, up to the next raw + // '/', before it decodes: an encoded slash inside a parameter goes with + // the parameter and never becomes a separator. + String decoded = decodeOnce(stripPathParams(value)); if (decoded == null || containsTraversal(decoded)) { return null; } @@ -104,17 +121,36 @@ private static String resolve(String value) { } /** - * Collapses a path the way the container does before mapping it: {@code ;params} - * stripped from each segment, empty and {@code .} segments dropped. {@code ..} - * never reaches here. + * Drops every {@code ;param} the way the container does on the raw path: + * from each {@code ;} up to the next {@code /}. + */ + private static String stripPathParams(String path) { + if (path.indexOf(';') == -1) { + return path; + } + StringBuilder stripped = new StringBuilder(path.length()); + int pos = 0; + while (pos < path.length()) { + int semicolon = path.indexOf(';', pos); + if (semicolon == -1) { + stripped.append(path, pos, path.length()); + break; + } + stripped.append(path, pos, semicolon); + int slash = path.indexOf('/', semicolon); + pos = slash == -1 ? path.length() : slash; + } + return stripped.toString(); + } + + /** + * Collapses a decoded, parameter-free path the way the container normalises + * it before mapping: empty and {@code .} segments dropped. {@code ..} never + * reaches here. */ private static String collapse(String path) { StringBuilder collapsed = new StringBuilder(path.length()); for (String segment : path.split("/", -1)) { - int semicolon = segment.indexOf(';'); - if (semicolon != -1) { - segment = segment.substring(0, semicolon); - } if (!segment.isEmpty() && !segment.equals(".")) { collapsed.append('/').append(segment); } @@ -130,11 +166,6 @@ private static boolean containsTraversal(String value) { } } for (String segment : value.split("/", -1)) { - // The container strips ";param" from each segment before normalising. - int semicolon = segment.indexOf(';'); - if (semicolon != -1) { - segment = segment.substring(0, semicolon); - } if (segment.equals("..")) { return true; } @@ -147,7 +178,7 @@ private static boolean containsTraversal(String value) { * a path: {@code %XX} with two ASCII hex digits only, {@code +} left alone. * * @return the decoded value, or {@code null} if an escape is malformed, is - * not valid UTF-8, or decodes to {@code %}, {@code ?} or {@code #} + * not valid UTF-8, or decodes to {@code %}, {@code ?}, {@code #} or {@code ;} */ private static String decodeOnce(String value) { if (value.indexOf('%') == -1) { @@ -176,7 +207,7 @@ private static String decodeOnce(String value) { return null; } int b = (hi << 4) | lo; - if (b == '%' || b == '?' || b == '#') { + if (b == '%' || b == '?' || b == '#' || b == ';') { // A second encoding layer, or a delimiter the container would // map literally: never a plain in-app path. return null; diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java index 57216f76ab..3b670c9112 100644 --- a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java @@ -85,8 +85,15 @@ public Object[][] traversingTargets() { {BASE + "//WEB-INF/web.xml"}, {BASE + "/./WEB-INF/web.xml"}, {BASE + "/WEB-INF;x/web.xml"}, + {BASE + "/;x/WEB-INF/web.xml"}, + {BASE + "/.;x/WEB-INF/web.xml"}, {BASE + "/%2e/WEB-INF/web.xml"}, {BASE + "/WEB-INF#/x"}, + // ";param" is stripped on the raw string, encoded slash included. + {BASE + "/;%2Fjunk/WEB-INF/web.xml"}, + {BASE + "/;jsessionid=1%2Fa/WEB-INF/web.xml"}, + // request.getRequestDispatcher() drops the fragment: /x/.. -> / + {BASE + "/x/..#"}, }; } diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java index fee68b6d5a..b6dfb2bbd5 100644 --- a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java @@ -36,6 +36,7 @@ public Object[][] safeForwardPaths() { {"/console//base/AMAdminFrame"}, // collapses to an ordinary path {"/./console/base/AMAdminFrame"}, {"/console/base;jsessionid=1/AMAdminFrame"}, + {"/console;x%2Fy/base/AMAdminFrame"}, // the parameter goes, encoded slash and all {"/XUI/#login/"}, // the container drops the fragment: /XUI/ }; } @@ -82,16 +83,28 @@ public Object[][] unsafeForwardPaths() { {"/./WEB-INF/web.xml"}, {"/WEB-INF;x/web.xml"}, {"/;/WEB-INF/web.xml"}, + {"/;x/WEB-INF/web.xml"}, + {"/.;x/WEB-INF/web.xml"}, {"/%2e/WEB-INF/web.xml"}, {"/WEB-INF/"}, {"/WEB-INF/./web.xml"}, + // The container strips ";param" on the raw string up to the next raw + // "/", before it decodes: an encoded slash inside the parameter goes + // with it, and the rest collapses to /WEB-INF/web.xml. + {"/;%2Fjunk/WEB-INF/web.xml"}, + {"/;jsessionid=1%2Fa/WEB-INF/web.xml"}, + {"/;x%2Fjunk/WEB-INF/web.xml"}, + {"/;%2Fa%2Fb/WEB-INF/web.xml"}, // request.getRequestDispatcher() drops a fragment before mapping. {"/WEB-INF#/x"}, {"/WEB-INF/web.xml#x"}, + {"/x/..#"}, // ".." once the fragment is gone + {"/..#"}, {"/x#/../WEB-INF/web.xml"}, // a ServletContext dispatcher keeps the fragment // An escape that decodes to a URL delimiter is never a plain in-app path. {"/WEB-INF%3fx/web.xml"}, {"/WEB-INF%23/web.xml"}, + {"/x%3by/WEB-INF/web.xml"}, {"/x/\u007f.jsp"}, {"/x/%7f.jsp"}, {"/x/%c0%ae%c0%ae/WEB-INF/web.xml"}, // overlong UTF-8 is not a path @@ -116,6 +129,7 @@ public Object[][] safeMetaAliases() { {"/myrealm/sp"}, {"/my-realm/my_sp.1"}, {"/my%20realm/sp"}, // raw request URI form of "/my realm/sp" + {"/idp;x/sp"}, // a path parameter is stripped, not traversed }; } @@ -129,6 +143,8 @@ public Object[][] unsafeMetaAliases() { {"\\..\\..\\WEB-INF\\web.xml"}, {"/idp\0"}, {"/idp?x=/../WEB-INF"}, + {"/..?"}, // the container cuts the path at '?': a whole ".." + {"/idp/..?x=1"}, {"/%2e%2e/%2e%2e/WEB-INF/web.xml"}, {"/idp/..%2f..%2fWEB-INF/web.xml"}, {"/%252e%252e/%252e%252e/WEB-INF/web.xml"},