Re: smtpd-filters: swap link-auth fields
On Wed, 14 Jun 2023 16:34:39 +0200, Omar Polo wrote: > the `link-auth' event hash the user first and the result of the > operation after; this breaks when a username has a '|' character in > it. Since this is triggered by the `auth login' command, anyone could > send a user with a '|' and, depending on the filter used, make smtpd > exit. (if the filter dies, smtpd does too) > > This was reported on the OpenSMTPD-portable github repository with > Gilles' opensmtpd-filter-rspamd: > > https://github.com/OpenSMTPD/OpenSMTPD/issues/1213 > > Diff below is straightforward and includes the documentation changes. > I believe link-auth was forgotten in revision 1.61 of lka_filter.c > when the mail-from/rcpt-to events got their fields swapped. OK millert@ - todd
Re: smtpd-filters: swap link-auth fields
Just released a new filter-rspamd with your diff, thanks > On 14 Jun 2023, at 19:23, Omar Polo wrote: > > Hello, > > the `link-auth' event hash the user first and the result of the > operation after; this breaks when a username has a '|' character in > it. Since this is triggered by the `auth login' command, anyone could > send a user with a '|' and, depending on the filter used, make smtpd > exit. (if the filter dies, smtpd does too) > > This was reported on the OpenSMTPD-portable github repository with > Gilles' opensmtpd-filter-rspamd: > >https://github.com/OpenSMTPD/OpenSMTPD/issues/1213 > > Diff below is straightforward and includes the documentation changes. > I believe link-auth was forgotten in revision 1.61 of lka_filter.c > when the mail-from/rcpt-to events got their fields swapped. > > For opensmtpd-filter-rspamd I have a corresponding diff that I'll send > to Gilles as it is off-topic for tech@, but here it is too if you want > to play with it: > >https://paste.omarpolo.com/9jtli2w > > To reproduce: (there may be quicker ways, this is just the first i > found) > ># pkg_add rspamd opensmtpd-filter-rspamd ># rcctl enable rspamd ># rcctl start rspamd > > add the rspamd filter to /etc/mail/smtpd.conf > >filter "rspamd" proc-exec "filter-rspamd" >listen on lo0 smtps pki localhost auth filter "rspamd" > > and try to do a login: > >$ nc -c -Tnoverify localhost 465 >helo localhost >auth login >b3xw >MTMyNA== > > > Thanks, > > Omar Polo > > > diff /usr/src > commit - 66c6b79616659a94b04092c9f103e3aa29809704 > path + /usr/src > blob - 0c63657be21352fb1f060505250f7a9ef4fc8d8c > file + usr.sbin/smtpd/lka_filter.c > --- usr.sbin/smtpd/lka_filter.c > +++ usr.sbin/smtpd/lka_filter.c > @@ -24,7 +24,7 @@ > #include "smtpd.h" > #include "log.h" > > -#definePROTOCOL_VERSION"0.6" > +#definePROTOCOL_VERSION"0.7" > > struct filter; > struct filter_session; > @@ -1461,7 +1461,7 @@ lka_report_smtp_link_auth(const char *direction, struc >fs->username = xstrdup(username); >} >report_smtp_broadcast(reqid, direction, tv, "link-auth", "%s|%s\n", > -username, result); > +result, username); > } > > void > blob - 313404c111c77b099b3855f43252c26877874b17 > file + usr.sbin/smtpd/smtpd-filters.7 > --- usr.sbin/smtpd/smtpd-filters.7 > +++ usr.sbin/smtpd/smtpd-filters.7 > @@ -271,12 +271,9 @@ This event is generated upon disconnection of the clie > the cipher suite used by the session and the cipher strength in bits. > .It Ic link-disconnect > This event is generated upon disconnection of the client. > -.It Ic link-auth : Ar username result > +.It Ic link-auth : Ar result username > This event is generated upon an authentication attempt by the client. > .Pp > -.Ar username > -contains the username used for the authentication attempt. > -.Pp > .Ar result > contains the string > .Dq pass , > @@ -284,6 +281,9 @@ depending on the result of the authentication attempt. > or > .Dq error > depending on the result of the authentication attempt. > +.Pp > +.Ar username > +contains the username used for the authentication attempt. > .It Ic tx-reset : Op message-id > This event is generated when a transaction is reset. > .Pp
Re: csh(1), ksh(1), time(1): print durations with millisecond precision
On Wed, Jun 14, 2023 at 10:34:20AM -0400, Josiah Frentsos wrote:
> On Tue, Jun 13, 2023 at 10:59:53PM -0500, Scott Cheloha wrote:
> > Index: usr.bin/time/time.c
> > ===
> > RCS file: /cvs/src/usr.bin/time/time.c,v
> > retrieving revision 1.25
> > diff -u -p -r1.25 time.c
> > --- usr.bin/time/time.c 21 Aug 2017 13:38:02 - 1.25
> > +++ usr.bin/time/time.c 14 Jun 2023 03:23:29 -
> > @@ -100,19 +100,19 @@ main(int argc, char *argv[])
> > timespecsub(&after, &before, &during);
> >
> > if (portableflag) {
> > - fprintf(stderr, "real %9lld.%02ld\n",
> > - (long long)during.tv_sec, during.tv_nsec/1000);
> > - fprintf(stderr, "user %9lld.%02ld\n",
> > - (long long)ru.ru_utime.tv_sec,
> > ru.ru_utime.tv_usec/1);
> > - fprintf(stderr, "sys %9lld.%02ld\n",
> > - (long long)ru.ru_stime.tv_sec,
> > ru.ru_stime.tv_usec/1);
> > + fprintf(stderr, "real %9lld.%03ld\n",
> > + (long long)during.tv_sec, during.tv_nsec / 100);
> > + fprintf(stderr, "user %9lld.%03ld\n",
> > + (long long)ru.ru_utime.tv_sec, ru.ru_utime.tv_usec / 1000);
> > + fprintf(stderr, "sys %9lld.%03ld\n",
> > + (long long)ru.ru_stime.tv_sec, ru.ru_stime.tv_usec / 1000);
> > } else {
> > - fprintf(stderr, "%9lld.%02ld real ",
> > - (long long)during.tv_sec, during.tv_nsec/1000);
> > - fprintf(stderr, "%9lld.%02ld user ",
> > - (long long)ru.ru_utime.tv_sec,
> > ru.ru_utime.tv_usec/1);
> > - fprintf(stderr, "%9lld.%02ld sys\n",
> > - (long long)ru.ru_stime.tv_sec,
> > ru.ru_stime.tv_usec/1);
> > + fprintf(stderr, "%9lld.%03ld real ",
> > + (long long)during.tv_sec, during.tv_nsec / 100);
> > + fprintf(stderr, "%9lld.%0ld user ",
>
>
> Should this be "%03ld"?
Whoops, yep, good catch.
Index: bin/csh/time.c
===
RCS file: /cvs/src/bin/csh/time.c,v
retrieving revision 1.18
diff -u -p -r1.18 time.c
--- bin/csh/time.c 8 Mar 2023 04:43:04 - 1.18
+++ bin/csh/time.c 14 Jun 2023 15:30:01 -
@@ -40,6 +40,7 @@
* C Shell - routines handling process timing and niceing
*/
static voidpdeltat(struct timeval *, struct timeval *);
+static voidpdelta_hms(const struct timespec *, const struct timespec *);
void
settimes(void)
@@ -145,7 +146,7 @@ prusage(struct rusage *r0, struct rusage
break;
case 'E': /* elapsed (wall-clock) time */
- pcsecs((long) ms);
+ pdelta_hms(e, b);
break;
case 'P': /* percent time spent running */
@@ -227,8 +228,7 @@ pdeltat(struct timeval *t1, struct timev
struct timeval td;
timersub(t1, t0, &td);
-(void) fprintf(cshout, "%lld.%01ld", (long long)td.tv_sec,
- td.tv_usec / 10);
+fprintf(cshout, "%lld.%03ld", (long long)td.tv_sec, td.tv_usec / 1000);
}
#define P2DIG(i) (void) fprintf(cshout, "%d%d", (i) / 10, (i) % 10)
@@ -254,23 +254,18 @@ minsec:
}
void
-pcsecs(long l) /* PWP: print mm:ss.dd, l is in sec*100 */
+pdelta_hms(const struct timespec *t1, const struct timespec *t0)
{
-int i;
+struct timespec elapsed;
+long long hours, minutes, seconds;
-i = l / 36;
-if (i) {
- (void) fprintf(cshout, "%d:", i);
- i = (l % 36) / 100;
- P2DIG(i / 60);
- goto minsec;
-}
-i = l / 100;
-(void) fprintf(cshout, "%d", i / 60);
-minsec:
-i %= 60;
-(void) fputc(':', cshout);
-P2DIG(i);
-(void) fputc('.', cshout);
-P2DIG((int) (l % 100));
+timespecsub(t1, t0, &elapsed);
+hours = elapsed.tv_sec / 3600;
+minutes = (elapsed.tv_sec % 3600) / 60;
+seconds = elapsed.tv_sec % 60;
+if (hours != 0)
+ fprintf(cshout, "%lld:%02lld:", hours, minutes);
+else
+ fprintf(cshout, "%lld:", minutes);
+fprintf(cshout, "%02lld.%03ld", seconds, elapsed.tv_nsec / 100);
}
Index: bin/ksh/c_sh.c
===
RCS file: /cvs/src/bin/ksh/c_sh.c,v
retrieving revision 1.64
diff -u -p -r1.64 c_sh.c
--- bin/ksh/c_sh.c 22 May 2020 07:50:07 - 1.64
+++ bin/ksh/c_sh.c 14 Jun 2023 15:30:01 -
@@ -681,13 +681,13 @@ p_tv(struct shf *shf, int posix, struct
char *suffix)
{
if (posix)
- shf_fprintf(shf, "%s%*lld.%02ld%s", prefix ? prefix : "",
- width, (long long)tv->tv_sec, tv->tv_usec / 1, suffix);
+ shf_fprintf(shf, "%s%*lld.%03ld%s", prefix ? prefix : "",
+ width, (long long)tv->tv_sec, tv->tv_usec / 1000, suffix)
Re: smtpd-filters: swap link-auth fields
On 2023/06/14 16:34:39 +0200, Omar Polo wrote: > For opensmtpd-filter-rspamd I have a corresponding diff that I'll send > to Gilles as it is off-topic for tech@, but here it is too if you want > to play with it: > > https://paste.omarpolo.com/9jtli2w apologize, this one has a stupid typo. I've opend a PR on github with an updated diff. https://github.com/poolpOrg/filter-rspamd/pull/46 sorry for the noise.
lpd(8): document correct control character for DVI
r is for FORTRAN output, not for DVI. Index: lpd.8 === RCS file: /cvs/src/usr.sbin/lpr/lpd/lpd.8,v retrieving revision 1.32 diff -u -p -r1.32 lpd.8 --- lpd.8 10 Oct 2022 09:13:43 - 1.32 +++ lpd.8 6 Jun 2023 12:30:54 - @@ -242,7 +242,7 @@ The file contains troff output (cat phot .It n Ditroff File. The file contains device independent troff output. -.It r +.It d DVI File. The file contains .Tn Tex l
smtpd-filters: swap link-auth fields
Hello, the `link-auth' event hash the user first and the result of the operation after; this breaks when a username has a '|' character in it. Since this is triggered by the `auth login' command, anyone could send a user with a '|' and, depending on the filter used, make smtpd exit. (if the filter dies, smtpd does too) This was reported on the OpenSMTPD-portable github repository with Gilles' opensmtpd-filter-rspamd: https://github.com/OpenSMTPD/OpenSMTPD/issues/1213 Diff below is straightforward and includes the documentation changes. I believe link-auth was forgotten in revision 1.61 of lka_filter.c when the mail-from/rcpt-to events got their fields swapped. For opensmtpd-filter-rspamd I have a corresponding diff that I'll send to Gilles as it is off-topic for tech@, but here it is too if you want to play with it: https://paste.omarpolo.com/9jtli2w To reproduce: (there may be quicker ways, this is just the first i found) # pkg_add rspamd opensmtpd-filter-rspamd # rcctl enable rspamd # rcctl start rspamd add the rspamd filter to /etc/mail/smtpd.conf filter "rspamd" proc-exec "filter-rspamd" listen on lo0 smtps pki localhost auth filter "rspamd" and try to do a login: $ nc -c -Tnoverify localhost 465 helo localhost auth login b3xw MTMyNA== Thanks, Omar Polo diff /usr/src commit - 66c6b79616659a94b04092c9f103e3aa29809704 path + /usr/src blob - 0c63657be21352fb1f060505250f7a9ef4fc8d8c file + usr.sbin/smtpd/lka_filter.c --- usr.sbin/smtpd/lka_filter.c +++ usr.sbin/smtpd/lka_filter.c @@ -24,7 +24,7 @@ #include "smtpd.h" #include "log.h" -#definePROTOCOL_VERSION"0.6" +#definePROTOCOL_VERSION"0.7" struct filter; struct filter_session; @@ -1461,7 +1461,7 @@ lka_report_smtp_link_auth(const char *direction, struc fs->username = xstrdup(username); } report_smtp_broadcast(reqid, direction, tv, "link-auth", "%s|%s\n", - username, result); + result, username); } void blob - 313404c111c77b099b3855f43252c26877874b17 file + usr.sbin/smtpd/smtpd-filters.7 --- usr.sbin/smtpd/smtpd-filters.7 +++ usr.sbin/smtpd/smtpd-filters.7 @@ -271,12 +271,9 @@ This event is generated upon disconnection of the clie the cipher suite used by the session and the cipher strength in bits. .It Ic link-disconnect This event is generated upon disconnection of the client. -.It Ic link-auth : Ar username result +.It Ic link-auth : Ar result username This event is generated upon an authentication attempt by the client. .Pp -.Ar username -contains the username used for the authentication attempt. -.Pp .Ar result contains the string .Dq pass , @@ -284,6 +281,9 @@ depending on the result of the authentication attempt. or .Dq error depending on the result of the authentication attempt. +.Pp +.Ar username +contains the username used for the authentication attempt. .It Ic tx-reset : Op message-id This event is generated when a transaction is reset. .Pp
Re: csh(1), ksh(1), time(1): print durations with millisecond precision
On Tue, Jun 13, 2023 at 10:59:53PM -0500, Scott Cheloha wrote:
> Index: usr.bin/time/time.c
> ===
> RCS file: /cvs/src/usr.bin/time/time.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 time.c
> --- usr.bin/time/time.c 21 Aug 2017 13:38:02 - 1.25
> +++ usr.bin/time/time.c 14 Jun 2023 03:23:29 -
> @@ -100,19 +100,19 @@ main(int argc, char *argv[])
> timespecsub(&after, &before, &during);
>
> if (portableflag) {
> - fprintf(stderr, "real %9lld.%02ld\n",
> - (long long)during.tv_sec, during.tv_nsec/1000);
> - fprintf(stderr, "user %9lld.%02ld\n",
> - (long long)ru.ru_utime.tv_sec,
> ru.ru_utime.tv_usec/1);
> - fprintf(stderr, "sys %9lld.%02ld\n",
> - (long long)ru.ru_stime.tv_sec,
> ru.ru_stime.tv_usec/1);
> + fprintf(stderr, "real %9lld.%03ld\n",
> + (long long)during.tv_sec, during.tv_nsec / 100);
> + fprintf(stderr, "user %9lld.%03ld\n",
> + (long long)ru.ru_utime.tv_sec, ru.ru_utime.tv_usec / 1000);
> + fprintf(stderr, "sys %9lld.%03ld\n",
> + (long long)ru.ru_stime.tv_sec, ru.ru_stime.tv_usec / 1000);
> } else {
> - fprintf(stderr, "%9lld.%02ld real ",
> - (long long)during.tv_sec, during.tv_nsec/1000);
> - fprintf(stderr, "%9lld.%02ld user ",
> - (long long)ru.ru_utime.tv_sec,
> ru.ru_utime.tv_usec/1);
> - fprintf(stderr, "%9lld.%02ld sys\n",
> - (long long)ru.ru_stime.tv_sec,
> ru.ru_stime.tv_usec/1);
> + fprintf(stderr, "%9lld.%03ld real ",
> + (long long)during.tv_sec, during.tv_nsec / 100);
> + fprintf(stderr, "%9lld.%0ld user ",
Should this be "%03ld"?
> + (long long)ru.ru_utime.tv_sec, ru.ru_utime.tv_usec / 1000);
> + fprintf(stderr, "%9lld.%03ld sys\n",
> + (long long)ru.ru_stime.tv_sec, ru.ru_stime.tv_usec / 1000);
> }
>
> if (lflag) {
Re: iked: replace last print_host users to print_addr
On Wed, Jun 14, 2023 at 02:30:44PM +0200, Claudio Jeker wrote:
> On Wed, Jun 14, 2023 at 12:37:35PM +0200, Theo Buehler wrote:
> > On Wed, Jun 14, 2023 at 11:38:15AM +0200, Claudio Jeker wrote:
> > > On Wed, Jun 14, 2023 at 11:10:52AM +0200, Theo Buehler wrote:
> > > > On Wed, Jun 14, 2023 at 10:44:23AM +0200, Claudio Jeker wrote:
> > > > > There is no real need to have print_host() with the extra arguments.
> > > > > So convert the last remaining print_host() calls to use print_addr().
> > > > > I'm not entierly sure how to really test all these code paths but the
> > > > > changes are failry simple.
> > > >
> > > > Thanks for beating me to it...
> > > >
> > > > In ikev2_pld_cp() you need to garbage collect buf and replace its
> > > > remaining uses by the appropriate print_addr().
> > >
> > > Oh boy, I missed the fact that this double prints the same info.
> > > I moved the first log_debug as default: of the switch statement.
> >
> > Yeah, that makes sense.
> >
> > > > In ikev2_print_id() it would seem more appropriate to use strlcat()
> > > > instead of strlcpy() in the switch and and reserve the special dance
> > > > of adjusting idstr and idstrlen for the /* XXX test */ path, but that
> > > > should be done in a separate diff.
> > >
> > > Oh boy, this is even worse than the above case. WTF!
> >
> > Indeed.
> >
> > > So many things are utterly bad in this function and the XXX test path is
> > > very broken. I tried to fix this up a bit.
> >
> > Should s4 and s6 not be struct sockaddr_in{,6} on the stack and get a
> > treatment similar to start4 and start6 in ikev2_pld_ts()? This buf thing
> > seems completely nuts. The weird length check in the FQDN case could be
> > done with a simple comparison to BUFSIZ and then buf could become a
> > local 3-byte buffer in the for loop of the XXX test case.
> >
> > However, I completely understand if you don't want to touch more of this
> > mess than you already did...
>
> It never pays out to be lazy. Fixed in diff below.
> Guess that saves around 990bytes of stack :)
>
> > Just one thing, otherwise your diff is ok:
> >
> > > @@ -6980,9 +6975,11 @@ ikev2_print_id(struct iked_id *id, char
> > > break;
> > > default:
> > > /* XXX test */
> > > - for (i = 0; i < ((ssize_t)idstrlen - 1) && i < len; i++)
> > > - snprintf(idstr + i, idstrlen - i,
> > > - "%02x", ptr[i]);
> > > + for (i = 0; i < len; i++) {
> > > + snprintf(buf, sizeof(buf), "%02x", ptr[i]);
> > > + if (strlcat(idstr, buf, idstrlen) >= idstrlen)
> >
> > Should this not return -1 like the other cases?
> >
>
> From my understanding the idea is that we try to dump as much of the
> unknown playload as possible. The code before did that with this
> complicated logic:
> i < ((ssize_t)idstrlen - 1) && i < len
> I decided to just fill the buffer until strlcat() complains and exit the
> loop then.
I see. I'm fine with that. Given that that was broken the same way
since import...
I can't for the life of me figure out what that (ssize_t)sizeof(buf)
check could have been about. It's been there since import, too. It seems
alright to drop it.
ok tb
Re: iked: replace last print_host users to print_addr
On Wed, Jun 14, 2023 at 12:37:35PM +0200, Theo Buehler wrote:
> On Wed, Jun 14, 2023 at 11:38:15AM +0200, Claudio Jeker wrote:
> > On Wed, Jun 14, 2023 at 11:10:52AM +0200, Theo Buehler wrote:
> > > On Wed, Jun 14, 2023 at 10:44:23AM +0200, Claudio Jeker wrote:
> > > > There is no real need to have print_host() with the extra arguments.
> > > > So convert the last remaining print_host() calls to use print_addr().
> > > > I'm not entierly sure how to really test all these code paths but the
> > > > changes are failry simple.
> > >
> > > Thanks for beating me to it...
> > >
> > > In ikev2_pld_cp() you need to garbage collect buf and replace its
> > > remaining uses by the appropriate print_addr().
> >
> > Oh boy, I missed the fact that this double prints the same info.
> > I moved the first log_debug as default: of the switch statement.
>
> Yeah, that makes sense.
>
> > > In ikev2_print_id() it would seem more appropriate to use strlcat()
> > > instead of strlcpy() in the switch and and reserve the special dance
> > > of adjusting idstr and idstrlen for the /* XXX test */ path, but that
> > > should be done in a separate diff.
> >
> > Oh boy, this is even worse than the above case. WTF!
>
> Indeed.
>
> > So many things are utterly bad in this function and the XXX test path is
> > very broken. I tried to fix this up a bit.
>
> Should s4 and s6 not be struct sockaddr_in{,6} on the stack and get a
> treatment similar to start4 and start6 in ikev2_pld_ts()? This buf thing
> seems completely nuts. The weird length check in the FQDN case could be
> done with a simple comparison to BUFSIZ and then buf could become a
> local 3-byte buffer in the for loop of the XXX test case.
>
> However, I completely understand if you don't want to touch more of this
> mess than you already did...
It never pays out to be lazy. Fixed in diff below.
Guess that saves around 990bytes of stack :)
> Just one thing, otherwise your diff is ok:
>
> > @@ -6980,9 +6975,11 @@ ikev2_print_id(struct iked_id *id, char
> > break;
> > default:
> > /* XXX test */
> > - for (i = 0; i < ((ssize_t)idstrlen - 1) && i < len; i++)
> > - snprintf(idstr + i, idstrlen - i,
> > - "%02x", ptr[i]);
> > + for (i = 0; i < len; i++) {
> > + snprintf(buf, sizeof(buf), "%02x", ptr[i]);
> > + if (strlcat(idstr, buf, idstrlen) >= idstrlen)
>
> Should this not return -1 like the other cases?
>
>From my understanding the idea is that we try to dump as much of the
unknown playload as possible. The code before did that with this
complicated logic:
i < ((ssize_t)idstrlen - 1) && i < len
I decided to just fill the buffer until strlcat() complains and exit the
loop then.
--
:wq Claudio
Index: ikev2.c
===
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.370
diff -u -p -r1.370 ikev2.c
--- ikev2.c 13 Jun 2023 12:34:12 - 1.370
+++ ikev2.c 14 Jun 2023 12:28:24 -
@@ -2285,7 +2285,7 @@ ikev2_nat_detection(struct iked *env, st
struct sockaddr_in *in4;
struct sockaddr_in6 *in6;
ssize_t ret = -1;
- struct sockaddr *src, *dst, *ss;
+ struct sockaddr_storage *src, *dst, *ss;
uint64_t rspi, ispi;
struct ibuf *buf;
uint32_t rnd;
@@ -2299,13 +2299,13 @@ ikev2_nat_detection(struct iked *env, st
return (-1);
ispi = hdr->ike_ispi;
rspi = hdr->ike_rspi;
- src = (struct sockaddr *)&msg->msg_peer;
- dst = (struct sockaddr *)&msg->msg_local;
+ src = &msg->msg_peer;
+ dst = &msg->msg_local;
} else {
ispi = htobe64(sa->sa_hdr.sh_ispi);
rspi = htobe64(sa->sa_hdr.sh_rspi);
- src = (struct sockaddr *)&msg->msg_local;
- dst = (struct sockaddr *)&msg->msg_peer;
+ src = &msg->msg_local;
+ dst = &msg->msg_peer;
}
ctx = EVP_MD_CTX_new();
@@ -2337,7 +2337,7 @@ ikev2_nat_detection(struct iked *env, st
EVP_DigestUpdate(ctx, &ispi, sizeof(ispi));
EVP_DigestUpdate(ctx, &rspi, sizeof(rspi));
- switch (ss->sa_family) {
+ switch (ss->ss_family) {
case AF_INET:
in4 = (struct sockaddr_in *)ss;
EVP_DigestUpdate(ctx, &in4->sin_addr.s_addr,
@@ -6902,15 +6902,14 @@ ikev2_print_static_id(struct iked_static
int
ikev2_print_id(struct iked_id *id, char *idstr, size_t idstrlen)
{
- uint8_t buf[BUFSIZ], *ptr;
- struct sockaddr_in *s4;
- struct sockaddr_in6 *s6;
+ uint8_t *ptr;
+ struct sockaddr_in s4 = { 0 };
+ struct sockaddr_in
Re: iked: replace last print_host users to print_addr
On Wed, Jun 14, 2023 at 11:38:15AM +0200, Claudio Jeker wrote:
> On Wed, Jun 14, 2023 at 11:10:52AM +0200, Theo Buehler wrote:
> > On Wed, Jun 14, 2023 at 10:44:23AM +0200, Claudio Jeker wrote:
> > > There is no real need to have print_host() with the extra arguments.
> > > So convert the last remaining print_host() calls to use print_addr().
> > > I'm not entierly sure how to really test all these code paths but the
> > > changes are failry simple.
> >
> > Thanks for beating me to it...
> >
> > In ikev2_pld_cp() you need to garbage collect buf and replace its
> > remaining uses by the appropriate print_addr().
>
> Oh boy, I missed the fact that this double prints the same info.
> I moved the first log_debug as default: of the switch statement.
Yeah, that makes sense.
> > In ikev2_print_id() it would seem more appropriate to use strlcat()
> > instead of strlcpy() in the switch and and reserve the special dance
> > of adjusting idstr and idstrlen for the /* XXX test */ path, but that
> > should be done in a separate diff.
>
> Oh boy, this is even worse than the above case. WTF!
Indeed.
> So many things are utterly bad in this function and the XXX test path is
> very broken. I tried to fix this up a bit.
Should s4 and s6 not be struct sockaddr_in{,6} on the stack and get a
treatment similar to start4 and start6 in ikev2_pld_ts()? This buf thing
seems completely nuts. The weird length check in the FQDN case could be
done with a simple comparison to BUFSIZ and then buf could become a
local 3-byte buffer in the for loop of the XXX test case.
However, I completely understand if you don't want to touch more of this
mess than you already did...
Just one thing, otherwise your diff is ok:
> @@ -6980,9 +6975,11 @@ ikev2_print_id(struct iked_id *id, char
> break;
> default:
> /* XXX test */
> - for (i = 0; i < ((ssize_t)idstrlen - 1) && i < len; i++)
> - snprintf(idstr + i, idstrlen - i,
> - "%02x", ptr[i]);
> + for (i = 0; i < len; i++) {
> + snprintf(buf, sizeof(buf), "%02x", ptr[i]);
> + if (strlcat(idstr, buf, idstrlen) >= idstrlen)
Should this not return -1 like the other cases?
> + break;
> + }
> break;
> }
Re: iked: replace last print_host users to print_addr
On Wed, Jun 14, 2023 at 11:10:52AM +0200, Theo Buehler wrote:
> On Wed, Jun 14, 2023 at 10:44:23AM +0200, Claudio Jeker wrote:
> > There is no real need to have print_host() with the extra arguments.
> > So convert the last remaining print_host() calls to use print_addr().
> > I'm not entierly sure how to really test all these code paths but the
> > changes are failry simple.
>
> Thanks for beating me to it...
>
> In ikev2_pld_cp() you need to garbage collect buf and replace its
> remaining uses by the appropriate print_addr().
Oh boy, I missed the fact that this double prints the same info.
I moved the first log_debug as default: of the switch statement.
> In ikev2_print_id() it would seem more appropriate to use strlcat()
> instead of strlcpy() in the switch and and reserve the special dance
> of adjusting idstr and idstrlen for the /* XXX test */ path, but that
> should be done in a separate diff.
Oh boy, this is even worse than the above case. WTF!
So many things are utterly bad in this function and the XXX test path is
very broken. I tried to fix this up a bit.
--
:wq Claudio
Index: ikev2.c
===
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.370
diff -u -p -r1.370 ikev2.c
--- ikev2.c 13 Jun 2023 12:34:12 - 1.370
+++ ikev2.c 14 Jun 2023 09:37:31 -
@@ -2285,7 +2285,7 @@ ikev2_nat_detection(struct iked *env, st
struct sockaddr_in *in4;
struct sockaddr_in6 *in6;
ssize_t ret = -1;
- struct sockaddr *src, *dst, *ss;
+ struct sockaddr_storage *src, *dst, *ss;
uint64_t rspi, ispi;
struct ibuf *buf;
uint32_t rnd;
@@ -2299,13 +2299,13 @@ ikev2_nat_detection(struct iked *env, st
return (-1);
ispi = hdr->ike_ispi;
rspi = hdr->ike_rspi;
- src = (struct sockaddr *)&msg->msg_peer;
- dst = (struct sockaddr *)&msg->msg_local;
+ src = &msg->msg_peer;
+ dst = &msg->msg_local;
} else {
ispi = htobe64(sa->sa_hdr.sh_ispi);
rspi = htobe64(sa->sa_hdr.sh_rspi);
- src = (struct sockaddr *)&msg->msg_local;
- dst = (struct sockaddr *)&msg->msg_peer;
+ src = &msg->msg_local;
+ dst = &msg->msg_peer;
}
ctx = EVP_MD_CTX_new();
@@ -2337,7 +2337,7 @@ ikev2_nat_detection(struct iked *env, st
EVP_DigestUpdate(ctx, &ispi, sizeof(ispi));
EVP_DigestUpdate(ctx, &rspi, sizeof(rspi));
- switch (ss->sa_family) {
+ switch (ss->ss_family) {
case AF_INET:
in4 = (struct sockaddr_in *)ss;
EVP_DigestUpdate(ctx, &in4->sin_addr.s_addr,
@@ -6931,9 +6931,6 @@ ikev2_print_id(struct iked_id *id, char
strlcat(idstr, "/", idstrlen) >= idstrlen)
return (-1);
- idstrlen -= strlen(idstr);
- idstr += strlen(idstr);
-
switch (id->id_type) {
case IKEV2_ID_IPV4:
s4 = (struct sockaddr_in *)buf;
@@ -6941,8 +6938,7 @@ ikev2_print_id(struct iked_id *id, char
s4->sin_len = sizeof(*s4);
memcpy(&s4->sin_addr.s_addr, ptr, len);
- if (print_host((struct sockaddr *)s4,
- idstr, idstrlen) == NULL)
+ if (strlcat(idstr, print_addr(s4), idstrlen) >= idstrlen)
return (-1);
break;
case IKEV2_ID_FQDN:
@@ -6953,7 +6949,7 @@ ikev2_print_id(struct iked_id *id, char
if ((str = get_string(ptr, len)) == NULL)
return (-1);
- if (strlcpy(idstr, str, idstrlen) >= idstrlen) {
+ if (strlcat(idstr, str, idstrlen) >= idstrlen) {
free(str);
return (-1);
}
@@ -6965,14 +6961,13 @@ ikev2_print_id(struct iked_id *id, char
s6->sin6_len = sizeof(*s6);
memcpy(&s6->sin6_addr, ptr, len);
- if (print_host((struct sockaddr *)s6,
- idstr, idstrlen) == NULL)
+ if (strlcat(idstr, print_addr(s6), idstrlen) >= idstrlen)
return (-1);
break;
case IKEV2_ID_ASN1_DN:
if ((str = ca_asn1_name(ptr, len)) == NULL)
return (-1);
- if (strlcpy(idstr, str, idstrlen) >= idstrlen) {
+ if (strlcat(idstr, str, idstrlen) >= idstrlen) {
OPENSSL_free(str);
return (-1);
}
@@ -6980,9 +6975,11 @@ ikev2_print_id(struct iked_id *id, char
break;
default:
/* XXX test */
- for (i = 0; i < ((ssize_t)idstrlen - 1) && i < len; i++)
- snprintf(i
Re: iked: replace last print_host users to print_addr
On Wed, Jun 14, 2023 at 10:44:23AM +0200, Claudio Jeker wrote: > There is no real need to have print_host() with the extra arguments. > So convert the last remaining print_host() calls to use print_addr(). > I'm not entierly sure how to really test all these code paths but the > changes are failry simple. Thanks for beating me to it... In ikev2_pld_cp() you need to garbage collect buf and replace its remaining uses by the appropriate print_addr(). In ikev2_print_id() it would seem more appropriate to use strlcat() instead of strlcpy() in the switch and and reserve the special dance of adjusting idstr and idstrlen for the /* XXX test */ path, but that should be done in a separate diff. other than that this looks good.
iked: replace last print_host users to print_addr
There is no real need to have print_host() with the extra arguments.
So convert the last remaining print_host() calls to use print_addr().
I'm not entierly sure how to really test all these code paths but the
changes are failry simple.
--
:wq Claudio
Index: ikev2.c
===
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.370
diff -u -p -r1.370 ikev2.c
--- ikev2.c 13 Jun 2023 12:34:12 - 1.370
+++ ikev2.c 13 Jun 2023 13:18:56 -
@@ -2285,7 +2285,7 @@ ikev2_nat_detection(struct iked *env, st
struct sockaddr_in *in4;
struct sockaddr_in6 *in6;
ssize_t ret = -1;
- struct sockaddr *src, *dst, *ss;
+ struct sockaddr_storage *src, *dst, *ss;
uint64_t rspi, ispi;
struct ibuf *buf;
uint32_t rnd;
@@ -2299,13 +2299,13 @@ ikev2_nat_detection(struct iked *env, st
return (-1);
ispi = hdr->ike_ispi;
rspi = hdr->ike_rspi;
- src = (struct sockaddr *)&msg->msg_peer;
- dst = (struct sockaddr *)&msg->msg_local;
+ src = &msg->msg_peer;
+ dst = &msg->msg_local;
} else {
ispi = htobe64(sa->sa_hdr.sh_ispi);
rspi = htobe64(sa->sa_hdr.sh_rspi);
- src = (struct sockaddr *)&msg->msg_local;
- dst = (struct sockaddr *)&msg->msg_peer;
+ src = &msg->msg_local;
+ dst = &msg->msg_peer;
}
ctx = EVP_MD_CTX_new();
@@ -2337,7 +2337,7 @@ ikev2_nat_detection(struct iked *env, st
EVP_DigestUpdate(ctx, &ispi, sizeof(ispi));
EVP_DigestUpdate(ctx, &rspi, sizeof(rspi));
- switch (ss->sa_family) {
+ switch (ss->ss_family) {
case AF_INET:
in4 = (struct sockaddr_in *)ss;
EVP_DigestUpdate(ctx, &in4->sin_addr.s_addr,
@@ -6941,8 +6941,7 @@ ikev2_print_id(struct iked_id *id, char
s4->sin_len = sizeof(*s4);
memcpy(&s4->sin_addr.s_addr, ptr, len);
- if (print_host((struct sockaddr *)s4,
- idstr, idstrlen) == NULL)
+ if (strlcpy(idstr, print_addr(s4), idstrlen) >= idstrlen)
return (-1);
break;
case IKEV2_ID_FQDN:
@@ -6965,8 +6964,7 @@ ikev2_print_id(struct iked_id *id, char
s6->sin6_len = sizeof(*s6);
memcpy(&s6->sin6_addr, ptr, len);
- if (print_host((struct sockaddr *)s6,
- idstr, idstrlen) == NULL)
+ if (strlcpy(idstr, print_addr(s6), idstrlen) >= idstrlen)
return (-1);
break;
case IKEV2_ID_ASN1_DN:
Index: ikev2_pld.c
===
RCS file: /cvs/src/sbin/iked/ikev2_pld.c,v
retrieving revision 1.129
diff -u -p -r1.129 ikev2_pld.c
--- ikev2_pld.c 6 Jun 2023 16:09:35 - 1.129
+++ ikev2_pld.c 14 Jun 2023 08:41:36 -
@@ -1522,9 +1522,8 @@ int
ikev2_pld_ts(struct iked *env, struct ikev2_payload *pld,
struct iked_message *msg, size_t offset, size_t left, unsigned int type)
{
- struct sockaddr_in s4;
- struct sockaddr_in6 s6;
- uint8_t buf[2][128];
+ struct sockaddr_in start4, end4;
+ struct sockaddr_in6 start6, end6;
uint8_t *msgbuf = ibuf_data(msg->msg_data);
uint8_t *ptr;
@@ -1539,22 +1538,21 @@ ikev2_pld_ts(struct iked *env, struct ik
return (-1);
}
- bzero(&s4, sizeof(s4));
- s4.sin_family = AF_INET;
- s4.sin_len = sizeof(s4);
- memcpy(&s4.sin_addr.s_addr, ptr, 4);
+ bzero(&start4, sizeof(start4));
+ start4.sin_family = AF_INET;
+ start4.sin_len = sizeof(start4);
+ memcpy(&start4.sin_addr.s_addr, ptr, 4);
ptr += 4;
left -= 4;
- print_host((struct sockaddr *)&s4,
- (char *)buf[0], sizeof(buf[0]));
- memcpy(&s4.sin_addr.s_addr, ptr, 4);
+ bzero(&end4, sizeof(end4));
+ end4.sin_family = AF_INET;
+ end4.sin_len = sizeof(end4);
+ memcpy(&end4.sin_addr.s_addr, ptr, 4);
left -= 4;
- print_host((struct sockaddr *)&s4,
- (char *)buf[1], sizeof(buf[1]));
log_debug("%s: start %s end %s", __func__,
- buf[0], buf[1]);
+ print_addr(&start4), print_addr(&end4));
break;
case IKEV2_TS_IPV6_ADDR_RANGE:
if (left < 2 * 16) {
@@ -1563,21 +1561
vmd: relax absolute path requirement for configtest
Hello,
after vmd.c rv 1.142 vmd(8) errors when ran with a non-absolute path;
this makes using -n (configtest) slightly more verbose, as the full
path is needed and not just `vmd -n'.
here's an attempt at relaxing the requirement for the -n case only.
since we're not going to run any vm it should be fine, but apologize
if I've missed something.
Thanks,
Omar Polo
diff /usr/src
commit - 66c6b79616659a94b04092c9f103e3aa29809704
path + /usr/src
blob - 86a5132fe224856a3679f1a1d6863b87b561c9d0
file + usr.sbin/vmd/vmd.c
--- usr.sbin/vmd/vmd.c
+++ usr.sbin/vmd/vmd.c
@@ -874,7 +874,7 @@ main(int argc, char **argv)
log_setverbose(env->vmd_verbose);
/* Re-exec from the vmm child process requires an absolute path. */
- if (proc_id == PROC_PARENT && *argv[0] != '/')
+ if (proc_id == PROC_PARENT && *argv[0] != '/' && !env->vmd_noaction)
fatalx("re-exec requires execution with an absolute path");
env->argv0 = argv[0];
