Security Issue - HTTP Response Splitting

[cschofld]

There is an HTTP response splitting vulnerability in the login redirection. When formatting the service parameter any control characters should be removed.

See https://www.owasp.org/index.php/HTTP_Response_Splitting

[pames]

Thanks for reporting this. To help us better assess the severity of this, can you also share the following information?

1. mod_auth_cas version you are using (if on a distribution, please include the distribution info, or if from git, the commit hash)
2. CAS server you are using
3. details about the client that you are using (e.g. are you seeing this in a web browser - if so, which one? or instead with a program that is using e.g. libcurl)

We don't intentionally unescape anything in the service parameter in any redirects which is why I'd like to better understand how/why you are seeing this issue.

[cschofld]

mod_auth_cas - 1.1 d80ac39
cas-server - 3.5.2
apache - 2.4

The issue came out of penetration testing. The real problem is that the login URL, including the service parameter, is copied to the location header. The service parameter can contain end-of-line markers (CRLF) that would allow a malicious user to insert other headers and content.

The following example, while not malicious, demonstrates the result:
REQUEST:
GET /myapp/userManagement/payload%0d%0ainsertedheader HTTP/1.0
Host: localhost

RESPONSE:
HTTP/1.1 302 Found
Date: Fri, 30 Sep 2016 13:57:27 GMT
Location: https://localhost/cas/login?service=https%3a%2f%2flocalhost%2fmyapp%2fuserManagement%2fpayload
insertedheader
Content-Length: 356
Connection: close
Content-Type: text/html; charset=iso-8859-1

From https://tools.ietf.org/html/rfc2616#section-2.2:

   HTTP/1.1 header field values can be folded onto multiple lines if the
   continuation line begins with a space or horizontal tab. All linear
   white space, including folding, has the same semantics as SP. A
   recipient MAY replace any linear white space with a single SP before
   interpreting the field value or forwarding the message downstream.

       LWS            = [CRLF] 1*( SP | HT )

   The TEXT rule is only used for descriptive field contents and values
   that are not intended to be interpreted by the message parser. Words
   of *TEXT MAY contain characters from character sets other than ISO-
   8859-1 [22] only when encoded according to the rules of RFC 2047
   [14].

       TEXT           = <any OCTET except CTLs,
                               but including LWS>

   A CRLF is allowed in the definition of TEXT only as part of a header
   field continuation. It is expected that the folding LWS will be
   replaced with a single SP before interpretation of the TEXT value.

It looks like the only CTLs that should be allowed are LWS. Even then they could be replaced by a single SP.

Of course, this issue does not cause mod_auth_cas to fail, but a change would make it more secure.

Thank you for maintaining mod_auth_cas.

[dhawes]

Some notes so I don't forget:

Adding '\r' and '\n' to https://github.com/Jasig/mod_auth_cas/blob/master/src/mod_auth_cas.c#L826 could be a quick fix, but I'll need to test.

I do wonder if using something like https://apr.apache.org/docs/apr/1.5/group___a_p_r___util___escaping.html#ga9caffb30731e3a07a8e23fa6464d35b5 would work better in general here.

[pames]

@cschofld thanks for providing the additional extra detail. So, I am not aware of any browser which will actually take action when the response code is a 302 but more than 1 Location header, which I think is what would really be necessary to achieve "bad things". If you know of one, please let us know.

That said, I agree we should fix this. @dhawes my main hesitation with your proposal is about how that particular function is used, what contracts it commits anyone to, etc. Given that a service is("SHOULD"?) required to be a valid URL, I wonder if we can leverage that and e.g. apr_uri_unparse or similar constructs to make us consistently safe throughout.

All that said, a simple pass over any decoded service parameter to ensure everything lies in the safe printable ASCII space is probably a suitable approach also, although I'm a little surprised that Apache wouldn't encode them as they were passed in...

[pames]

@dhawes OK I had some time to look at how we use that function, and the implementation as well. Looks like that interface was added to APR in 2013. The implementation also references the RFC1738 character set which is in the comment in mod_auth_cas.c, so it may not encode newlines (we would need to test, their existing unit tests don't answer that question).

My suggestion would be to actually just replace this with something that URL encodes anything not in ASCII printable (so anything in [0x00-0x20), [0x7F, 0xFF] or "+ <>"%{}|^~[]`;/?:@=&#" as we currently do.

[dhawes]

@pames I did a quick test with apr_pescape_urlencoded() and it worked as expected (my CAS server was not too happy with it though).

That said, I think your suggestion is probably more straightforward and simple.