Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall

[Daisuke Nojiri]

Hi, Blue,

> I missed somehow 1/3, so I'll comment to this one.

I updated 1/3 and pasted it below. The rest of the points you raised will be addressed in 2/3 and 3/3.

> With the TCP firewall in mind, I'd use a more general syntax,

> something like "deny=proto:udp". More complete rules would need
> denying a block inside allowed block, so address part should be used
> here (or "all" for deny all).

How about drop=udp|tcp|all? drop switch makes all packets denied and allow rules will work as exceptions.

This way, we can avoid conflicts between denys and allows.

> Don't use global variables. It's possible to have several interfaces,
> each on a different user mode stack, so each interface may have
> different rules. There's struct Slirp defined in slirp.h, please put
> these there.

Addressed as suggested. Thanks, Blue.

Dai

----------- Subject: [Qemu-devel] [Patch 1/3] Slirp Reverse UDP Firewall ------------

This patch series adds a simple reverse UDP firewall functionality to Slirp.

The series consists of three patches. Each adds one -net user option:

    1. drop=udp|all - enables the firewall

    2. droplog=FILE - sets the drop log filename

    3. allow=PROTO:ADDR:PORT - adds an allow rule

  e.g.) $ qemu -net user,drop=udp,droplog=qemu.drop,allow=udp:10.0.2.3:53

All UDP packets except ones allowed by allow rules will be dropped. The source

and the destination of the dropped packets are logged in the file specified by FILE.

PORT can be a single number (e.g. 53) or a range (e.g. [80-81]). ADDR can be a

single address (e.g. 1.2.3.4) or a range (e.g. 1.2.3.4/24). If ADDR is ommitted,

all addresses match the rule. If PROTO is omitted, all protocols match the rule.

TCP support will follow in another patch series.

Signed-off-by: Daisuke Nojiri <address@hidden>

diff --git a/net.c b/net.c

index 8d6a555..2742741 100644

--- a/net.c

+++ b/net.c

@@ -925,6 +925,10 @@ static const struct {

                 .name = "guestfwd",

                 .type = QEMU_OPT_STRING,

                 .help = "IP address and port to forward guest TCP connections",

+            }, {

+                .name = "drop",

+                .type = QEMU_OPT_STRING,

+                .help = "Enable the simple reverse firewall",

             },

             { /* end of list */ }

         },

diff --git a/net/slirp.c b/net/slirp.c

index b41c60a..bd83700 100644

--- a/net/slirp.c

+++ b/net/slirp.c

@@ -141,7 +141,7 @@ static int net_slirp_init(VLANState *vlan, const char *model,

                           const char *vhostname, const char *tftp_export,

                           const char *bootfile, const char *vdhcp_start,

                           const char *vnameserver, const char *smb_export,

-                          const char *vsmbserver)

+                          const char *vsmbserver, unsigned char drop)

 {

     /* default settings according to historic slirp */

     struct in_addr net  = { .s_addr = htonl(0x0a000200) }; /* 10.0.2.0 */

@@ -246,7 +246,7 @@ static int net_slirp_init(VLANState *vlan, const char *model,

     s = DO_UPCAST(SlirpState, nc, nc);

 

     s->slirp = slirp_init(restricted, net, mask, host, vhostname,

-                          tftp_export, bootfile, dhcp, dns, s);

+                          tftp_export, bootfile, dhcp, dns, drop, s);

     QTAILQ_INSERT_TAIL(&slirp_stacks, s, entry);

 

     for (config = slirp_configs; config; config = config->next) {

@@ -693,6 +693,7 @@ int net_init_slirp(QemuOpts *opts,

     char *vnet = NULL;

     int restricted = 0;

     int ret;

+    unsigned char drop = 0;

 

     vhost       = qemu_opt_get(opts, "host");

     vhostname   = qemu_opt_get(opts, "hostname");

@@ -726,11 +727,25 @@ int net_init_slirp(QemuOpts *opts,

         restricted = 1;

     }

 

+    if (qemu_opt_get(opts, "drop")) {

+        switch (qemu_opt_get(opts, "drop")[0]) {

+        case 'u':

+            drop &= SLIRP_DROP_UDP;

+            break;

+        case 'a':

+            drop &= SLIRP_DROP_UDP;

+            break;

+        default:

+            error_report("Unknown protocol name given to drop option");

+            return -1;

+        }

+    }

+

     qemu_opt_foreach(opts, net_init_slirp_configs, NULL, 0);

 

     ret = net_slirp_init(vlan, "user", name, restricted, vnet, vhost,

                          vhostname, tftp_export, bootfile, vdhcp_start,

-                         vnamesrv, smb_export, vsmbsrv);

+                         vnamesrv, smb_export, vsmbsrv, drop);

 

     while (slirp_configs) {

         config = slirp_configs;

@@ -768,4 +783,3 @@ int net_slirp_parse_legacy(QemuOptsList *opts_list, const char *optarg, int *ret

 

     return 1;

 }

-

diff --git a/qemu-options.hx b/qemu-options.hx

index ef60730..ef3e726 100644

--- a/qemu-options.hx

+++ b/qemu-options.hx

@@ -1067,7 +1067,7 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,

 #ifdef CONFIG_SLIRP

     "-net user[,vlan=n][,name=str][,net=addr[/mask]][,host=addr][,restrict=y|n]\n"

     "         [,hostname=host][,dhcpstart=addr][,dns=addr][,tftp=dir][,bootfile=f]\n"

-    "         [,hostfwd=rule][,guestfwd=rule]"

+    "         [,hostfwd=rule][,guestfwd=rule][,drop=udp|all]"

 #ifndef _WIN32

                                              "[,smb=dir[,smbserver=addr]]\n"

 #endif

diff --git a/slirp/libslirp.h b/slirp/libslirp.h

index 67c70e3..5778bf4 100644

--- a/slirp/libslirp.h

+++ b/slirp/libslirp.h

@@ -14,7 +14,8 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,

                   struct in_addr vnetmask, struct in_addr vhost,

                   const char *vhostname, const char *tftp_path,

                   const char *bootfile, struct in_addr vdhcp_start,

-                  struct in_addr vnameserver, void *opaque);

+                  struct in_addr vnameserver, unsigned char drop,

+                  void *opaque);

 void slirp_cleanup(Slirp *slirp);

 

 void slirp_select_fill(int *pnfds,

@@ -44,6 +45,14 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr guest_addr,

 size_t slirp_socket_can_recv(Slirp *slirp, struct in_addr guest_addr,

                              int guest_port);

 

+/* Reverse Firewall */

+#define SLIRP_DROP_UDP    1

+

+int slirp_should_drop(Slirp *slirp,

+                      struct in_addr dst_addr,

+                      unsigned short dst_port,

+                      u_int8_t proto);

+

 #else /* !CONFIG_SLIRP */

 

 static inline void slirp_select_fill(int *pnfds, fd_set *readfds,

diff --git a/slirp/slirp.c b/slirp/slirp.c

index 1593be1..298ccb4 100644

--- a/slirp/slirp.c

+++ b/slirp/slirp.c

@@ -200,7 +200,7 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,

                   struct in_addr vnetmask, struct in_addr vhost,

                   const char *vhostname, const char *tftp_path,

                   const char *bootfile, struct in_addr vdhcp_start,

-                  struct in_addr vnameserver, void *opaque)

+                  struct in_addr vnameserver, unsigned char drop, void *opaque)

 {

     Slirp *slirp = qemu_mallocz(sizeof(Slirp));

 

@@ -230,6 +230,8 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,

     slirp->vdhcp_startaddr = vdhcp_start;

     slirp->vnameserver_addr = vnameserver;

 

+    slirp->drop = drop;

+

     slirp->opaque = opaque;

 

     register_savevm(NULL, "slirp", 0, 3,

@@ -1111,3 +1113,20 @@ static int slirp_state_load(QEMUFile *f, void *opaque, int version_id)

 

     return 0;

 }

+

+int slirp_should_drop(Slirp *slirp,

+                      struct in_addr dst_addr,

+                      unsigned short dst_port,

+                      u_int8_t proto) {

+    switch (proto) {

+    case IPPROTO_UDP:

+        if (!(slirp->drop & SLIRP_DROP_UDP)) {

+            return 0;

+        }

+        break;

+    default:

+        return 0;   /* unrecognized protocol. default pass. */

+    }

+

+    return 1;

+}

diff --git a/slirp/slirp.h b/slirp/slirp.h

index 954289a..bfea30d 100644

--- a/slirp/slirp.h

+++ b/slirp/slirp.h

@@ -180,6 +180,9 @@ struct Slirp {

     struct in_addr vdhcp_startaddr;

     struct in_addr vnameserver_addr;

 

+    /* Reverse Firewall configuration */

+    unsigned char drop;

+

     /* ARP cache for the guest IP addresses (XXX: allow many entries) */

     uint8_t client_ethaddr[6];

 

diff --git a/slirp/udp.c b/slirp/udp.c

index 02b3793..95c4af0 100644

--- a/slirp/udp.c

+++ b/slirp/udp.c

@@ -98,6 +98,17 @@ udp_input(register struct mbuf *m, int iphlen)

  ip->ip_len = len;

  }

 

+        /*

+         * User mode firewall

+         */

+        if (slirp_should_drop(

+            slirp, ip->ip_dst, uh->uh_dport, IPPROTO_UDP)) {

+            /* DROP */

+            goto bad;

+        } else {

+            /* PASS */

+        }

+

  /*

  * Save a copy of the IP header in case we want restore it

  * for sending an ICMP error message in response.

On Wed, Apr 13, 2011 at 1:52 PM, Blue Swirl <address@hidden> wrote:

On Tue, Apr 12, 2011 at 7:19 PM, Daisuke Nojiri <address@hidden> wrote:
> This patch adds: -drop-udp, -allow-udp ADDR:PORT, -drop-log FILE
>   e.g.) $ qemu -net user -drop-log qemu.drop -drop-udp -allow-udp
> 10.0.2.3:53
> -drop-udp enables usermode firewall for out-going UDP packats from a guest.
> All UDP packets except ones allowed by -allow-udp will be dropped. Dropped
> packets are logged in the file specified by FILE. PORT can be a single
> number
> (e.g. 53) or a range (e.g. [80-81]). If ADDR is ommitted, all addresses
> match
> the rule.
> Signed-off-by: Daisuke Nojiri <address@hidden>

I missed somehow 1/3, so I'll comment to this one.

> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1119,6 +1119,24 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>      "vde|"
>  #endif
>      "socket],id=str[,option][,option][,...]\n", QEMU_ARCH_ALL)
> +
> +DEF("drop-udp", 0, QEMU_OPTION_drop_udp,

With the TCP firewall in mind, I'd use a more general syntax,
something like "deny=proto:udp". More complete rules would need
denying a block inside allowed block, so address part should be used
here (or "all" for deny all).

> +"-drop-udp\tDrop UDP packets by usermode firewall\n",
> +QEMU_ARCH_ALL)
> +
> +DEF("allow-udp", HAS_ARG, QEMU_OPTION_allow_udp,

Likewise, "allow=proto:udp:addr:port". Then, if "udp" is left out, the
rule should apply to all protocols. The address part should allow for
range specifiers, like "10.0.0.0/24". Colon is not so great choice
when thinking also of IPv6 addresses.

> +    "-allow-udp addr:port\n"
> +    "                Add an allowed rule for -drop-udp. If destination
> matches\n"
> +    "                the rule, the packet won't be dropped. 'port' can be a
> single\n"
> +    "                number (e.g. 53) or a range (e.g. [80-81]. If 'addr'
> is omitted,
> +    "                all addresses match.\n",
> +    QEMU_ARCH_ALL)
> +
> +DEF("drop-log", HAS_ARG, QEMU_OPTION_drop_log,
> +    "-drop-log file\n"
> +    "                Set usermode firewall log filename to 'file'.\n",
> +    QEMU_ARCH_ALL)
> +
>  STEXI
> address@hidden -net nic[,address@hidden,address@hidden,address@hidden
> [,address@hidden,address@hidden,address@hidden
> address@hidden -net
> diff --git a/slirp/libslirp.h b/slirp/libslirp.h
> index 67c70e3..1ce5d68 100644
> --- a/slirp/libslirp.h
> +++ b/slirp/libslirp.h
> @@ -44,6 +44,15 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr
> guest_addr,
>  size_t slirp_socket_can_recv(Slirp *slirp, struct in_addr guest_addr,
>                               int guest_port);
>
> +/* Usermode firewall functions */
> +void slirp_enable_drop_udp(void);
> +void slirp_set_drop_log_fd(FILE *fd);
> +void slirp_add_allow(const char *optarg, u_int8_t proto);
> +int slirp_drop_log(const char *format, ...);
> +int slirp_should_drop(unsigned long dst_addr,
> +                      unsigned short dst_port,
> +                      u_int8_t proto);
> +
>  #else /* !CONFIG_SLIRP */
>
>  static inline void slirp_select_fill(int *pnfds, fd_set *readfds,
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 1593be1..d321316 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -1111,3 +1111,192 @@ static int slirp_state_load(QEMUFile *f, void
> *opaque, int version_id)
>
>      return 0;
>  }
> +
> +/*
> + * Allow rule for the usermode firewall
> + */
> +struct ufw_allowed {
> +    struct ufw_allowed *next;

Please use QLIST macros.

> +    unsigned long dst_addr;

This is not IPv6 safe, though Slirp does not implement IPv6. I'd still
use the proper address type.

> +    /* Port range. For a single port, dst_lport = dst_hport. */
> +    unsigned short dst_lport;   /* in host byte order */
> +    unsigned short dst_hport;   /* in host byte order */
> +};
> +
> +/*
> + * Global variables for the usermode firewall
> + */
> +static int drop_udp = 0;
> +static FILE *drop_log_fd = NULL;
> +static struct ufw_allowed *fw_allowed_udp = NULL;

Don't use global variables. It's possible to have several interfaces,
each on a different user mode stack, so each interface may have
different rules. There's struct Slirp defined in slirp.h, please put
these there.

> +
> +void slirp_enable_drop_udp(void)
> +{
> +    drop_udp = 1;
> +}
> +
> +void slirp_set_drop_log_fd(FILE *fd)
> +{
> +    drop_log_fd = fd;
> +}
> +
> +int slirp_should_drop(unsigned long dst_addr,
> +                      unsigned short dst_port,
> +                      u_int8_t proto) {
> +    struct ufw_allowed *fwa = NULL;
> +    unsigned short dport;   /* host byte order */
> +
> +    switch (proto) {
> +    case IPPROTO_UDP:
> +        if (drop_udp == 0) {
> +            return 0;
> +        } else {
> +            fwa = fw_allowed_udp;
> +        }
> +        break;
> +    case IPPROTO_TCP:
> +    default:
> +        return 1;   /* unrecognized protocol. default drop. */
> +    }
> +
> +    /* Find matching allow rule. 0 works as a wildcard for address. */
> +    for (; fwa; fwa = fwa->next) {
> +        dport = ntohs(dst_port);
> +        if ((fwa->dst_lport <= dport) && (dport <= fwa->dst_hport)) {
> +            /* allow any destination if 0 */
> +            if (fwa->dst_addr == 0 || fwa->dst_addr == dst_addr) {
> +                return 0;
> +            }
> +        }
> +    }
> +    return 1;
> +}
> +
> +/*
> + * Register an allow rule for user mode firewall
> + */
> +void slirp_add_allow_internal(unsigned long dst_addr,

static? I'm not sure which functions are really needed outside of this file.

> +                              unsigned short dst_lport,
> +                              unsigned short dst_hport,
> +                              u_int8_t proto) {
> +    struct ufw_allowed *fwa;
> +
> +    fwa = (struct ufw_allowed *)malloc(sizeof(struct ufw_allowed));

The cast is useless in C. Please use qemu_malloc, then the following
check is not needed.

> +    if (!fwa) {
> +        DEBUG_MISC((dfd, "Unabled to create a new firewall rule "
> +                    "due to malloc failure\n"));
> +        exit(-1);
> +    }
> +
> +    fwa->dst_addr = dst_addr;
> +    fwa->dst_lport = dst_lport;
> +    fwa->dst_hport = dst_hport;
> +
> +    switch (proto) {
> +    case IPPROTO_UDP:
> +        fwa->next = fw_allowed_udp;
> +        fw_allowed_udp = fwa;
> +        break;
> +    case IPPROTO_TCP:
> +    default:
> +        free(fwa);

This should then become qemu_free().

> +        return;   /* unrecognized protocol */
> +    }
> +}
> +
> +/*
> + * Parse a null-terminated string specifying a network port or port range
> (e.g.
> + * "[1024-65535]"). In case of a single port, lport and hport are the same.
> + * Returns 0 on success and -1 on error.
> + */
> +int parse_port_range(const char *str,
> +                     unsigned short *lport,
> +                     unsigned short *hport) {
> +    unsigned int low = 0, high = 0;
> +    char *p, *arg = strdup(str);

qemu_strdup(), but I'd avoid that by using strtol().

> +
> +    p = rindex(arg, ']');
> +    if ((*arg == '[') & (p != NULL)) {
> +        p = arg + 1;   /* skip '[' */
> +        low  = atoi(strsep(&p, "-"));
> +        high = atoi(p);
> +    } else {
> +        low = atoi(arg);
> +        high = low;
> +    }
> +
> +    free(arg);
> +
> +    if ((low > 0) && (high > 0) && (low <= high) && (high < 65536)) {
> +        *lport = low;
> +        *hport = high;
> +        return 0;
> +    }
> +
> +    return -1;
> +}
> +
> +/*
> + * Add allow rules for the usermode firewall.
> + */
> +void slirp_add_allow(const char *optarg, u_int8_t proto)
> +{
> +    /*
> +     * we expect the following format:
> +     *  dst_addr:dst_port OR dst_addr:[dst_lport-dst_hport]
> +     */
> +    char *argument = strdup(optarg), *p = argument;
> +    char *dst_addr_str, *dst_port_str;
> +    struct in_addr dst_addr;
> +    unsigned short dst_lport, dst_hport;
> +
> +    dst_addr_str = strsep(&p, ":");
> +    dst_port_str = p;
> +
> +    if (dst_addr_str == NULL || dst_port_str == NULL) {
> +        fprintf(stderr,
> +                "Invalid argument %s for -allow. We expect "
> +                "dst_addr:dst_port or dst_addr:[dst_lport-dst_hport]\n",
> +                optarg);
> +        exit(1);
> +    }
> +
> +    /* handling ":port" notation (when IP address is omitted entirely). */
> +    if (*dst_addr_str == '\0') {
> +        dst_addr.s_addr = 0;
> +    }
> +    /* inet_aton returns 0 on failure. */
> +    else if (inet_aton(dst_addr_str, &dst_addr) == 0) {

else belongs to the line with }.

> +        fprintf(stderr, "Invalid destination IP address: %s\n",
> dst_addr_str);
> +        exit(1);
> +    }
> +
> +    if (parse_port_range(dst_port_str, &dst_lport, &dst_hport) == -1) {
> +        fprintf(stderr, "Invalid destination port or port range\n");
> +        exit(1);
> +    }
> +
> +    slirp_add_allow_internal(dst_addr.s_addr, dst_lport, dst_hport, proto);
> +
> +    free(argument);
> +}
> +
> +/*
> + * Write to drop-log
> + */
> +int slirp_drop_log(const char *format, ...)
> +{
> +    va_list args;
> +
> +    if (!drop_log_fd) {
> +        return 0;
> +    }
> +
> +    va_start(args, format);
> +    vfprintf(drop_log_fd, format, args);
> +    va_end(args);
> +
> +    fflush(drop_log_fd);
> +
> +    return 1;
> +}
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index 954289a..29ee425 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -299,6 +299,15 @@ int tcp_emu(struct socket *, struct mbuf *);
>  int tcp_ctl(struct socket *);
>  struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
>
> +/* slirp.c */
> +void slirp_add_allow_internal(unsigned long dst_addr,
> +                              unsigned short dst_lport,
> +                              unsigned short dst_hport,
> +                              u_int8_t proto);
> +int parse_port_range(const char *str,
> +                     unsigned short *lport,
> +                     unsigned short *hport);
> +
>  #ifdef USE_PPP
>  #define MIN_MRU MINMRU
>  #define MAX_MRU MAXMRU
> diff --git a/slirp/udp.c b/slirp/udp.c
> index 02b3793..db7e4ca 100644
> --- a/slirp/udp.c
> +++ b/slirp/udp.c
> @@ -67,6 +67,8 @@ udp_input(register struct mbuf *m, int iphlen)
>   DEBUG_ARG("m = %lx", (long)m);
>   DEBUG_ARG("iphlen = %d", iphlen);
>
> +        time_t timestamp = time(NULL);
> +
>   /*
>   * Strip IP options, if any; should skip this,
>   * make available to user, and use on returned packets,
> @@ -98,6 +100,28 @@ udp_input(register struct mbuf *m, int iphlen)
>   ip->ip_len = len;
>   }
>
> +        /*
> +         * User mode firewall
> +         */
> +        if (slirp_should_drop(ip->ip_dst.s_addr, uh->uh_dport,
> IPPROTO_UDP)) {
> +            slirp_drop_log(
> +                "Dropped UDP: src:0x%08x:0x%04hx dst:0x%08x:0x%04hx %ld\n",
> +                ntohl(ip->ip_src.s_addr),
> +                ntohs(uh->uh_sport),
> +                ntohl(ip->ip_dst.s_addr),
> +                ntohs(uh->uh_dport),
> +                timestamp);

Maybe tracepoints could be used instead of logging, though a separate
log may be useful too.

> +            goto bad;
> +        } else {
> +            slirp_drop_log(
> +                "Allowed UDP: src:0x%08x:0x%04hx dst:0x%08x:0x%04hx %ld\n",
> +                ntohl(ip->ip_src.s_addr),
> +                ntohs(uh->uh_sport),
> +                ntohl(ip->ip_dst.s_addr),
> +                ntohs(uh->uh_dport),
> +                timestamp);
> +        }
> +
>   /*
>   * Save a copy of the IP header in case we want restore it
>   * for sending an ICMP error message in response.
> diff --git a/vl.c b/vl.c
> index 68c3b53..b0583e3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2287,6 +2287,24 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  break;
>  #endif
> +            case QEMU_OPTION_drop_udp:
> +                slirp_enable_drop_udp();
> +                break;
> +            case QEMU_OPTION_allow_udp:
> +                slirp_add_allow(optarg, IPPROTO_UDP);
> +                break;
> +            case QEMU_OPTION_drop_log:
> +                {
> +                    FILE *drop_log_fd;
> +                    drop_log_fd = fopen(optarg, "w");
> +
> +                    if (!drop_log_fd) {
> +                        fprintf(stderr, "Cannot open drop log: %s\n",
> optarg);
> +                        exit(1);
> +                    }
> +                    slirp_set_drop_log_fd(drop_log_fd);
> +                }
> +                break;
>              case QEMU_OPTION_bt:
>                  add_device_config(DEV_BT, optarg);
>                  break;
>