gosa-project / gosa-core

GOsa core
GNU General Public License v2.0
16 stars 14 forks source link

Use base64 encoding on passwords sent to shell hooks to avoid RCE #16

Closed tmack0 closed 6 years ago

tmack0 commented 6 years ago

escapeshellarg() is not that great at what it is intended to do, especially when not used with escapeshellcmd(). Even if it is, there are known ways to exploit it. Our sec team identified that the way escapeshellarg() is used in Gosa for hook calls, allows a user to set carefully crafted values to perform RCE via injection attacks. It also caused problems with certain special characters being used by password managers. A safer way to pass user input data to a shell command is to base64 encode the value, and have the hook command itself use "base64 -d" on the value to have it properly passed as a single argument that will not get interpolated by the shell. Since passwords are the most likely parameter passed via hooks, are set by the user directly, and can contain any valid character including very problematic ones that many shells will act on, this is the minimal fix that should be done. Any/all values settable by untrusted users, that get passed to shell unchecked, should use this method. Note that this will require password_hook configs to call a base64 decoder as part of their command, either in a shell script or directly in the hook config line. ex:

User Passoword premodify hook: /usr/local/bin/gam update user %uid password $(echo %new_password|/usr/bin/base64 -d)

This would call gam (google account manager) to sync a password change from gosa into google, using the base64 program available in most linux distros.

sunweaver commented 6 years ago

@master-caster: plesase review this PR, thanks. Mike

master-caster commented 6 years ago

@sunweaver am I missing something?

Is there a reason we shouldn't merge it? I think we can add documentation to FAQ and/or gosa.conf.5

sunweaver commented 6 years ago

The only missing bit is a documentation part. (And the NEWS file in debian/NEWS should be prepped accordingly, but this is not here).

sunweaver commented 6 years ago

@tmack0: Can you push a follow up commit that adds some info to gosa.conf.1?

tmack0 commented 6 years ago

Added some text to gosa.conf.5 to mention dangers of hook calls and note the use of base64 on password values.

sunweaver commented 6 years ago

On Mi 15 Aug 2018 22:08:00 CEST, Theral Mackey wrote:

Added some text to gosa.conf.5 to mention dangers of hook calls and
note the use of base64 on password values.

Thanks. Please drop us a one line comment that indicates we can go
into the next review iteration. Before you haven't sent such a note,
we wait...

Mike --

DAS-NETZWERKTEAM mike gabriel, herweg 7, 24357 fleckeby mobile: +49 (1520) 1976 148 landline: +49 (4354) 8390 139

GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31 mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de

tmack0 commented 6 years ago

You can proceed to the next review interation