Patches for GOsa

[Theral Mackey]

Following up again, with PRs:

SHA-256/SHA-512 PR

https://github.com/gosa-project/gosa-core/pull/15

and base64 passwords for hook calls:

https://github.com/gosa-project/gosa-core/pull/16

This one it appears that the debian source (and the upstream I pulled
a long time ago, which I based most of my changes on) uses
escapeshellarg() inside the change_password() function, which gets
passed on to fill_replacements(), which calls escapeshellarg() again,
where the current gosa-core does not call escapeshellarg() until
inside the fill_replacements() call. I updated the change to go ahead
and perform base64_encode() on the value since escapeshellarg() should
be a noop on any value it outputs. The diffs I have in my patch repo I
mentioned earlier are for changing escapeshellarg() to
base64_encode(). The PR has a lengthy note on why, and how to use
hooks after this change. Note that this will break current hooks using
%password or %old_password until they are updated to use a base64
decode method.

-Theral


On Thu, Aug 9, 2018 at 4:33 PM, Theral Mackey <tmackey at evernote.com> wrote:
> Hi Mike,
>
> On Mon, Jul 23, 2018 at 7:08 AM, Mike Gabriel <sunweaver at debian.org> wrote:
>> HI Theral,
>>
>> sorry for the late reply.
>
> Ditto (vacation)...
>
>> On  Di 10 Jul 2018 02:03:32 CEST, Theral Mackey wrote:
>>
>>> Hi, I have a few patches for Gosa I feel could/should be incorporated
>>> to the debian pkg (and upstream, if thats still alive?). If possible,
>>> could I get PR access to the project on salsa to submit these?
>>> (username currently tmack0-guest)
>>
>>
>> Please file PRs on github.com against the approprivate upstream repo. We
>> will discuss patch inclusion there and I will then cherry-pick stuff into
>> the Debian package.
>>
>
> I have a github repo with these up right now (had not found the
> upstream until now (gosa-project)):
> https://github.com/tmack0/gosa_debian_patches
>
> Now that I have the upstream project I will create the PRs and reply
> with the PR#/link, etc.
>
>>> We here at Evernote have been using these (along with a few custom
>>> mods for stuff like TOTP integration) for a number of years now:
>>
>>
>> Nice. (Is TOTP integration not something for upstream?).
>>
>
> Possibly, though the way we did it was with a few ugly UI hacks on the
> password change and user management pages that could be redone much
> much more nicely. I will see if I can get things stripped out into
> something not specific to Evernote and get up a PR. I can definitely
> supply the back-end functions for it if someone with better UI skills
> can rework those components (includes admin options for
> enabling/disabling/resetting 2fa, self-service style provisioning with
> QR codes, etc. Saves the 2fa secret encrypted in an LDAP attr). This
> will take a bit more time..
>
>>> 1. base64() encode passwords sent to shell via hook calls
>>
>>
>> Ok.
>>
>>> 2. add support for CRYPT sha256 and sha512 passwords
>>
>>
>> Ok.
>>
>>> The second one above I can't take full credit for, I think I pieced it
>>
>>
>> Are you eligible to file a PR then? Can you get in touch with the original
>> author? Or do you know the license of the patch? If so, we can simply add
>> the patch (if it is GPL'ish) and give credits to the original author. Other
>> ideas?
>>
>>> together from forum posts if not out-right copy/pasta, but allows
>>> using more secure hashing functions available in openldap.
>>
>>
>> That would be nice to have. If the patch has been modified by you
>> sufficiently, it would be ok to take full credits for it.
>>
>
> I looked around again, and found it in this code on github (looks like
> a clone of Gosa?):
> https://github.com/makinacorpus/fusiondirectory/blob/master/include/password-methods/class_password-methods-crypt.inc
>
> Though the - in the hash name is evidence this is not where I got it
> from originally (my patches have "sha256", not "sha-256"). I also
> remember only sha256 being given, and I added 512 myself using that as
> a hint. So, credit can be placed where ever makes sense. Its a fairly
> basic bit of code.
>
>
>>> The first we implemented because the escapeshellarg code used in Gosa
>>> seemed to be applied inconsistently, causing breakage if certain
>>> characters were used, and lead to our sec team being able to use it
>>> for RCE. Passing through base64() encoded values bypasses this much
>>> the same way the other patches for NTLM/perl stuff did (0006, 0007 and
>>> 1004). Its likely all variables in hooks could be exploited this way
>>> tbh, passwords are just the quickest way to notice due to the charsets
>>> involved.
>>
>>
>> Understood. Please explain the situation in detail on the upstream tracker
>> (github.com/gosa-project/gosa-core).
>>
>>> Related to which, another patch I am putting together now from our
>>> code, moves the NTLM/LM hashing to native php, removing that
>>> shell/perl call all together. (we eventually stripped out LM
>>> completely as we have no use for it, and NTLM is all of 3 lines).
>>
>>
>> That would be nice-to-have, too. Please keep us posted.
>>
>
> Will do.
>
>>> Thanks for your time!
>>> -Theral Mackey
>>
>>
>> Once you have filed the PRs upstream, please ping me and I nudge the
>> upstream maintainer from GONICUS to review it within the next weeks.
>>
>> Thanks+Greets,
>> Mike
>>
>>
>
> Thanks!
> -Theral
>
> --