apereo / phpCAS

Apereo PHP CAS Client
https://apereo.github.io/phpCAS/
Apache License 2.0
796 stars 397 forks source link

Using urlencode() on ticket in SAML payload #232

Closed Offlein closed 7 years ago

Offlein commented 7 years ago

Hello,

My company is providing a CAS implementation to a client who needs us to gather extra user attributes from their server via the /samlValidate URL. However, no matter what, their server is considering the passed ticket to be invalid.

While inspecting the traffic being sent, I noticed that the ticket received from the server includes the server port following a colon. This : character is urlencoded to be %3A, via the _buildSAMLPayload() method in Client.php. If I hack the library to remove this urlencoding, the ticket is accepted as expected.

I tried to trace this issue and more-or-less appreciate why the urlencoding was added in the first place, but it's not clear if all CAS servers are currently expected to un-encode the token before validating it, or if the expectation goes beyond that, or what. Or, somehow, if I am misusing it.

Any clarification of intent would be appreciated!

jfritschi commented 7 years ago

Please have a look at the official protocol specification. https://apereo.github.io/cas/5.0.x/protocol/CAS-Protocol-Specification.html

I however would speculate this is a side effect from security issues we had. Since ticket are often used in the URLs and other sensitive areas any the use without urlencoding offers injection possibilities. This would be my first guess.

Having a : in the ticket seems very odd and I would not recommend this. Since the ticket suffix is an configuration value on the server side you should fix this on the server side and use "clean" characters only.

Offlein commented 7 years ago

Hi Joachim, thanks for the reply. When you say "the server side", you mean the CAS server has the ability to configure the format that their tickets are generated in?

This makes sense to me, but I am not the administrator of the CAS server in question. I am providing a service to multiple clients, all of whom configure their CAS servers independently. The client exhibiting the problem is using Jasig CAS 3.6.0. I see there is documentation (at least for other versions of CAS) that has config for a HostNameBasedUniqueTicketIdGenerator.

I assume this is what they're using. In their case, they're using a non-standard port, however, and that is where the colon is being inserted from (as a delimiter between the hostname and its port). I'll comment this to the client, however.

I did look at that protocol spec, and -- unless I'm missing it -- I don't see anything about colons or other safe/unsafe characters in the Service Ticket specification.

It totally makes sense why we'd want to do the urlencoding, but it seems like maybe that's not a reliable method of securing these tickets?

Thanks!

jfritschi commented 7 years ago

Yes, server side I mean on the CAS server.

If you do not use any specific custom generation the ticket suffix dependes on the

cas.host.name=

in the cas.properties file. This is appended to the ticket.

During the second check I found the right part of the protocol specs. It's a bit hidden:

3.7. ticket and ticket-granting cookie character set In addition to the above requirements, all CAS tickets and the value of the ticket-granting cookie MUST contain only characters from the set {A-Z, a-z, 0-9}, and the hyphen character -.

With unsafe I do not refer to the protocol specs but more common sense/experience. Any special characters or umlauts are fairly certain to trip you up at some point. There are so many CAS clients, languages, encodings out there that these characters will get you in trouble at some point.

Urlencoding might not be the best choice in this case but it's about erring on the side of caution. Probably some XML filtering might be better. With the protocol specs like they are this does not seem to be an issue though.

Offlein commented 7 years ago

Hello! Got it -- I can't thank you enough for taking the time to explain this. This is really great.

Espeically appreciated is the protocol specs. I'll let the client know right away; that makes it quite clearly a legitimate issue on their side. Thanks so much. I'll close this ticket.