Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141926972
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -82,8 +87,17 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
if (request != null) {
String ticket =
request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
+ Credentials ticketCredentials =
ticketService.validateTicket(ticket);
+ if (ticketCredentials != null) {
+ String username = ticketCredentials.getUsername();
+ if (username != null)
+ credentials.setUsername(username);
+ String password = ticketCredentials.getPassword();
+ if (password != null)
+ credentials.setPassword(password);
+ }
AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
-
authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+ authenticatedUser.init(credentials.getUsername(),
credentials);
--- End diff --
This is dangerous. Consider the following flow:
1. A malicious user sets the `username` query parameter to a known user
(say, "guacadmin"), knowing that Guacamole will automatically populate the
`Credentials` object with that value.
2. `ticketService.validateTicket()` does not set the username due to
`principal.getName()` returning `null`, thus the username of `credentials`
remains unchanged.
3. After CAS authentication completes, the user is given the identity of
the user they specified, rather than the identity verified by CAS.
The identity of the user should come purely from CAS.
I would suggest:
* Passing the main `Credentials` object into `validateTicket()`, relying on
`validateTicket()` to handle assignment of username/password values.
* Going back to simply returning the username, rather than a `Credentials`
object which must be manually copied into the main `Credentials` object,
trusting *only* the value which comes from CAS.
---
