search

gosa-project / gosa-core

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

External hook command processing is broken #60

Open gber opened 1 year ago

gber commented 1 year ago

The processing of the plugin hook command string in the plugin::callHook() is broken on a fundamental level. Before executing the command gosa performs %-substitution and then naively tries to split off any leading shell variable definitions in order to put them into the environment using the corresponding argument of proc_open(). I assume the latter is an attempt to hide possibly sensitive content such as passwords from being read through proc, ps and the like by other users.

However then approach of emulating shell field splitting via regex does not work and cannot be fixed as expansion, unqoting and field splitting performed by the shell is highly complex. The current use of preg_match("/^(.* )?(\/.*)$/", $command, $matches) already fails on trivial commands such as a sudo wrapper "/usr/bin/sudo /usr/share/debian-edu-config/tools/gosa-modify-host %cn". And the fact that substitution is performed before splitting introduces even more potential for errors. The fact that possible shell injection via field substitution is documented in gosa.conf.5 does not make it any better.

CC @sunweaver @dzatoah

dzatoah commented 1 year ago

Hey @gber,

I also found that the plugin::callHook() is somehow broken and I discussed this very briefly with @sunweaver. Then my attention was spent on another project... You apparently spent some time on this. Thank you.

gber commented 1 year ago

After thinking about it a bit more, this could be addressed by gosa placing all attributes in the environment itself rather than using command line arguments with substitution, e.g. setting GOSA_CN instead of substituting %cn in an argument. This avoids messing with the command which could be passed to proc_open() as is, avoiding any shell injection issues. A common GOSA_* prefix would allow passing the enviroment variables on to a sudo command. The problem is that it breaks backwards compatibility with existing hooks.

sunweaver commented 1 year ago

@gber the interesting bit is: this hasn't been problematic in Debian 11 with PHP 7.4... It seems that code hasn't been changed since then. So, the question arises whether PHP 8.x behaves differently regarding regexp parsing?

gber commented 1 year ago

@gber the interesting bit is: this hasn't been problematic in Debian 11 with PHP 7.4... It seems that code hasn't been changed since then. So, the question arises whether PHP 8.x behaves differently regarding regexp parsing?

I just tried a minimal example on both PHP 8 and 7.4 with the same result, as I expected /^(.* )? matches greedily to the last space. After much back and forth I just booted into bullseye and found that the code in question simply isn't there. The 2.8 git package is based on the development branch which has a truncated history but seems to have been copied from the master branch back in 2021. The master branch does have the broken code since 2012(!) (see https://github.com/gosa-project/gosa-core/commit/87e54bc6e8e628dd60793e955c4eed9dfa78cbd4) and is tagged with version 2.7.5 (the earliest tagged release in this repo). Checking the Debian patches it also hadn't been patched out in bullseye. I don't know what branch the release tarballs used in bullseye are based on.