Extended request in kludge acl

[Nadezhda Ivanova]

Hi Matthias,
I thought a bit about this control, and look at samdb_set_password. I think
that if we allow this control, its use must never be transparent to the
caller, it should be impossible for anyone to use it inadvertently without
performing the password check first. The way things are now, this is
possible - the only indication is a comment line informing is its the
callers responsibility. I think that any function that sends this control
must also perform the password checking, so we either move password checking
to samdb_set_password, or use a new function, and remove this control from
samdb_set_password. Andrew also suggested that the GUID of the account whose
password is being changed is included as a value in the control, and the acl
module makes sure that the GUIDs match, as a second line of defence. If we
could do these two things I will be willing to add your patch to acl.c.

Regards,
Nadya

On Fri, Jul 9, 2010 at 11:56 AM, Nadezhda Ivanova <nivanova at samba.org>wrote:

> And again I disagree. It has the potential to be misused and I dont like
> it. I'll think some more on how we can reconcile the issue.
>
> Regards,
> Nadya
>
>
> On Thu, Jul 8, 2010 at 8:38 PM, Matthias Dieter Wallnöfer <mdw at samba.org>wrote:
>
>> Hi Nadya,
>>
>> I've tried to introduce the user <-> message equal DN check but didn't had
>> in mind that also other authorized users (with the change permission set -
>> as the default Administrator) can call "samr_ChangePasswordUser" (as it's
>> with the standard LDAP user-password change). Therefore tests broke and I
>> had to revert.
>> Then I thought about the behaviour that every one can change other
>> people's password. And no, this isn't a security leak since you have to know
>> the old password. And then it is just the same if you login with the other
>> user credentials (username and the old password you know) and then perform
>> the password change.
>>
>> I really think we need to apply the short (first) version.
>>
>> Greets,
>> Matthias
>>
>> Nadezhda Ivanova wrote:
>>
>>> Hi Andrew,
>>> We had some discussion with Matthias on IRC, and we came up with 3 ways
>>> to solve the problem.
>>> One way is to keep the control and apply the patch as it is. I really do
>>> not like it because of reasons listed in previous mails. Another is to use
>>> an extended operation, which would complicate things and is yet another way
>>> to bypass security checks.
>>> The third way, which I am most in favor of, is for the samr to actually
>>> start providing both passwords on a password change, so we can use the
>>> standard flow of things. It just seems wrong to me to use sambd_set_password
>>> instead of a function that will provide both, and then introduce internal
>>> hacks to handle the problem.
>>>
>>> What do you think?
>>>
>>> Regards,
>>> Nadya
>>>
>>> On Thu, Jul 8, 2010 at 10:45 AM, Matthias Dieter Wallnöfer <
>>> mdw at samba.org <mailto:mdw at samba.org>> wrote:
>>>
>>>    Could we discuss this on IRC with Andrew?
>>>
>>>    Matthias
>>>
>>>    Nadezhda Ivanova wrote:
>>>
>>>        Hi Matthias,
>>>        I thought some more about the patch and I have missed
>>>        something very important.
>>>        This patch makes it so given the control we actually execute a
>>>        reset operation with the permissions of a change operation.
>>>        However, every user account actually is given permission to
>>>        change every other accoun't password. There is an ACE that
>>>        comes from the defaultSd that gives this right to EVERYONE.
>>>        The only thing that we count on after that is that the user
>>>        knows the correct old password to verify identity. With the
>>>        replace its different, permission has to be granted. So what
>>>        you are proposing is to essentially allow everyone to reset a
>>>        user's password with samr, which worries me A LOT. Maybe there
>>>        is something I am missing on the samr side.
>>>
>>>        Sorry I did not think of this earlier...
>>>
>>>        Regards,
>>>        Nadya
>>>
>>>
>>>        On Thu, Jul 8, 2010 at 10:20 AM, Matthias Dieter Wallnöfer
>>>        <mdw at samba.org <mailto:mdw at samba.org> <mailto:mdw at samba.org
>>>
>>>        <mailto:mdw at samba.org>>> wrote:
>>>
>>>           Andrew,
>>>
>>>           Andrew Bartlett wrote:
>>>
>>>               On Thu, 2010-07-08 at 08:12 +0200, Matthias Dieter
>>>        Wallnöfer
>>>               wrote:
>>>                 The approach you suggest would also work, but would
>>>        not have
>>>               as strict a
>>>               control over the transaction, as you would not be in the
>>>               transaction
>>>               when the old pw was checked.   (I'm not sure this
>>>        matters in
>>>               practice,
>>>               given you could have the same race on multiple DCs anyway).
>>>
>>>           Well, we have only one search/read request and then one modify
>>>           one. I think it should be safe to split them up in two distinct
>>>           transactions since the latter one (mainly the code in
>>>           "password_hash" module) has to be performed in an atomic
>>>        manner.
>>>
>>>               Like the AS_SYSTEM control, this control is a little
>>>               dangerous, and
>>>               would have to be carefully restricted.  Please ensure
>>>        the kpasswd
>>>               password change code handles this too.
>>>
>>>           The control is strictly private. You cannot set it over the
>>>        LDAP
>>>           protocol. But anyway I'm fine to rechange the code when we do
>>>           support extended operations ACLs. The kpasswd patch does
>>>        naturally
>>>           also exist - it's here:
>>>
>>> http://repo.or.cz/w/Samba/mdw.git/commitdiff/11f24a9a72d4c47359662ee5ba63433ac11e4b2c
>>> .
>>>
>>>           The code was tested manually and by "make test" and it does
>>>        work.
>>>           I hope you are fine about pushing it.
>>>
>>>           Matthias
>>>
>>>
>>>
>>>
>>>
>>
>