Full_Name: Michael Str.der Version: master / RE24 OS: irrelevant URL: Submission from: (NULL) (213.240.182.101) back-sock should also send extended operations to external listener. Patch will follow.
The attached patch file is derived from OpenLDAP Software. All of the modifications to OpenLDAP Software represented in the following patch(es) were developed by Michael Ströder <michael@stroeder.com>. I have not assigned rights and/or interest in this work to any party. I, Michael Ströder, hereby place the following modifications to OpenLDAP Software (and only these modifications) into the public domain. Hence, these modifications may be freely used and/or redistributed for any purpose with or without attribution and/or other notice. This patch can also be found here: ftp://ftp.openldap.org/incoming/0001-ITS-8714-Send-out-EXTENDED-operation-message-from-back-sock.patch
michael@stroeder.com wrote: > + /* write out the request to the extended process */ > + fprintf( fp, "EXTENDED\n" ); > + fprintf( fp, "msgid: %ld\n", (long) op->o_msgid ); > + sock_print_conn( fp, op->o_conn, si ); > + sock_print_suffixes( fp, op->o_bd ); > + fprintf( fp, "oid: %s\n", op->ore_reqoid.bv_val ); > + if (op->ore_reqdata) { > + fprintf( fp, "valuelen: %lu\n", op->ore_reqdata->bv_len ); > + fprintf( fp, "value: %s\n", op->ore_reqdata->bv_val ); > + } > + fprintf( fp, "\n" ); This isn't safe. The reqdata is binary, not a nul-terminated C string. I suppose you could hex or base64-encode it instead. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Howard Chu wrote: > michael@stroeder.com wrote: >> + /* write out the request to the extended process */ >> + fprintf( fp, "EXTENDED\n" ); >> + fprintf( fp, "msgid: %ld\n", (long) op->o_msgid ); >> + sock_print_conn( fp, op->o_conn, si ); >> + sock_print_suffixes( fp, op->o_bd ); >> + fprintf( fp, "oid: %s\n", op->ore_reqoid.bv_val ); >> + if (op->ore_reqdata) { >> + fprintf( fp, "valuelen: %lu\n", op->ore_reqdata->bv_len ); >> + fprintf( fp, "value: %s\n", op->ore_reqdata->bv_val ); >> + } >> + fprintf( fp, "\n" ); > > This isn't safe. The reqdata is binary, not a nul-terminated C string. I suppose you > could hex or base64-encode it instead. Frankly I don't understand. I considered using base64 but I wanted to stick to what's already done in back-sock. See openldap/servers/slapd/back-sock/bind.c for the password value which is also an arbitrary OctetString: /* write out the request to the bind process */ [..] fprintf( fp, "credlen: %lu\n", op->oq_bind.rb_cred.bv_len ); fprintf( fp, "cred: %s\n", op->oq_bind.rb_cred.bv_val ); /* XXX */ fprintf( fp, "\n" ); The above should also work with null-bytes, shoudn't it? Ciao, Michael.
Michael Ströder wrote: > Howard Chu wrote: >> michael@stroeder.com wrote: >>> + /* write out the request to the extended process */ >>> + fprintf( fp, "EXTENDED\n" ); >>> + fprintf( fp, "msgid: %ld\n", (long) op->o_msgid ); >>> + sock_print_conn( fp, op->o_conn, si ); >>> + sock_print_suffixes( fp, op->o_bd ); >>> + fprintf( fp, "oid: %s\n", op->ore_reqoid.bv_val ); >>> + if (op->ore_reqdata) { >>> + fprintf( fp, "valuelen: %lu\n", op->ore_reqdata->bv_len ); >>> + fprintf( fp, "value: %s\n", op->ore_reqdata->bv_val ); >>> + } >>> + fprintf( fp, "\n" ); >> >> This isn't safe. The reqdata is binary, not a nul-terminated C string. I suppose you >> could hex or base64-encode it instead. > > Frankly I don't understand. > > I considered using base64 but I wanted to stick to what's already done in back-sock. > > See openldap/servers/slapd/back-sock/bind.c for the password value which is also an > arbitrary OctetString: > > /* write out the request to the bind process */ > [..] > fprintf( fp, "credlen: %lu\n", op->oq_bind.rb_cred.bv_len ); > fprintf( fp, "cred: %s\n", op->oq_bind.rb_cred.bv_val ); /* XXX */ > fprintf( fp, "\n" ); > > The above should also work with null-bytes, shoudn't it? No, it's a bug. Probably a benign bug because it's difficult for users to create passwords with embedded NULs. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Find attached a new patch which base64-encoded the extended operation request value before sending it to the socket. You can also download patch file here: https://www.stroeder.com/temp/0001-ITS-8714-Send-out-EXTENDED-operation-message-from-back-sock_rev2.patch Ciao, Michael.
michael@stroeder.com wrote: > This is a multi-part message in MIME format. > --------------E329EF3D834E0A798BAC2EBC > Content-Type: text/plain; charset=UTF-8; format=flowed > Content-Transfer-Encoding: 7bit > > Find attached a new patch which base64-encoded the extended > operation request value before sending it to the socket. > > You can also download patch file here: Your patch is missing extended.c -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Howard Chu wrote: > Your patch is missing extended.c Aargh! Corrected herein: https://www.stroeder.com/temp/0001-ITS-8714-Send-out-EXTENDED-operation-message-from-back-sock_rev3.patch Ciao, Michael.
Michael Ströder wrote: > Howard Chu wrote: >> Your patch is missing extended.c > > Aargh! Corrected herein: > > https://www.stroeder.com/temp/0001-ITS-8714-Send-out-EXTENDED-operation-message-from-back-sock_rev3.patch There's a weird indent at extended.c:50 or so, the if() statement. Would be better to use op->o_tmpalloc instead of ber_memalloc since you're immediately freeing the buffer again anyway. I can fix those here if you don't care. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
hyc@symas.com wrote: > There's a weird indent at extended.c:50 or so, the if() > statement. > > Would be better to use op->o_tmpalloc instead of ber_memalloc > since you'r= e=20 immediately freeing the buffer again anyway. > > I can fix those here if you don't care. Yes, I'd highly appreciate if you simply adjust it to your coding style. One additional point: Currently the external program is not able to produce a custom extended operation response. Mainly it should always return CONTINUE or an error response. I wanted to make this limitation clear in the man-page but was unsure about the appropriate section. Ciao, Michael.
Michael Ströder wrote: > hyc@symas.com wrote: >> There's a weird indent at extended.c:50 or so, the if() >> statement. >> >> Would be better to use op->o_tmpalloc instead of ber_memalloc >> since you'r= e=20 immediately freeing the buffer again anyway. >> >> I can fix those here if you don't care. > > Yes, I'd highly appreciate if you simply adjust it to your coding style. > > One additional point: > Currently the external program is not able to produce a custom extended > operation response. Mainly it should always return CONTINUE or an error > response. I wanted to make this limitation clear in the man-page but was > unsure about the appropriate section. You could add a LIMITATIONS section, as slapd-monitor.5 and slapd-shell.5 does. The manpage now needs an update to note that the exop value is base64 encoded. Also, since it is encoded, I don't believe it's necessary to explicitly send the valuelen. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Howard Chu wrote: > You could add a LIMITATIONS section, as slapd-monitor.5 and > slapd-shell.5 does. The manpage now needs an update to note that > the exop value is base64 encoded. Also, since it is encoded, I > don't believe it's necessary to explicitly send the valuelen. Ah, yes. Forgot to update the message format in the man-page. If you don't mind I just produce another follow-up patch for the man-page. Ok? Ciao, Michael.
michael@stroeder.com wrote: > If you don't mind I just produce another follow-up patch for the > man-page. Find this man-page patch here: https://www.stroeder.com/temp/0001-ITS-8714-man-page-corrections-regarding-EXTENDED-ope.patch Ciao, Michael.
changed notes
changed notes changed state Open to Test moved from Incoming to Software Enhancements
changed notes changed state Test to Release
in master in RE24 (2.4.47)
changed notes changed state Release to Closed