Mitigate xss issue

[mhindery]

The example template for the form post is vulnerable to a reflective XSS attack, by injecting via the next parameter, e.g.:

[your_host]/saml2/login/?idp=[something]?idpid=[something]&next=a"><img%20src=x%20onerror="alert(1)

[mhindery]

@knaperek / @OskarPersson

[knaperek]

Have you actually tried exploiting this?

As far as I remember, the explicit |safe filter is applied in the template because we need to be able to submit SAMLRequest, which is an XML document.
The next parameter is verified in the view already and it will not be passed into the template unless it is a valid URL to a whitelisted destination.

https://github.com/knaperek/djangosaml2/blob/46f9b836bb060e196d4088c65f63e363e66e6084/djangosaml2/views.py#L121-L124

[mhindery]

Yes, I do have a working exploit available. Checkout the branch xss-demonstration from my repo here: https://github.com/OTA-Insight/djangosaml2idp/tree/xss-demonstration. Then fire up the demo setup, which contains an SP and an IDP running via docker (more instructions here if necessary):

cd example_setup
docker-compose up -d

Now visit the sp's saml2 login page, which uses the vulnerable template (see the config here):

http://localhost:8000/saml2/login/?idp=http://localhost:9000/idp/metadata/&next=a%22%3E%3Cimg%20src=x%20onerror=%22alert(document.cookie)

And you'll see the script passed via the url being executed (which in this case will simply print your cookie content). After clicking ok, you'll be redirected to the idp.

If you switch out the commented lines for the one which uses the template without the safe filters. That one does not execute the script, and the login still works.

[mhindery]

Regarding the valid url checking: this doesn't actually check that the url is safe. It merely checks the host and the scheme, which isn't enough, as this is an escaping issue. In fact the latest django release has added a deprecation warning to this function, in order to make clearer what it does, https://docs.djangoproject.com/en/3.0/_modules/django/utils/http/:

def is_safe_url(url, allowed_hosts, require_https=False):
    warnings.warn(
        'django.utils.http.is_safe_url() is deprecated in favor of '
        'url_has_allowed_host_and_scheme().',
        RemovedInDjango40Warning, stacklevel=2,
    )
    return url_has_allowed_host_and_scheme(url, allowed_hosts, require_https)

[knaperek]

gotcha. Then I believe we should explicitly mark the SAMLRequest parameter as safe in the view (before it's passed onto template engine).

[mhindery]

Why do we need to do so at all? It works without doing so, the content of that variable is just a base64 string, which doesn't have a need for being marked as safe (unless I'm missing something):

[knaperek]

You're right, I don't really remember why it was there. But it's really not needed, so let's remove the unsafe |safe filter :-) Thanks a lot for this.