Bug 28260 - UMC-Module sollten PatternSanitizer als filter benutzen, weil RegexDoS
UMC-Module sollten PatternSanitizer als filter benutzen, weil RegexDoS
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC (Generic)
UCS 3.0
Other Linux
: P4 normal (vote)
: UCS 3.2
Assigned To: Florian Best
Alexander Kläser
https://www.owasp.org/index.php/Regul...
: interim-1
Depends on: 27720
Blocks: 32177
  Show dependency treegraph
 
Reported: 2012-08-21 10:09 CEST by Florian Best
Modified: 2021-06-23 07:29 CEST (History)
5 users (show)

See Also:
What kind of report is it?: Security Issue
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): Security
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Best univentionstaff 2012-08-21 10:09:47 CEST
Momentan wird fnmatch (und regex im package-Modul) benutzt, um Suchergebnisse zu filtern. fnmatch benutzt intern eine regex. Diese erlaubt die Benutzung von '*'. Ein evil-pattern kann benutzt werden, um das System mächtig auszulasten.
Einen Absturz des Systems konnte ich noch nicht produzieren, dafür aber CPU Lasten von ~95%.

In Bug #27720 wurde ein PatternSanitizer für die Suchmuster erstellt, dieser bereinigt das Suchmuster in eine sichere regular expression.
Die UMC-Module sollten diesen Sanitizer benutzen.

für weitere Infos zu regex DoS: siehe Bugzilla-URL.

---
aus Bug #27720c5
> Nein, das sollte kein (neues) Problem sein:
> 
> Erste Möglichkeit: ReDoS findet hier keine Anwendung, ich zitiere:
> 
> Evil Regex pattern contains:
>  * Grouping with repetition
>  * Inside the repeated group:
>  * * Repetition
>  * * Alternation with overlapping
> 
> Das würde ich mal stark annehmen. Die Regexp aus dem Sanitizer kennt nur
> Buchstaben und .*, eben keine Gruppen oder anderes Zeug. Das sollte kein
> Problem darstellen.
> 
> Zweite Möglichkeit: ReDoS findet Anwendung, weil Python eine schlechte
> Implementierung hat. Dann sind wir auch mit fnmatch verwundbar, denn fnmatch
> verwendet intern auch reguläre Ausdrücke (und sogar "interessantere" als es der
> Sanitizer erlaubt). Siehe /usr/lib/python2.7/fnmatch.py +78
> 
> Wenn das der Fall ist, ist das Problem sehr viel größer und wir müssen ab
> sofort unseren eigenen Suchalgorithmus implementieren. Wäre gar nicht so schwer
> (wir erlauben ja nur *), aber da ich davon ausgehe, dass ReDoS keine Anwendung
> findet, sollte man davon absehen.
> 
> Aber der Hinweis auf ReDoS ist jedenfalls gut und zeigt, wie wichtig das
> einheitliche Behandeln solcher Fälle ist (das alte umc-packages hat noch ein
> echtes re.compile verwendet, das wäre vielleicht ein wenig verwundbar).
Comment 1 Florian Best univentionstaff 2012-08-21 11:31:53 CEST
Zum Nachstellen:
session=$(curl -s -i -H 'Content-Type: application/json' -d '{"options": {"version": 1, "username": "Administrator", "password": "univention"}}' http://localhost/umcp/auth | grep 'X-Umc-Session-Id: ' | sed 's/\r//g')

python -c "open('test', 'w').write('{\"options\":{\"category\":\"all\",\"filter\":\"%s\"}}' % ('A*' * 50000000,))"

curl  -i -H 'Content-Type: application/json' -H "$session" -d@test http://localhost/umcp/command/top/query; echo

Der curl-Prozess kann dann auch abgebrochen werden, der Server wird weitermachen...

aus top:
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
16091 root      20   0  762m 745m  820 R 98.8 73.8   3:51.23 univention-mana

Die Logfiles werden auch vollgespammt mit 6 Einträgen pro Sekunde:
19.08.12 00:26:16.630  PARSER      ( INFO    ) : The message body is not complete: 17792952 of 100000046 bytes
19.08.12 00:26:16.630  MAIN        ( INFO    ) : MagicBucket: incomplete message: Der Nachrichtenrumpf ist (noch) nicht vollständig
Comment 2 Florian Best univentionstaff 2013-07-05 10:42:38 CEST
Only the following two packages did not already use the PatternSanitizer:

univention-printserver (7.0.0-1)
univention-virtual-machine-manager-daemon (3.0.0-1)

QA: only check if the search still works
Comment 3 Philipp Hahn univentionstaff 2013-07-23 20:24:29 CEST
(In reply to Florian Best from comment #2)
> univention-virtual-machine-manager-daemon (3.0.0-1)
> 
> QA: only check if the search still works

No, it does not: Selecting a "node" in the tree no longer returns any domains on that node for the grid; this is caused by "bad code™":

/var/log/univention/virtual-machine-manager-daemon.log shows, that uri (which is actually a "uriWildcard") contains the uri plus leading and trailing asterisk:
  2013-07-23 20:18:35,442 - uvmmd.command - DEBUG - DOMAIN_LIST *qemu://xen12.phahn.dev/system* *

But in src/univention/uvmm/node.py#domain_list the uri is only matched against the prefix-stripped *pd.name*, not the *uri*:
»···elif '*' in uri:
»···»···regex = re.compile( fnmatch.translate( uri ), re.IGNORECASE )
»···»···node_list = [n for n in nodes.values() if regex.match( n.pd.name ) is not None]
Comment 4 Florian Best univentionstaff 2013-07-24 11:45:51 CEST
(In reply to Philipp Hahn from comment #3)
> But in src/univention/uvmm/node.py#domain_list the uri is only matched
> against the prefix-stripped *pd.name*, not the *uri*:
> »···elif '*' in uri:
> »···»···regex = re.compile( fnmatch.translate( uri ), re.IGNORECASE )
> »···»···node_list = [n for n in nodes.values() if regex.match( n.pd.name )
> is not None]
Yes, we select only one or all server(s) to search domains at so that code have been removed. If a node-uri is given only domains will be found from that node otherwise all nodes will be used for the domain search.

univention-virtual-machine-manager-daemon (3.0.4-1)
Comment 5 Alexander Kläser univentionstaff 2013-07-30 18:38:50 CEST
Note: Please do not change reformat coding style unless necessary or in a place that is modified anyway. This just adds more changes to check for the QA. Lets rather agree upon a coding standard and throw all code through a code styling tool ;) .
Comment 6 Alexander Kläser univentionstaff 2013-08-01 15:02:06 CEST
In the UMC module top I get the following error for the search pattern "*k*s*o*f*t*irq*":

> Webfrontend-Fehler: Die angegebenen Argumente für die UMCP-Modul-Methode sind 
> nicht zulässig oder fehlen.
> Fehlernachricht des Servers:
> 1 Fehler aufgetreten
Comment 7 Alexander Kläser univentionstaff 2013-08-01 15:03:58 CEST
(In reply to Alexander Kläser from comment #6)
> In the UMC module top I get the following error for the search pattern
> "*k*s*o*f*t*irq*":
> 
> > Webfrontend-Fehler: Die angegebenen Argumente für die UMCP-Modul-Methode sind 
> > nicht zulässig oder fehlen.
> > Fehlernachricht des Servers:
> > 1 Fehler aufgetreten

Server response is:

{"status": 409, "message": "1 Fehler aufgetreten", "result": {"pattern": "Die H\u00f6chstzahl an Sternen (*) im Suchwort ist 5"}}

I guess result.pattern should be returned as message?
Comment 8 Dirk Wiesenthal univentionstaff 2013-08-01 15:14:14 CEST
There should be a generic solution for this.

The problem is that the form, the request and the naming of the parameters are somewhat independent.

The server only knows that the internal parameter is called "pattern". There is no way of returning "Schlüsselwort: Die Höchstzahl an Sternen (*) im Suchwort ist 5".

On the other hand the umcpCommand only gets the error message. It is not connected to the form by any means. So even the frontend cannot know what "pattern" really meant.
Comment 9 Alexander Kläser univentionstaff 2013-08-01 18:25:08 CEST
(In reply to Florian Best from comment #2)
> Only the following two packages did not already use the PatternSanitizer:
> 
> univention-printserver (7.0.0-1)
> univention-virtual-machine-manager-daemon (3.0.0-1)

Could you please have a look at the UCS@school modules and create a bug if necessary?

> QA: only check if the search still works

UMC modules Printer administration + UVMM look fine, the commands can be executed as expected, more than '*' are blocked, as expected.

Changelog → OK
Changes   → OK

Please adjust the aforementioned error message.
Comment 10 Florian Best univentionstaff 2013-08-07 12:51:33 CEST
(In reply to Alexander Kläser from comment #9)
> Could you please have a look at the UCS@school modules and create a bug if
> necessary?Bug #32177
Comment 11 Florian Best univentionstaff 2013-08-07 16:20:33 CEST
(In reply to Alexander Kläser from comment #9)
> Please adjust the aforementioned error message.Bug #32181
Comment 12 Alexander Kläser univentionstaff 2013-08-07 18:00:55 CEST
(In reply to Florian Best from comment #10)
> (In reply to Alexander Kläser from comment #9)
> > Could you please have a look at the UCS@school modules and create a bug if
> > necessary?
> → Bug #32177

OK

(In reply to Florian Best from comment #11)
> (In reply to Alexander Kläser from comment #9)
> > Please adjust the aforementioned error message.
> → Bug #32181

OK

→ VERIFIED
Comment 13 Stefan Gohmann univentionstaff 2013-11-19 06:42:32 CET
UCS 3.2 has been released:
 http://docs.univention.de/release-notes-3.2-en.html
 http://docs.univention.de/release-notes-3.2-de.html

If this error occurs again, please use "Clone This Bug".