Mailing List Archive: FreeBSD change for openssh

FreeBSD change for openssh

Nov 15, 2021, 8:44 AM

Post #1 of 9 (1027 views)

I've been working on cleanup of remnants of Internet Class A/B/C in
FreeBSD, and came across a piece of code in sshconnect.c that I'd
like to change. The current code checks for loopback addresses by
picking apart the address as Class A (24 bit shift). FreeBSD has a
newer IN_LOOPBACK() macro that determines whether an address is in the
loopback range, and I'd like to use that. As not all systems provide
such a macro, I'd propose a default version that is essentially the
current FreeBSD version. Part of the reason for using the system macro
is that there is a proposed change to the reserved space for loopback
heading toward the IETF, reserving 127.0.0.0/16 rather than /8.

The following is a proposed change to sshconnect.c.

diff --git a/crypto/openssh/sshconnect.c b/crypto/openssh/sshconnect.c
index 8f7541942ac1..74636005eb7b 100644
--- a/crypto/openssh/sshconnect.c
+++ b/crypto/openssh/sshconnect.c
@@ -592,13 +592,20 @@ confirm(const char *prompt, const char *fingerprint)
}
}

+/*
+ * <netinet/in.h> may provide an IN_LOOPBACK() macro; use it if provided.
+ */
+#ifndef IN_LOOPBACK
+#define IN_LOOPBACK(i) (((i) & 0xff000000) == 0x7f000000)
+#endif
+
static int
sockaddr_is_local(struct sockaddr *hostaddr)
{
switch (hostaddr->sa_family) {
case AF_INET:
- return (ntohl(((struct sockaddr_in *)hostaddr)->
- sin_addr.s_addr) >> 24) == IN_LOOPBACKNET;
+ return (IN_LOOPBACK(ntohl(((struct sockaddr_in *)hostaddr)->
+ sin_addr.s_addr)));
case AF_INET6:
return IN6_IS_ADDR_LOOPBACK(
&(((struct sockaddr_in6 *)hostaddr)->sin6_addr));

Comments or suggestions?

Thanks,
Mike
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: FreeBSD change for openssh [ In reply to ]

emaste at freebsd

Nov 17, 2021, 7:10 AM

Post #2 of 9 (1027 views)

Permalink

On Mon, 15 Nov 2021 at 11:51, Mike Karels <karels@freebsd.org> wrote:
>
> +#ifndef IN_LOOPBACK
> +#define IN_LOOPBACK(i) (((i) & 0xff000000) == 0x7f000000)
> +#endif
...
> static int
> sockaddr_is_local(struct sockaddr *hostaddr)
> {
> switch (hostaddr->sa_family) {
> case AF_INET:
> - return (ntohl(((struct sockaddr_in *)hostaddr)->
> - sin_addr.s_addr) >> 24) == IN_LOOPBACKNET;
> + return (IN_LOOPBACK(ntohl(((struct sockaddr_in *)hostaddr)->
> + sin_addr.s_addr)));

Looks fine to me.

IMO #ifndef is reasonable as it's unlikely that IN_LOOPBACK() would be
implemented as other than a macro anywhere, but could be a full
autoconf check if that's what OpenSSH folks want.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: FreeBSD change for openssh [ In reply to ]

karels at freebsd

Dec 9, 2021, 7:17 AM

Post #3 of 9 (1014 views)

Permalink

I submitted this proposed change for openssh a while back, designed to
reduce embedded knowledge of old Class A/B/C networks and use the system's
IN_LOOPBACK macro where it exists. I received just one comment:

Ed Maste <emaste@freebsd.org> wrote:
> On Mon, 15 Nov 2021 at 11:51, Mike Karels <karels@freebsd.org> wrote:
> >
> > +#ifndef IN_LOOPBACK
> > +#define IN_LOOPBACK(i) (((i) & 0xff000000) == 0x7f000000)
> > +#endif
> ...
> > static int
> > sockaddr_is_local(struct sockaddr *hostaddr)
> > {
> > switch (hostaddr->sa_family) {
> > case AF_INET:
> > - return (ntohl(((struct sockaddr_in *)hostaddr)->
> > - sin_addr.s_addr) >> 24) == IN_LOOPBACKNET;
> > + return (IN_LOOPBACK(ntohl(((struct sockaddr_in *)hostaddr)->
> > + sin_addr.s_addr)));

> Looks fine to me.

> IMO #ifndef is reasonable as it's unlikely that IN_LOOPBACK() would be
> implemented as other than a macro anywhere, but could be a full
> autoconf check if that's what OpenSSH folks want.

Any other comments or suggestions? Any general thoughts on doing this?
I'd like to get it committed if possible.

Thanks,
Mike
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: FreeBSD change for openssh [ In reply to ]

loganaden at gmail

Dec 9, 2021, 9:45 AM

Post #4 of 9 (1014 views)

Permalink

This looks like a good change to me.

On Thu, Dec 9, 2021 at 7:23 PM Mike Karels <karels@freebsd.org> wrote:
>
> I submitted this proposed change for openssh a while back, designed to
> reduce embedded knowledge of old Class A/B/C networks and use the system's
> IN_LOOPBACK macro where it exists. I received just one comment:
>
> Ed Maste <emaste@freebsd.org> wrote:
> > On Mon, 15 Nov 2021 at 11:51, Mike Karels <karels@freebsd.org> wrote:
> > >
> > > +#ifndef IN_LOOPBACK
> > > +#define IN_LOOPBACK(i) (((i) & 0xff000000) == 0x7f000000)
> > > +#endif
> > ...
> > > static int
> > > sockaddr_is_local(struct sockaddr *hostaddr)
> > > {
> > > switch (hostaddr->sa_family) {
> > > case AF_INET:
> > > - return (ntohl(((struct sockaddr_in *)hostaddr)->
> > > - sin_addr.s_addr) >> 24) == IN_LOOPBACKNET;
> > > + return (IN_LOOPBACK(ntohl(((struct sockaddr_in *)hostaddr)->
> > > + sin_addr.s_addr)));
>
> > Looks fine to me.
>
> > IMO #ifndef is reasonable as it's unlikely that IN_LOOPBACK() would be
> > implemented as other than a macro anywhere, but could be a full
> > autoconf check if that's what OpenSSH folks want.
>
> Any other comments or suggestions? Any general thoughts on doing this?
> I'd like to get it committed if possible.
>
> Thanks,
> Mike
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev@mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: FreeBSD change for openssh [ In reply to ]

karels at freebsd

Dec 16, 2021, 1:55 PM

Post #5 of 9 (1014 views)

Permalink

Loganaden Velvindron <loganaden@gmail.com> wrote:

> This looks like a good change to me.

I haven't seen any other comments. Can you commit this? If not, what
is the procedure to get it committed?

Thanks,
Mike

> On Thu, Dec 9, 2021 at 7:23 PM Mike Karels <karels@freebsd.org> wrote:
> >
> > I submitted this proposed change for openssh a while back, designed to
> > reduce embedded knowledge of old Class A/B/C networks and use the system's
> > IN_LOOPBACK macro where it exists. I received just one comment:
> >
> > Ed Maste <emaste@freebsd.org> wrote:
> > > On Mon, 15 Nov 2021 at 11:51, Mike Karels <karels@freebsd.org> wrote:
> > > >
> > > > +#ifndef IN_LOOPBACK
> > > > +#define IN_LOOPBACK(i) (((i) & 0xff000000) == 0x7f000000)
> > > > +#endif
> > > ...
> > > > static int
> > > > sockaddr_is_local(struct sockaddr *hostaddr)
> > > > {
> > > > switch (hostaddr->sa_family) {
> > > > case AF_INET:
> > > > - return (ntohl(((struct sockaddr_in *)hostaddr)->
> > > > - sin_addr.s_addr) >> 24) == IN_LOOPBACKNET;
> > > > + return (IN_LOOPBACK(ntohl(((struct sockaddr_in *)hostaddr)->
> > > > + sin_addr.s_addr)));
> >
> > > Looks fine to me.
> >
> > > IMO #ifndef is reasonable as it's unlikely that IN_LOOPBACK() would be
> > > implemented as other than a macro anywhere, but could be a full
> > > autoconf check if that's what OpenSSH folks want.
> >
> > Any other comments or suggestions? Any general thoughts on doing this?
> > I'd like to get it committed if possible.
> >
> > Thanks,
> > Mike
> > _______________________________________________
> > openssh-unix-dev mailing list
> > openssh-unix-dev@mindrot.org
> > https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev@mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: FreeBSD change for openssh [ In reply to ]

dtucker at dtucker

Dec 16, 2021, 3:11 PM

Post #6 of 9 (1014 views)

Permalink

On Fri, 17 Dec 2021 at 09:05, Mike Karels <karels@freebsd.org> wrote:
[...]
> I haven't seen any other comments. Can you commit this? If not, what
> is the procedure to get it committed?

Leaving aside the implementation details, the whole concept seems
questionable to me. Between the things that should work that won't
(eg non-updated bogon filters or host netmasks) and those that do work
that shouldn't (eg misidentifying external traffic as control plane)
it seems like more trouble than it's worth.

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: FreeBSD change for openssh [ In reply to ]

dtucker at dtucker

Dec 16, 2021, 3:14 PM

Post #7 of 9 (1014 views)

Permalink

On Fri, 17 Dec 2021 at 10:11, Darren Tucker <dtucker@dtucker.net> wrote:
>
> On Fri, 17 Dec 2021 at 09:05, Mike Karels <karels@freebsd.org> wrote:
> [...]
> > I haven't seen any other comments. Can you commit this? If not, what
> > is the procedure to get it committed?
>
> Leaving aside the implementation details, the whole concept seems
> questionable to me.

To be specific: the concept I find questionable is repurposing (most
of) 127.0.0.0/8 as global unicast addresses.

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: FreeBSD change for openssh [ In reply to ]

karels at freebsd

Dec 16, 2021, 4:56 PM

Post #8 of 9 (1014 views)

Permalink

Darren Tucker <dtucker@dtucker.net> wrote:
> On Fri, 17 Dec 2021 at 10:11, Darren Tucker <dtucker@dtucker.net> wrote:
> >
> > On Fri, 17 Dec 2021 at 09:05, Mike Karels <karels@freebsd.org> wrote:
> > [...]
> > > I haven't seen any other comments. Can you commit this? If not, what
> > > is the procedure to get it committed?
> >
> > Leaving aside the implementation details, the whole concept seems
> > questionable to me.

> To be specific: the concept I find questionable is repurposing (most
> of) 127.0.0.0/8 as global unicast addresses.

Neither this change, nor the larger set of changes I'm making in FreeBSD,
redefine the loopback network. FreeBSD has had an IN_LOOPBACK macro for
years, with essentially the same definition as the default I provided.
The goal of my changes is to reduce the knowledge of the obsolete Class
A/B/C network structure as much as possible. It is true that this change
makes it easier to change the system definition of the loopback network,
but I am not doing that. It seems to me to be preferable for programs like
OpenSSH to use system definitions where possible, and not to hard-code
things like the definition of the loopback network.

Mike
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: FreeBSD change for openssh [ In reply to ]

karels at freebsd

Dec 20, 2021, 11:08 AM

Post #9 of 9 (1004 views)

Permalink

Following up on my own email; I wrote:

> Darren Tucker <dtucker@dtucker.net> wrote:
> > On Fri, 17 Dec 2021 at 10:11, Darren Tucker <dtucker@dtucker.net> wrote:
> > >
> > > On Fri, 17 Dec 2021 at 09:05, Mike Karels <karels@freebsd.org> wrote:
> > > [...]
> > > > I haven't seen any other comments. Can you commit this? If not, what
> > > > is the procedure to get it committed?
> > >
> > > Leaving aside the implementation details, the whole concept seems
> > > questionable to me.

> > To be specific: the concept I find questionable is repurposing (most
> > of) 127.0.0.0/8 as global unicast addresses.

> Neither this change, nor the larger set of changes I'm making in FreeBSD,
> redefine the loopback network. FreeBSD has had an IN_LOOPBACK macro for
> years, with essentially the same definition as the default I provided.
> The goal of my changes is to reduce the knowledge of the obsolete Class
> A/B/C network structure as much as possible. It is true that this change
> makes it easier to change the system definition of the loopback network,
> but I am not doing that. It seems to me to be preferable for programs like
> OpenSSH to use system definitions where possible, and not to hard-code
> things like the definition of the loopback network.

Where do we stand on this? Any other opinions?

Mike
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev