On Fri, Mar 22, 2019 at 09:57:29PM +0100, Michał Koc wrote:
> W dniu 22.03.2019 o 11:19, Michał Koc pisze:
> > W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze:
> > > On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote:
> > >
> > > > W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
> > > > > On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
> > > > > > On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
> > > > > > > > I also fixed a case of parsing IPv6 addresses.
> > > > > > > >
> > > > > > > > Anyone willing to ok?
> > > > > > See comments inline.
> > > > > >
> > > > > > > And now also with a lexer bug fixed. Earlier I
> > > > > > > thougt it was an order
> > > > > > > dependency in the clauses. But is was an order dependency in
> > > > > > > comment
> > > > > > > lines and empty lines.
> > > > > > > +check_peer_addr(const char *peer_addr)
> > > > > > > +{
> > > > > > > + int valid = 1;
> > > > > > > + short peer_family = AF_UNSPEC;
> > > > > > > + struct ifaddrs *ifap = NULL, *ifa;
> > > > > > > + struct syncpeer *peer;
> > > > > > > + struct sockaddr_in sa;
> > > > > > > + struct sockaddr_in6 sa6;
> > > > > > > +
> > > > > > > + if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
> > > > > > > + peer_family = AF_INET;
> > > > > > > +
> > > > > > > + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6,
> > > > > > > peer_addr,
> > > > > > > + &sa6.sin6_addr) == 1)
> > > > > > > + peer_family = AF_INET6;
> > > > > > `peer_addr' must not be a CIDR network, so I'd go with the more AF
> > > > > > agnostic getaddrinfo(3) and check for res->ai_family in any case...
> > > > > >
> > > > > > > + if (peer_family == AF_UNSPEC) {
> > > > > > > + log_msg(2, "config: skip unparseable peer %s",
> > > > > > > peer_addr);
> > > > > > > + valid = 0;
> > > > > > > + }
> > > > > > ..but `peer_addr' may also be a hostname, so that is not handled by
> > > > > > your diff:
> > > > > >
> > > > > > net.h: char *name; /* FQDN or an IP, from conf
> > > > > > */
> > > > > >
> > > > > > getaddrinfo(3) can resolve however, thus inet_pton(3)
> > > > > > should not be used
> > > > > > here.
> > > > > >
> > > > > > Either way, this is a job for host_ip() as found in pfctl or bgpd.
> > > > > > Other daemons like iked still have host_v4() and
> > > > > > host_v6(). Parsers use
> > > > > > these functions to check for valid addresses, so I'd say
> > > > > > sasyncd should
> > > > > > be no exception and adopt the same approach.
> > > > > >
> > > > > > > @@ -325,7 +386,7 @@ yylex(void)
> > > > > > > /* Numerical token? */
> > > > > > > if (isdigit(*confptr)) {
> > > > > > > for (p = confptr; *p; p++)
> > > > > > > - if (*p == '.') /* IP address, or bad input */
> > > > > > > + if (*p == '.' || *p == ':') /* IP
> > > > > > > address, or bad input */
> > > > > > This fixes the parser as in
> > > > > >
> > > > > > # echo peer 2001:db8::1 > conf
> > > > > > # sasyncd -dnv -c conf
> > > > > > config: syntax error
> > > > > > # ./obj/sasyncd -dnv -c conf
> > > > > > configuration OK
> > > > > >
> > > > > > But I have not actually tested this with a proper IPv6
> > > > > > setup since I do
> > > > > > not use sasyncd; did you?
> > > > > >
> > > > > > > @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
> > > > > > > if (*s == '#') {
> > > > > > > while (*s != '\n' && s < buf + conflen)
> > > > > > > s++;
> > > > > > > + while (*s == '\n' && s < buf + conflen)
> > > > > > > + s++;
> > > > > > > + s--;
> > > > > > OK kn for this fix alone.
> > > > > >
> > > > > I'll test an ipv6 seup and commit the lexer parts if those work.
> > > > > Michal, can you work on the first set of comments? I think Klemens is
> > > > > right.
> > > > >
> > > > > -Otto
> > > > >
> > > > Of course, I'll follow the comments soon
> > > >
> > > > Best Regards
> > > > M.K.
> >
> > the diff is ready, what happened here:
> >
> > 1. host => ip manipulation moved to host_ip() function in net.c and is
> > using getaddrinfo()
> > 2. net_set_sa() in net.c modified to use host_ip()
> >
> > Bets Regards
> > M.K
>
> Do not actually compare two structs sockaddr if their lengths differ.
>
I did not have time yet to run this code, but some comments inline.
-Otto
> Index: conf.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
> retrieving revision 1.19
> diff -u -p -r1.19 conf.y
> --- conf.y 9 Apr 2017 02:40:24 -0000 1.19
> +++ conf.y 22 Mar 2019 20:55:21 -0000
> @@ -30,8 +30,10 @@
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <sys/socket.h>
> +#include <arpa/inet.h>
> #include <ctype.h>
> #include <fcntl.h>
> +#include <ifaddrs.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -48,6 +50,7 @@ struct cfgstate cfgstate;
> int conflen = 0;
> char *confbuf, *confptr;
>
> +int check_peer_addr(const char *);
> int yyparse(void);
> int yylex(void);
> void yyerror(const char *);
> @@ -172,17 +175,8 @@ setting : INTERFACE STRING
> | PEER STRING
> {
> struct syncpeer *peer;
> - int duplicate = 0;
>
> - for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
> - peer = LIST_NEXT(peer, link))
> - if (strcmp($2, peer->name) == 0) {
> - duplicate++;
> - break;
> - }
> - if (duplicate)
> - free($2);
> - else {
> + if (check_peer_addr($2)) {
> peer = calloc(1, sizeof *peer);
> if (!peer) {
> log_err("config: calloc(1, %lu) "
> @@ -191,10 +185,11 @@ setting : INTERFACE STRING
> YYERROR;
> }
> peer->name = $2;
> - }
> - LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
> - cfgstate.peercnt++;
> - log_msg(2, "config: add peer %s", peer->name);
> + LIST_INSERT_HEAD(&cfgstate.peerlist, peer,
> link);
> + cfgstate.peercnt++;
> + log_msg(2, "config: add peer %s", peer->name);
> + } else
> + free($2);
> }
> | LISTEN ON STRING af port
> {
> @@ -281,6 +276,51 @@ match(char *token)
> sizeof keywords[0], match_cmp);
>
> return k ? k->value : STRING;
> +}
> +
> +int
> +check_peer_addr(const char *peer_addr)
> +{
> + struct ifaddrs *ifap = 0, *ifa;
> + struct syncpeer *peer;
> + struct sockaddr *sa, *peer_sa;
> +
> + sa = host_ip(peer_addr);
> +
> + if(!sa)
I prefer comparing to NULL for pointers.
> + log_msg(2, "config: skip unparseable peer %s", peer_addr);
> +
> + if (sa && getifaddrs(&ifap) == 0) {
> + for (ifa = ifap; ifa && sa; ifa = ifa->ifa_next) {
> + if (!ifa->ifa_addr || ifa->ifa_addr->sa_family !=
> sa->sa_family)
Same here.
> + continue;
> +
> + if (sa->sa_len == ifa->ifa_addr->sa_len && memcmp(sa,
> ifa->ifa_addr, sa->sa_len) == 0) {
> + log_msg(2, "config: skip local peer %s",
> peer_addr);
> + free(sa);
> + sa = 0;
Use NULL for pointers.
> + }
> + }
> + freeifaddrs(ifap);
> + }
> +
> + if (sa) {
> + for (peer = LIST_FIRST(&cfgstate.peerlist); peer && sa;
> + peer = LIST_NEXT(peer, link)) {
> + peer_sa = host_ip(peer->name);
> +
> + if (sa->sa_len == peer_sa->sa_len && memcmp(sa, peer_sa,
> sa->sa_len) == 0) {
> + log_msg(2, "config: skip duplicate peer %s", peer_addr);
> + free(sa);
> + sa = 0;
Use NULL.
> + }
> + free(peer_sa);
> + }
> + }
> +
> + free(sa);
> +
> + return sa != 0;
And here
> }
>
> int
> Index: net.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/sasyncd/net.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 net.c
> --- net.c 12 Dec 2015 20:04:23 -0000 1.23
> +++ net.c 22 Mar 2019 20:55:21 -0000
> @@ -755,32 +755,26 @@ net_read(struct syncpeer *p, u_int32_t *
> static int
> net_set_sa(struct sockaddr *sa, char *name, in_port_t port)
> {
> - struct sockaddr_in *sin = (struct sockaddr_in *)sa;
> - struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sa;
> + struct sockaddr *peer_sa = host_ip(name);
>
> - if (!name) {
> - /* XXX Assume IPv4 */
> - sa->sa_family = AF_INET;
> - sin->sin_port = htons(port);
> - sin->sin_len = sizeof *sin;
> - return 0;
> - }
> + if (peer_sa) {
>
> - if (inet_pton(AF_INET, name, &sin->sin_addr) == 1) {
> - sa->sa_family = AF_INET;
> - sin->sin_port = htons(port);
> - sin->sin_len = sizeof *sin;
> - return 0;
> - }
> + memcpy(sa, peer_sa, peer_sa->sa_len);
> + free(peer_sa);
> +
> + switch(sa->sa_family) {
> + case AF_INET:
> + ((struct sockaddr_in *)sa)->sin_port = htons(port);
> + break;
> +
> + case AF_INET6:
> + ((struct sockaddr_in6 *)sa)->sin6_port = htons(port);
> + break;
> + }
>
> - if (inet_pton(AF_INET6, name, &sin6->sin6_addr) == 1) {
> - sa->sa_family = AF_INET6;
> - sin6->sin6_port = htons(port);
> - sin6->sin6_len = sizeof *sin6;
> - return 0;
> }
>
> - return -1;
> + return peer_sa == 0 ? -1 : 0;
Use NULL
> }
>
> static void
> @@ -848,4 +842,24 @@ net_check_peers(void *arg)
> {
> net_connect();
> (void)timer_add("peer recheck", 600, net_check_peers, 0);
> +}
> +
> +struct sockaddr *
> +host_ip(const char *hostname) {
> + struct addrinfo *ai = 0;
> + struct sockaddr *sa = 0;
> +
> + if (getaddrinfo(hostname, NULL, NULL, &ai) == 0) {
> +
> + switch (ai->ai_family) {
> + case AF_INET:
> + case AF_INET6:
> + sa = calloc(1, ai->ai_addr->sa_len);
> + memcpy(sa, ai->ai_addr, ai->ai_addr->sa_len);
Always check return value of allocations!
Why use calloc if you are going to overwrite thw whole chunk?
> + break;
> + }
> + freeaddrinfo(ai);
> + }
> +
> + return sa;
> }
> Index: net.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/sasyncd/net.h,v
> retrieving revision 1.5
> diff -u -p -r1.5 net.h
> --- net.h 2 Jun 2006 20:09:43 -0000 1.5
> +++ net.h 22 Mar 2019 20:55:21 -0000
> @@ -53,6 +53,7 @@ enum CTLTYPE { RESERVED = 0, CTL_STATE,
> /* net.c */
> void net_connect(void);
> void net_disconnect_peer(struct syncpeer *);
> +struct sockaddr *host_ip(const char *);
>
> /* net_ctl.c */
> void net_ctl_handle_msg(struct syncpeer *, u_int8_t *, u_int32_t);
