Another Horizon login page vulnerability to a DoS attack

[Timur Sufiev]

This bug is very similar to: https://bugs.launchpad.net/bugs/1394370

Steps to reproduce:
1) Setup Horizon to use db as session engine (using this doc: http://docs.openstack.org/admin-guide-cloud/content/dashboard-session-database.html). I've used MySQL.
2) Run 'for i in {1..100}; do curl -b "sessionid=aaaaa;" http://HORIZON__IP/auth/login/ &> /dev/null; done' from your terminal.
I've got 100 rows in django_session after this.

I've used devstack installation just with updated master branch.

[Tristan Cacqueray (tristan-cacqueray)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

The Horizon Liaison and Paul McMillan have been subscribed. Is this a regression or this case was not covered by previous patch ( https://review.openstack.org/#/c/140358/1 ) ?

[Timur Sufiev (tsufiev-x)]

IMO, that's the case not covered by previous patch.

[Lin Hua Cheng (lin-hua-cheng)]

The session object is getting created because of the call "user_id = request.session[auth.SESSION_KEY]" in
https://github.com/openstack/django_openstack_auth/blob/master/openstack_auth/utils.py#L60-L64

Anytime the session is accessed, it triggers the session record to be created.

The call above is not executed until request.user is access the first time, because the call is setup as Lazy object in:
https://github.com/django/django/blob/master/django/contrib/auth/middleware.py#L22

However, the horizon middleware always access request.user and this triggers the code in get_user() to be executed.

https://github.com/openstack/horizon/blob/master/horizon/middleware.py#L93

We should avoid accessing request.user if the user is not yet authenticated, don't know if that is possible though.

[Paul Karikh (pkarikh)]

Also I've find out that even if we add the following lines (thanks to Lin Hua Cheng for idea!) into horizon/middleware.py:
(https://github.com/openstack/horizon/blob/master/horizon/middleware.py#L93)
if request.path == '/auth/login/':
            return None

we still have problems.
Because it this case create() method will be triggered by translation moddleware: This line
`language = translation.get_language_from_request(
            request, check_path=self.is_language_prefix_patterns_used)`
https://github.com/django/django/blob/master/django/middleware/locale.py#L24

will cause an attempt to access to the session here:
lang_code = request.session.get(LANGUAGE_SESSION_KEY) (https://github.com/django/django/blob/master/django/utils/translation/trans_real.py#L491)

And after it it calls SessionStore.load() method and since there django tries to find session which does not exist, it creates it. https://github.com/django/django/blob/master/django/contrib/sessions/backends/db.py#L29

So it could be a Django bug, as for me. Looks like they have some problems in cached_db with session keys. https://www.djangoproject.com/weblog/2015/may/20/security-release/ May be this bug is next their problem?

[Paul Karikh (pkarikh)]

Even if a disable 'django.middleware.locale.LocaleMiddleware', we still will create session from here:
https://github.com/openstack/django_openstack_auth/blob/master/openstack_auth/views.py#L67
if not request.is_ajax():
        # If the user is already authenticated, redirect them to the
        # dashboard straight away, unless the 'next' parameter is set as it
        # usually indicates requesting access to a page that requires different
        # permissions.
        if (request.user.is_authenticated() and
                auth.REDIRECT_FIELD_NAME not in request.GET and
                auth.REDIRECT_FIELD_NAME not in request.POST):
            return shortcuts.redirect(settings.LOGIN_REDIRECT_URL)

When request.user.is_authenticated() will be called, it will make SimpleLazyObject make an instance of User object, which will make DOA utils.py call user_id = request.session[auth.SESSION_KEY], and we will come to the django SessionStore.load() method again.

[Paul Karikh (pkarikh)]

But looks like now I've triaged our bug.

Here three steps:
1) Add
if request.path == '/auth/login/':
            return None

above this line https://github.com/openstack/horizon/blob/master/horizon/middleware.py#L93

2) Disable 'django.middleware.locale.LocaleMiddleware' in settings.py
3) Remove 'request.user.is_authenticated()' from this line: https://github.com/openstack/django_openstack_auth/blob/master/openstack_auth/views.py#L72

After this curl request from the bug description do not make horizon create a lot of sessions in database. And also it looks like as a user I can use Horizon normally: I can login and browse pages.
But unit-tests are failing now. With error:

/horizon/horizon/middleware.py", line 98, in process_request
    var = request.user.__repr__()
AttributeError: 'HttpRequest' object has no attribute 'user'

[Paul Karikh (pkarikh)]

Sorry, my fault. Failing unittests were caused by my debugging code. I've cleared it up and now unit-tests pass ok.
Integrations tests are failing locally. /settings/ page now broken with
`AttributeError at /settings/
'WSGIRequest' object has no attribute 'LANGUAGE_CODE'`
since I've disabled LocaleMiddleware.

[Paul Karikh (pkarikh)]

Despite I still think that it is a Django issue, maybe we could somehow skip localisation middlware for login page request. And add lines from steps 1 and 3 from my comment #6. Also we will need to remove language-related lines from this file: https://github.com/openstack/horizon/blob/stable/kilo/openstack_dashboard/dashboards/settings/user/views.py#L36
If so, our loss will not be so large. But this workaround still very dirty, as for me.

[Paul Karikh (pkarikh)]

So, I'm not sure if I should file a bug for django team or just ping Paul McMillan.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks Paul Karikh for this investigation, can you attach the patch you are working on here ?

To sum up, the session is created at every call of request.user.* or request.session.*.
Horizon can prevent some of them (see Step 1 and 2 from comment #6) but still, the local middleware will create un-wanted session.

Paul McMillan: we'd like to hear what's django take on that. Is this something that would be fixed in django or should we proceed with Paul Karikh plan ?

[Paul Karikh (pkarikh)]

Here the patch for DOA.

[Paul Karikh (pkarikh)]

Here the patch for Horizon.

[Lin Hua Cheng (lin-hua-cheng)]

Looked at the two patches:
1. this add some weirdness in the behavior.

If the user is unauthenticated and hit: http://localhost:8000/auth/login/ , it will redirect to http://localhost:8000/auth/login/?next=/

2. Horizon
- checking the login_url (/auth/login) doesn't catch all, as the user hits "/" when logging in
- we can't remove the localemiddleware, translations won't work without it

[Lin Hua Cheng (lin-hua-cheng)]

The issue is described in here: http://stackoverflow.com/questions/4444758/huge-django-session-table-normal-behaviour-or-bug/4456686#comment24669604_4456686

I don't see any recommendation on how to mitigate the issue though.

I've sent and email to django security team (<email address hidden>) for help. Will update when I get a response.

[Matthias Runge (mrunge)]

Ugh, fiddling around with request paths looks ugly and may lead to more unexpected behaviour.

I agree with Lin, disabling the localization is not an option.

[Matthias Runge (mrunge)]

That being said, -1 to both patches

[Paul Karikh (pkarikh)]

I completly agree that both patches are rough. I added these patches only to show that we can localize the problem and where is the problem.
Looks like that all mentions of this problem I've found has only one conclusion: it is the expected behaviour for django and it is ok (stackoverflow link from Lin Hua Cheng is a good example).
So I think a response from django team could be useful. If they will say that it is not a bug in django. we will have to handle it. And actually right now I have no idea how we can do it correct.

[Lin Hua Cheng (lin-hua-cheng)]

It seems like we have two problems here:
1. Horizon code accessing request.user and request.session to meet some functionality

2. LocaleMiddleware - it seems it is accessing request.session, even for Django 1.8, it is still there. I am waiting for confirmation from django sec if that is a valid DOS vector.

https://github.com/django/django/blob/1.7.8/django/utils/translation/trans_real.py#L494-L498

One option provided to avoid the issue is using cookie backend, with json serializer (picke has a security issue). Unfortunately, this won't work as the information stored in our session is too big to fit in the cookie. I had a proposal to trim down the size of the session, but people prefer to shove everything in session instead of having extra setup of django cache.

[Paul Karikh (pkarikh)]

@lin-hua-cheng have you any feedback from django team?

[Lin Hua Cheng (lin-hua-cheng)]

I implemented one of the recommended fix, don't create session all time time, created it only if session is modified. I am still waiting for django team for the feedback.

So far from my testing, it looks promising. I don't see any regression and it solved the issue reported.

Still waiting feedback from the rest of the django team.

[Paul Karikh (pkarikh)]

I think we really need a feedback from django team. Because as for, the only way we can decrease the impact is to make patched version of LocalMiddleware and use it. But as for me, it does not make sense.

[Tristan Cacqueray (tristan-cacqueray)]

Lin, will the django fix sufficient on its own, or does Horizon also requires code change to squash this bug ?

[Lin Hua Cheng (lin-hua-cheng)]

The fix suggested by django sec looks promising. Tested it on horizon and works well without additonal changes on our side.

The change is to remove this line: https://github.com/django/django/blob/master/django/contrib/sessions/backends/db.py#L29

This creates the session when no session is found.

The change has been proposed to the rest of the django team, and seems no objection with the change so far.

Tristan, we should probably just wait til Django fix it on their side. It is not a bug that is exposed recently, so waiting a little bit longer shouldn't hurt.

[Paul Karikh (pkarikh)]

@lin-hua-cheng do we have any estimates from django team about when they are going to release this fix for Django?

[Lin Hua Cheng (lin-hua-cheng)]

@pkarikh No estimate yet, will drop them a note today.

[Lin Hua Cheng (lin-hua-cheng)]

@pkarikh: Heard back from django sec, there is an upcoming security release in the works that will include this fix. It
should be out within the next week or two.

[Tristan Cacqueray (tristan-cacqueray)]

This is a class C2 vulnerability. We are waiting for an upcoming release of django to close that issue.

I subscribed ossg-coresec to discuss an eventual OSSN.

[Paul Karikh (pkarikh)]

@lin-hua-cheng
Looks like security release has bean rolled out (1.8.3, 1.7.9, 1.4.21). https://www.djangoproject.com/weblog/2015/jul/08/security-releases/
Should we do something except for bumping django version in requirements to 1.8.3 current master? Looks like for stable/juno we have requrement 'Django>=1.4.2,<1.7'. But for these versions there is not security update, as far as I understand.

[Jeremy Stanley (fungi)]

We normally don't increase upper bounds on requirements in stable branches. Does horizon 2014.2.x actually work with Django 1.8? If not, is it possible to modify it to work without significant risk of introducing new regressions and behavior changes? This is primarily a concern for people continuously deploying stable/juno from source. Any distributions which packaged 2014.2 will almost certainly have security fixes backported to the release of Django they're shipping rather than upgrading to a later Django release.

Anyway, these are conversations which can be had in public now that we won't be disclosing the Django vulnerability by opening this bug report.

[Travis McPeak (travis-mcpeak)]

OSSN seems appropriate for this once we have guidance for a proper approach to mitigating this.

[Matthias Runge (mrunge)]

We can not increase upper bounds here.

I agree, Debian shipped 2014.2 with django-1.7, but e.g for Django-openstack-auth we just recently increased the upper cap to include django-1.7.

[Lin Hua Cheng (lin-hua-cheng)]

@pkarikh: We don't support Django 1.8 yet, working on it on L release.

@fungi: It might be too risky to backport the changes to stable branch.

[Paul Karikh (pkarikh)]

I had a conversation with Lin Hua Cheng in IRC about this bug, and looks like we have nothing to do with this bug since it is a Django bug and since we cannot restrict user to use only exact version.
As far as I understand, the only thing is should be done is to mention this bug and django version with fix in the security notes.
So I set "won't fix" status for this bug in Horizon, if there is no objections.

[Paul Karikh (pkarikh)]

Oh, looks like I have no permissions to set it to won't wix.

[Nathan Kinder (nkinder)]

There is not much we can recommend in an OSSN until we support running Django 1.8 with Horizon. I think we need to hold off on the OSSN until that time, unless there is some sort of external rate limiting that can be done to mitigate the issue.

[Travis McPeak (travis-mcpeak)]

Sessions fix was merged into 1.7.9 in Django unless I'm missing something? Horizon does support the 1.7.x branch, right?

[Lin Hua Cheng (lin-hua-cheng)]

Yes, horizon does support django 1.7.9.

And in L working on:
- support for Django 1.8.
- dropping of Django 14, 1.5, 1.6 which django will stop supporting by end of L

[Matthias Runge (mrunge)]

Kilo supports django-1.7.x
Horizon in Juno (and esp. django_openstack_auth) support only Django-1.6.

[Travis McPeak (travis-mcpeak)]

@nkinder - So we could write a security note describing the issue and recommending Django upgrades for Kilo deployments. We don't currently have any advice for Juno deployments.

@all - Is this an accurate description of our current state?

[Robert Clark (robert-clark)]

Security project is happy to create an OSSN for this but we're waiting for the upstream fix to provide complete guidance to OpenStack consumers on how to mitigate this issue.

[Robert Clark (robert-clark)]

We've created an OSSN for this now, please can interested developers review:

https://review.openstack.org/219885

[Nathan Kinder (nkinder)]

This has been published as OSSN-0054:

  https://wiki.openstack.org/wiki/OSSN/OSSN-0054