Thanks you for this review, I am going to send a v3 iteration with the changes done and tested.
El 14 de agosto de 2018 16:10:33 CEST, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> escribió:
>On Mon, Aug 13, 2018 at 06:57:08PM +0200, Fernando Fernandez Mancera
>wrote:
>[...]
>> diff --git a/include/nfnl_osf.h b/include/nfnl_osf.h
>> new file mode 100644
>> index 0000000..b676045
>> --- /dev/null
>> +++ b/include/nfnl_osf.h
>
>No need for a new nfnl_osf.h file, please use the existing
>include/osf.h.
>
>> @@ -0,0 +1,10 @@
>> +#ifndef _NFNL_OSF_H
>> +#define _NFNL_OSF_H
>> +
>> +#define OS_SIGNATURES DEFAULT_INCLUDE_PATH "/pf.os"
>> +
>> +bool osf_init;
>
>This needs to be:
>
> extern bool osf_init;
>
>Then, define 'bool osf_init' from the nfnl_osf.c file.
>
>Make sure you update include/Makefile.am so `make distcheck' doesn't
>break.
>
>> +
>> +int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx, int del);
>> +
>> +#endif /* _NFNL_OSF_H */
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index ed3640e..e569029 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -57,6 +57,7 @@ libnftables_la_SOURCES = \
>> services.c \
>> mergesort.c \
>> osf.c \
>> + nfnl_osf.c \
>> tcpopt.c \
>> socket.c \
>> libnftables.c
>> diff --git a/src/nfnl_osf.c b/src/nfnl_osf.c
>> new file mode 100644
>> index 0000000..53e4ec6
>> --- /dev/null
>> +++ b/src/nfnl_osf.c
>> @@ -0,0 +1,424 @@
>> +/*
>> + * Copyright (c) 2005 Evgeniy Polyakov <johnpol@xxxxxxxxxx>
>> + *
>> + *
>> + * This program is free software; you can redistribute it and/or
>modify
>> + * it under the terms of the GNU General Public License as published
>by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>02110-1301, USA.
>> + */
>> +
>> +#include <sys/time.h>
>> +
>> +#include <ctype.h>
>> +#include <errno.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <time.h>
>> +
>> +#include <netinet/ip.h>
>> +#include <netinet/tcp.h>
>> +
>> +#include <linux/unistd.h>
>> +
>> +#include <libmnl/libmnl.h>
>> +
>> +#include <linux/netfilter/nfnetlink.h>
>> +#include <linux/netfilter/nfnetlink_osf.h>
>> +#include <mnl.h>
>> +#include <nfnl_osf.h>
>> +
>> +#define OPTDEL ','
>> +#define OSFPDEL ':'
>> +#define MAXOPTSTRLEN 128
>> +
>> +static struct nf_osf_opt IANA_opts[] = {
>> + { .kind = 0, .length = 1,},
>> + { .kind=1, .length=1,},
>> + { .kind=2, .length=4,},
>> + { .kind=3, .length=3,},
>> + { .kind=4, .length=2,},
>> + { .kind=5, .length=1,}, /* SACK length is not defined */
>> + { .kind=6, .length=6,},
>> + { .kind=7, .length=6,},
>> + { .kind=8, .length=10,},
>> + { .kind=9, .length=2,},
>> + { .kind=10, .length=3,},
>> + { .kind=11, .length=1,}, /* CC: Suppose 1 */
>> + { .kind=12, .length=1,}, /* the same */
>> + { .kind=13, .length=1,}, /* and here too */
>> + { .kind=14, .length=3,},
>> + { .kind=15, .length=1,}, /* TCP Alternate Checksum Data. Length is
>not defined */
>> + { .kind=16, .length=1,},
>> + { .kind=17, .length=1,},
>> + { .kind=18, .length=3,},
>> + { .kind=19, .length=18,},
>> + { .kind=20, .length=1,},
>> + { .kind=21, .length=1,},
>> + { .kind=22, .length=1,},
>> + { .kind=23, .length=1,},
>> + { .kind=24, .length=1,},
>> + { .kind=25, .length=1,},
>> + { .kind=26, .length=1,},
>> +};
>> +
>> +static void uloga(const char *f, struct netlink_ctx *ctx, ...)
>> +{
>> + if (!(ctx->debug_mask & NFT_DEBUG_NETLINK))
>> + return;
>> +
>> + nft_print(ctx->octx, "%s", f);
>> +}
>> +
>> +static char *nf_osf_strchr(char *ptr, char c)
>> +{
>> + char *tmp;
>> +
>> + tmp = strchr(ptr, c);
>> + if (tmp)
>> + *tmp = '\0';
>> +
>> + while (tmp && tmp + 1 && isspace(*(tmp + 1)))
>> + tmp++;
>> +
>> + return tmp;
>> +}
>> +
>> +static void nf_osf_parse_opt(struct nf_osf_opt *opt, __u16 *optnum,
>char *obuf, int olen)
>> +{
>> + int i, op;
>> + char *ptr, wc;
>> + unsigned long val;
>> +
>> + ptr = &obuf[0];
>> + i = 0;
>> + while (ptr != NULL && i < olen && *ptr != 0) {
>> + val = 0;
>> + op = 0;
>> + wc = OSF_WSS_PLAIN;
>> + switch (obuf[i]) {
>> + case 'N':
>> + op = OSFOPT_NOP;
>> + ptr = nf_osf_strchr(&obuf[i], OPTDEL);
>> + if (ptr) {
>> + *ptr = '\0';
>> + ptr++;
>> + i += (int)(ptr - &obuf[i]);
>> + } else
>> + i++;
>> + break;
>> + case 'S':
>> + op = OSFOPT_SACKP;
>> + ptr = nf_osf_strchr(&obuf[i], OPTDEL);
>> + if (ptr) {
>> + *ptr = '\0';
>> + ptr++;
>> + i += (int)(ptr - &obuf[i]);
>> + } else
>> + i++;
>> + break;
>> + case 'T':
>> + op = OSFOPT_TS;
>> + ptr = nf_osf_strchr(&obuf[i], OPTDEL);
>> + if (ptr) {
>> + *ptr = '\0';
>> + ptr++;
>> + i += (int)(ptr - &obuf[i]);
>> + } else
>> + i++;
>> + break;
>> + case 'W':
>> + op = OSFOPT_WSO;
>> + ptr = nf_osf_strchr(&obuf[i], OPTDEL);
>> + if (ptr) {
>> + switch (obuf[i + 1]) {
>> + case '%':
>> + wc = OSF_WSS_MODULO;
>> + break;
>> + case 'S':
>> + wc = OSF_WSS_MSS;
>> + break;
>> + case 'T':
>> + wc = OSF_WSS_MTU;
>> + break;
>> + default:
>> + wc = OSF_WSS_PLAIN;
>> + break;
>> + }
>> +
>> + *ptr = '\0';
>> + ptr++;
>> + if (wc)
>> + val = strtoul(&obuf[i + 2], NULL, 10);
>> + else
>> + val = strtoul(&obuf[i + 1], NULL, 10);
>> + i += (int)(ptr - &obuf[i]);
>> +
>> + } else
>> + i++;
>> + break;
>> + case 'M':
>> + op = OSFOPT_MSS;
>> + ptr = nf_osf_strchr(&obuf[i], OPTDEL);
>> + if (ptr) {
>> + if (obuf[i + 1] == '%')
>> + wc = OSF_WSS_MODULO;
>> + *ptr = '\0';
>> + ptr++;
>> + if (wc)
>> + val = strtoul(&obuf[i + 2], NULL, 10);
>> + else
>> + val = strtoul(&obuf[i + 1], NULL, 10);
>> + i += (int)(ptr - &obuf[i]);
>> + } else
>> + i++;
>> + break;
>> + case 'E':
>> + op = OSFOPT_EOL;
>> + ptr = nf_osf_strchr(&obuf[i], OPTDEL);
>> + if (ptr) {
>> + *ptr = '\0';
>> + ptr++;
>> + i += (int)(ptr - &obuf[i]);
>> + } else
>> + i++;
>> + break;
>> + default:
>> + op = OSFOPT_EMPTY;
>> + ptr = nf_osf_strchr(&obuf[i], OPTDEL);
>> + if (ptr) {
>> + ptr++;
>> + i += (int)(ptr - &obuf[i]);
>> + } else
>> + i++;
>> + break;
>> + }
>> +
>> + if (op != OSFOPT_EMPTY) {
>> + opt[*optnum].kind = IANA_opts[op].kind;
>> + opt[*optnum].length = IANA_opts[op].length;
>> + opt[*optnum].wc.wc = wc;
>> + opt[*optnum].wc.val = val;
>> + (*optnum)++;
>> + }
>> + }
>> +}
>> +
>> +static int osf_load_line(char *buffer, int len, int del, struct
>mnl_socket *nl,
>> + struct netlink_ctx *ctx)
>> +{
>> + int i, cnt = 0;
>> + char obuf[MAXOPTSTRLEN];
>> + struct nf_osf_user_finger f;
>> + char *pbeg, *pend;
>> + struct nlmsghdr *nlh;
>> + struct nfgenmsg *nfg;
>> + char buf[MNL_SOCKET_BUFFER_SIZE];
>> +
>> + memset(&f, 0, sizeof(struct nf_osf_user_finger));
>> +
>> + uloga("Loading '%s'.\n", ctx, buffer);
>> +
>> + for (i = 0; i < len && buffer[i] != '\0'; ++i) {
>> + if (buffer[i] == ':')
>> + cnt++;
>> + }
>> +
>> + if (cnt != 8) {
>> + uloga("Wrong input line '%s': cnt: %d, must be 8, i: %d, must be
>%d.\n", ctx, buffer, cnt, i, len);
>> + return -EINVAL;
>> + }
>> +
>> + memset(obuf, 0, sizeof(obuf));
>> +
>> + pbeg = buffer;
>> + pend = nf_osf_strchr(pbeg, OSFPDEL);
>> + if (pend) {
>> + *pend = '\0';
>> + if (pbeg[0] == 'S') {
>> + f.wss.wc = OSF_WSS_MSS;
>> + if (pbeg[1] == '%')
>> + f.wss.val = strtoul(&pbeg[2], NULL, 10);
>> + else if (pbeg[1] == '*')
>> + f.wss.val = 0;
>> + else
>> + f.wss.val = strtoul(&pbeg[1], NULL, 10);
>> + } else if (pbeg[0] == 'T') {
>> + f.wss.wc = OSF_WSS_MTU;
>> + if (pbeg[1] == '%')
>> + f.wss.val = strtoul(&pbeg[2], NULL, 10);
>> + else if (pbeg[1] == '*')
>> + f.wss.val = 0;
>> + else
>> + f.wss.val = strtoul(&pbeg[1], NULL, 10);
>> + } else if (pbeg[0] == '%') {
>> + f.wss.wc = OSF_WSS_MODULO;
>> + f.wss.val = strtoul(&pbeg[1], NULL, 10);
>> + } else if (isdigit(pbeg[0])) {
>> + f.wss.wc = OSF_WSS_PLAIN;
>> + f.wss.val = strtoul(&pbeg[0], NULL, 10);
>> + }
>> +
>> + pbeg = pend + 1;
>> + }
>> + pend = nf_osf_strchr(pbeg, OSFPDEL);
>> + if (pend) {
>> + *pend = '\0';
>> + f.ttl = strtoul(pbeg, NULL, 10);
>> + pbeg = pend + 1;
>> + }
>> + pend = nf_osf_strchr(pbeg, OSFPDEL);
>> + if (pend) {
>> + *pend = '\0';
>> + f.df = strtoul(pbeg, NULL, 10);
>> + pbeg = pend + 1;
>> + }
>> + pend = nf_osf_strchr(pbeg, OSFPDEL);
>> + if (pend) {
>> + *pend = '\0';
>> + f.ss = strtoul(pbeg, NULL, 10);
>> + pbeg = pend + 1;
>> + }
>> +
>> + pend = nf_osf_strchr(pbeg, OSFPDEL);
>> + if (pend) {
>> + *pend = '\0';
>> + cnt = snprintf(obuf, sizeof(obuf), "%s,", pbeg);
>> + pbeg = pend + 1;
>> + }
>> +
>> + pend = nf_osf_strchr(pbeg, OSFPDEL);
>> + if (pend) {
>> + *pend = '\0';
>> + if (pbeg[0] == '@' || pbeg[0] == '*')
>> + cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg + 1);
>> + else
>> + cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg);
>> + pbeg = pend + 1;
>> + }
>> +
>> + pend = nf_osf_strchr(pbeg, OSFPDEL);
>> + if (pend) {
>> + *pend = '\0';
>> + cnt = snprintf(f.version, sizeof(f.version), "%s", pbeg);
>> + pbeg = pend + 1;
>> + }
>> +
>> + pend = nf_osf_strchr(pbeg, OSFPDEL);
>> + if (pend) {
>> + *pend = '\0';
>> + cnt =
>> + snprintf(f.subtype, sizeof(f.subtype), "%s", pbeg);
>> + pbeg = pend + 1;
>> + }
>> +
>> + nf_osf_parse_opt(f.opt, &f.opt_num, obuf, sizeof(obuf));
>> +
>> + memset(buf, 0, sizeof(buf));
>> +
>> + if (del) {
>> + nlh = nftnl_nlmsg_build_hdr(buf, (NFNL_SUBSYS_OSF << 8) |
>> + OSF_MSG_REMOVE, AF_UNSPEC,
>> + NLM_F_REQUEST | NLM_F_ACK,
>> + ctx->seqnum);
>> +
>> + nfg = mnl_nlmsg_put_extra_header(nlh, sizeof(*nfg));
>> + nfg->nfgen_family = AF_UNSPEC;
>> + nfg->version = NFNETLINK_V0;
>> + nfg->res_id = 0;
>> + } else {
>> + nlh = nftnl_nlmsg_build_hdr(buf, (NFNL_SUBSYS_OSF << 8) |
>> + OSF_MSG_ADD, AF_UNSPEC,
>> + NLM_F_REQUEST | NLM_F_CREATE |
>> + NLM_F_ACK, ctx->seqnum);
>> +
>> + nfg = mnl_nlmsg_put_extra_header(nlh, sizeof(*nfg));
>> + nfg->nfgen_family = AF_UNSPEC;
>> + nfg->version = NFNETLINK_V0;
>> + nfg->res_id = 0;
>> +
>> + mnl_attr_put(nlh, OSF_ATTR_FINGER, sizeof(struct
>nf_osf_user_finger), &f);
>> + }
>> +
>> + return nft_mnl_talk(ctx, nlh, nlh->nlmsg_len, 0, NULL);
>> +}
>> +
>> +static int osf_load_entries(int del, struct mnl_socket *nl,
>> + struct netlink_ctx *ctx)
>> +{
>> + FILE *inf;
>> + int err = 0;
>> + char buf[1024];
>> +
>> + inf = fopen(OS_SIGNATURES, "r");
>> + if (!inf) {
>> + uloga("Failed to open file '%s'", ctx, OS_SIGNATURES);
>> + return -1;
>> + }
>> +
>> + while(fgets(buf, sizeof(buf), inf)) {
>> + int len;
>> +
>> + if (buf[0] == '#' || buf[0] == '\n' || buf[0] == '\r')
>> + continue;
>> +
>> + len = strlen(buf) - 1;
>> +
>> + if (len <= 0)
>> + continue;
>> +
>> + buf[len] = '\0';
>> +
>> + err = osf_load_line(buf, len, del, nl, ctx);
>> + if (err)
>> + break;
>> +
>> + memset(buf, 0, sizeof(buf));
>> + }
>> +
>> + fclose(inf);
>> + return err;
>> +}
>> +
>> +int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx, int del)
>> +{
>> + int err;
>> + struct mnl_socket *nl;
>> +
>> + nl = mnl_socket_open(NETLINK_NETFILTER);
>> + if (nl == NULL) {
>> + err = -EINVAL;
>> + uloga("Failed to open mnl socket", ctx);
>> + goto err_out_exit;
>> + }
>> +
>> + if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0) {
>> + err = -EINVAL;
>> + uloga("Failed to bind mnl socket", ctx);
>> + goto err_out_exit;
>> + }
>> +
>> +#ifndef NFNL_SUBSYS_OSF
>> +#define NFNL_SUBSYS_OSF 5
>> +#endif
>
>Remove this #ifdef, not needed. NFNL_SUBSYS_OSF is already defined in
>nfnetlink_osf.h
>
>> + err = osf_load_entries(del, nl, ctx);
>> + if (err < 0)
>> + goto err_out_close;
>> +
>> + return 0;
>> +
>> +err_out_close:
>> + mnl_socket_close(nl);
>> +err_out_exit:
>> + return err;
>> +}
>> diff --git a/src/osf.c b/src/osf.c
>> index 131d54e..210bfbe 100644
>> --- a/src/osf.c
>> +++ b/src/osf.c
>> @@ -3,6 +3,7 @@
>> #include <utils.h>
>> #include <string.h>
>> #include <osf.h>
>> +#include <nfnl_osf.h>
>>
>> static void osf_expr_print(const struct expr *expr, struct
>output_ctx *octx)
>> {
>> @@ -26,6 +27,7 @@ struct expr *osf_expr_alloc(const struct location
>*loc)
>> const struct datatype *type = &string_type;
>> struct expr *expr;
>>
>> + osf_init = true;
>> expr = expr_alloc(loc, &osf_expr_ops, type,
>> BYTEORDER_HOST_ENDIAN, len);
>>
>> diff --git a/src/rule.c b/src/rule.c
>> index 7a7ac73..7421ffc 100644
>> --- a/src/rule.c
>> +++ b/src/rule.c
>> @@ -22,6 +22,7 @@
>> #include <netdb.h>
>> #include <netlink.h>
>> #include <json.h>
>> +#include <nfnl_osf.h>
>>
>> #include <libnftnl/common.h>
>> #include <libnftnl/ruleset.h>
>> @@ -2135,6 +2136,9 @@ int do_command(struct netlink_ctx *ctx, struct
>cmd *cmd)
>> default:
>> BUG("invalid command object type %u\n", cmd->obj);
>> }
>> +
>> + if (osf_init)
>> + nfnl_osf_load_fingerprints(ctx, 0);
>
>You have to update do_command() to replace all:
>
> case blah:
> return ...;
>
>to
> case blah:
> err = ...;
> break;
>
>Otherwise this code is never exercised.
>
>When testing this, make sure nfnl_osf_load_fingerprints() is called.
>
>You can use:
>
> nft --debug=netlink add rule x y osf name "linux"
>
>to see if you trigger uloga() calls for debugging so you make sure you
>properly test your patches.
>
>Thanks.
