Bug 39720 - backend side rate limiting for password reset self service
backend side rate limiting for password reset self service
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: Self Service
UCS 4.1
Other Linux
: P5 normal (vote)
: UCS 4.1-0-errata
Assigned To: Daniel Tröder
Florian Best
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-03 08:04 CET by Daniel Tröder
Modified: 2015-12-10 13:40 CET (History)
3 users (show)

See Also:
What kind of report is it?: ---
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Tröder univentionstaff 2015-11-03 08:04:05 CET
The UMC backend of the password reset self service needs some kind of protection
* from DOS on itself: high DB (PostgreSQL and LDAP) and memory load on dc master/backup,
* for the users mail boxes in case of email reset method,
* for the companies phone bill in case of SMS reset method.

When this is done, the "Token for user 'test' still valid. Please retry in one hour." test and message can be removed from the UMC code.
Comment 1 Daniel Tröder univentionstaff 2015-11-17 14:27:45 CET
I suggest to use memcached for persistence of a request count, as it does not incur any I/O. It'd store IP-date pairs with a UCR configurable decay.

u-s-s-umc must then add a Depend: on 'memcached' and 'python-memcache' and bring its own conf file for memcached (the init script reads /etc/memcached_*.conf).

Thoughts?
Comment 2 Daniel Tröder univentionstaff 2015-11-17 15:03:25 CET
In the UMC module we cannot know the IP address of a user that connects through a web frontend.

IMHO that is not necessary, rules should be configurable that limit:
* total requests per {day, hour, min}
* request per user per {day, hour, min}

* Limiting total requests (tr) per min protects the server owner against DOS on the server.
* Limiting total requests (tr) per day protects the server owner against a high phone bill.
* Limiting requests per user (ur) per d+h+m protects users mailboxes/mobiles.

Multiple rules configurable in UCRVs are necessary. I suggest as a safe default:

* tr: 60/min (DOS DB)
* tr: 100/h (DOS self-service)
* tr: 100/d (phone bill)
* ur: 1/min
* ur: 6/h
* ur: 10/d
Comment 3 Daniel Tröder univentionstaff 2015-11-20 15:55:29 CET
Code + YAML: r65809

This adds UCRVs
umc/self-service/passwordreset/limit/total/.*
umc/self-service/passwordreset/limit/per_user/.*
umc/self-service/passwordreset/token_validity_period
with the following default values:

umc/self-service/passwordreset/limit/total/min: 60
umc/self-service/passwordreset/limit/total/hour: 100
umc/self-service/passwordreset/limit/total/day: 100
umc/self-service/passwordreset/limit/per_user/min: 1
umc/self-service/passwordreset/limit/per_user/hour: 6
umc/self-service/passwordreset/limit/per_user/day: 10
umc/self-service/passwordreset/token_validity_period: 3600

At the beginning of each request counters for all requests and for those of the connecting user are incremented. When they reach the configured amount, at error is returned.

A scipt is installed in /usr/lib/univention-self-service/univention-self-service-request-count (by accident currently not executable) that will display the request count stored in the memcached database.

BTW: this does not limit the request rate to the password _change_ service.

... just saw, that a current code change seems to have broken the check, it is counting but not stopping... not fixed yet...
Comment 4 Daniel Tröder univentionstaff 2015-11-23 10:23:42 CET
Commit 65839 adds the missing str→int conversion.
It also raises the request limits, because for a single password reset process at least 3 requests are necessary.
YAML was updated with r65840.
Comment 5 Daniel Tröder univentionstaff 2015-11-25 08:33:45 CET
pylibmc 1.2.2-1 (for binary package python-pylibmc) built to errata4.1-0
Comment 6 Florian Best univentionstaff 2015-12-01 18:52:11 CET
I receive the message after some requests (e.g. 15?):
"""The allowed maximum number of connections to the server has been reached. Please retry later."""

Please tell how long one should have to wait. I waited about 15 minutes and still no luck.
Comment 7 Florian Best univentionstaff 2015-12-01 18:55:43 CET
Memcached runs as root, I am not sure it this is the best idea, even when using UNIX sockets:
http://blog.couchbase.com/memcached-security
Comment 8 Florian Best univentionstaff 2015-12-01 19:03:44 CET
"Die erlaubte maximale Anzahl an Verbindungen zum Server wurde erreicht.Bitte versuchen Sie es später noch einmal."
Missing space between the two sentences.
"noch einmal" → "erneut".
Comment 9 Daniel Tröder univentionstaff 2015-12-02 09:24:07 CET
(In reply to Florian Best from comment #6)
> I receive the message after some requests (e.g. 15?):
> """The allowed maximum number of connections to the server has been reached.
> Please retry later."""
> 
> Please tell how long one should have to wait. I waited about 15 minutes and
> still no luck.
How exact do you wish the message to be?
"Please wait one {minute, hour, day}." or
"Please wait {one minute, 34 minutes, 18 hours}."?
If multiple limits were hit, it would alway only display the largest one.
Comment 10 Daniel Tröder univentionstaff 2015-12-02 17:57:12 CET
66053: run memcached as non-privileged user
66068: return waiting time if request limit is reached and a lot of other fixes
66069: update yaml
Comment 11 Florian Best univentionstaff 2015-12-04 16:08:20 CET
Please don't use 0 as default value when evaluating the UCR variables. Use the values which are also set in postinst.
Comment 12 Daniel Tröder univentionstaff 2015-12-07 10:34:13 CET
That is intentional. The implicit default for the unset UCRV is 0, as that does the same as deactivating the limit.

The values in the postinst are just meant as suggestions, not defaults.

I have amended the UCRV description to reflect this.

Text change: 66120
YAML adaption: 66121 (→ 1.0.3-7.54.201512071030)
Comment 13 Florian Best univentionstaff 2015-12-07 11:34:24 CET
(In reply to Daniel Tröder from comment #12)
> That is intentional. The implicit default for the unset UCRV is 0, as that
> does the same as deactivating the limit.
No it doesn't. Having a value of 0 causes the limit to always show "Please retry in 24 hours.".
Comment 14 Florian Best univentionstaff 2015-12-07 17:59:52 CET
Works now with the latest changes. I wrote a test case in svn r66129.
YAML: adjusted in svn r66131.
Comment 15 Janek Walkenhorst univentionstaff 2015-12-09 16:48:24 CET
<http://errata.software-univention.de/ucs/4.1/24.html>
Comment 16 Erik Damrose univentionstaff 2015-12-10 13:05:18 CET
python-pylibmc was not announced to maintained with the initial release of erratum 24. It will be released via Bug #40209
Comment 17 Janek Walkenhorst univentionstaff 2015-12-10 13:40:20 CET
<http://errata.software-univention.de/ucs/4.1/29.html>