Conversation
…ST 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 OpenIdentityPlatform#120, OpenIdentityPlatform#164, OpenIdentityPlatform#173, OpenIdentityPlatform#174, OpenIdentityPlatform#196, OpenIdentityPlatform#197, OpenIdentityPlatform#198, OpenIdentityPlatform#208, OpenIdentityPlatform#209
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…arget - 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four independent hardenings from the CodeQL high triage, grouped because each is a few lines.
ID-FF forwards (
java/unvalidated-url-forward#196, #197, #198)FSSingleLogoutServlet(/liberty-logout, unauthenticated) andFSReturnLogoutServletappend themetaAliasrequest parameter to the/ProcessLogout/metaAliasdispatcher path unchecked, andFSUtils.forwardRequestforwards to any same-host path taken fromgoto/LRURL. ARequestDispatchercan reach/WEB-INFand does not run theweb.xmlfilters, sometaAlias=/../../WEB-INF/web.xmlserved the file.New
ForwardPathValidator: no..segments (path parameters stripped the way the container does), no backslashes or control characters, not under/WEB-INFor/META-INF. The logout servlets answer 400 for a traversing alias, andforwardRequestanswers 400 when the target is not a plain in-app path (a rejected target is never a legitimate flow).FilesRepo (
java/path-injection#208, #209;java/regex-injection#164)constructFilebuiltnew File(typeDir, name)straight from the identity name; with a Files data store configured an identity could be created, read or deleted outside the repository. Names containing path separators, NUL,.or..(or empty/null) now raiseIdRepoException(ILLEGAL_ARGUMENTS).FileRepoFileFiltercompiled the search pattern as a regex after replacing only*; literal parts are nowPattern.quote()d (a(bno longer throws,user.1no longer matchesuserx1).SAML 1.x POST profile (
java/xss#120)SAMLUtils.postToTargetwrote the target URL intoFORM ACTIONunescaped;postYN()validates host, port and path but not the query string. Escaped withStringEscapeUtils.escapeHtml4(commons-lang3, already on the module classpath; a sanitizer CodeQL models).OAuth module (
java/insecure-randomness#173, #174)The CSRF state token id (CTS key, also the
NONCE_TOKEN_IDcookie) came fromRandomStringUtils.randomAlphanumeric(java.util.Random).newCsrfStateTokenId()now draws from the module'sSecureRandom. Thestatevalue itself was alreadyUUID.randomUUID().CodeQL on this PR
The first revision drew two new findings on the changed lines: the
FORM ACTIONescaper was not one CodeQL recognises (#489) and the forward guard's redirect fallback reused the request URL (#488). 76ef05a switches toescapeHtml4and to a 400 response respectively; both threads are answered inline.Tests
ForwardPathValidatorTest(29 cases),SAMLUtilsTest,FilesRepoTest(13),OAuthTest+1. Full suites of openam-federation-library (73) and openam-auth-oauth2 (5) pass.Closes CodeQL alerts #120, #164, #173, #174, #196, #197, #198, #208, #209.