On 10/24/19 3:29 PM, Robert Klein wrote:
> On Thu, 24 Oct 2019 14:06:47 +0200,
> Martijn van Duren wrote:
>>
>> On 10/24/19 1:50 PM, Robert Klein wrote:
>>> Hi,
>>>
>>>
>>>
>>> On Thu, 24 Oct 2019 05:26:49 +0200,
>>> Predrag Punosevac wrote:
>>>>
>>>> Kapetanakis Giannis wrote:
>>>>
>>>>> On 23/10/2019 19:14, Predrag Punosevac wrote:
>>>>>> Hi Misc,
>>>>>>
>>>>>> I just upgraded a LDAP server from 6.5 to 6.6 running authorization and
>>>>>> authentication services for a 100 some member university research group.
>>>>>> It appears TLS handshake is broken. This worked perfectly on 6.5 and
>>>>>> earlier.
>>>>>>
>>>
>>> [ rest deleted ]
>>>
>>>> I am out of fuel to look more this tonight but I am 99% sure something
>>>> have changed on 6.6 which broke the things. Maybe my configuration was
>>>> wrong all along and in 6.6 few screws got tighten up which bit me for my
>>>> rear end. I would appreciate any further commend or suggestions how to
>>>> debug this. I would also appreciate any reports of fully working ldapd
>>>> on 6.6 release
>>>>
>>>> Best,
>>>> Predrag
>>>>
>>>
>>> This is related to commit “Make sure that ber in ber_scanf_elements is
>>> not NULL before parsing format” (martijn@) and caused by the scan string
>>> used by ber_scanf_elements on line 310 in ldape.c
>>
>> Thanks for looking into this. I didn't found the time yet.
>>>
>>> Regarding the commit, see also emails with subject “ber.c: Don't
>>> continue on nonexistent ber” to tech@ on August, 13.
>>>
>>> When you set scan string for ber_scanf_elements in line 310 of ldape.c
>>> from "{se" to "{s" it works again. Patch below.
>>>
>>> When you look at the ldap_extended function on ldape.c, you see ext_val
>>> is assigned to req_op in line 314. The only use could happen in the
>>> extended_ops[i]fn(req) call in line 318. This currently can only be a
>>> call to ldap_starttls (beginning at line 285, same file) which doesn't
>>> use req_op either. So it the `e' shouldn't matter.
>>>
>>> At a guess, this also conforms to RFC4511, section 4.14.1.
>>
>> Glancing over the RFC seems that you are correct.
>>>
>>> If ldap_extended is extended to handle other operations than starttls,
>>> care must be taken for an optional additional octet string in the
>>> request (see definition of extended request in RFC4511, section 4.12).
>>
>> Diff below should handle this. Also, you forgot to remove the ext_val.
>
> Sorry. Been too happy to get it working.
>
> Is it necessary to assign req->op ? I didn't see it used and it gets
> freed in the call to request_free().
In its current form probably not, but on the other hand it keeps the
current behaviour/intent more consistent and might help expand if we
ever want to add additional extended operations.
If you feel strongly I'll remove it altogether, I'm not strongly
inclined either way.
>
>
> Robert
>
>>>
>>>
>>> Best regards
>>> Robert
>>>
>> martijn@
>>
>> Index: ldape.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/ldapd/ldape.c,v
>> retrieving revision 1.31
>> diff -u -p -r1.31 ldape.c
>> --- ldape.c 28 Jun 2019 13:32:48 -0000 1.31
>> +++ ldape.c 24 Oct 2019 12:05:19 -0000
>> @@ -298,7 +298,6 @@ ldap_extended(struct request *req)
>> {
>> int i, rc = LDAP_PROTOCOL_ERROR;
>> char *oid = NULL;
>> - struct ber_element *ext_val = NULL;
>> struct {
>> const char *oid;
>> int (*fn)(struct request *);
>> @@ -307,11 +306,11 @@ ldap_extended(struct request *req)
>> { NULL }
>> };
>>
>> - if (ber_scanf_elements(req->op, "{se", &oid, &ext_val) != 0)
>> + if (ber_scanf_elements(req->op, "{s", &oid) != 0)
>> goto done;
>>
>> log_debug("got extended operation %s", oid);
>> - req->op = ext_val;
>> + req->op = req->op->be_sub->be_next;
>>
>> for (i = 0; extended_ops[i].oid != NULL; i++) {
>> if (strcmp(oid, extended_ops[i].oid) == 0) {
>
