On Thu, 24 Oct 2019 15:35:44 +0200,
Martijn van Duren wrote:
>
> 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.
No, I just wanted to know. Better keep it for the moment.
Robert
> >
> >
> > 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) {
> >
