[libvirt] [PATCH v2] nwfilter: Validate rule after parsing

[Daniel P. Berrange]

On Wed, Apr 23, 2014 at 10:59:25AM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb at linux.vnet.ibm.com>
> 
> An IP or IPv6 rule with port specification but without protocol
> specification cannot be instantiated by ebtables. The documentation
> points to 'protocol' being required but implementation does not
> enforce it to be given.
> 
> Implement a rule validation function that checks whether the rule is
> valid when it is defined. This for example prevents the definition
> of rules like:
> 
> <ip dstportstart='53'>
> 
> where a protocol attribute would be required for it to be valid and for
> ebtables to be able to instantiate it. A valid rule then is:
> 
> <ip protocol='udp' dstportstart='53'>
> 
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> 
> Changes:
> v1->v2:
>   - fixed access to ipv6 structures
> 
> ---
>  src/conf/nwfilter_conf.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> index f5a75e4..69b1d97 100644
> --- a/src/conf/nwfilter_conf.c
> +++ b/src/conf/nwfilter_conf.c
> @@ -2093,6 +2093,66 @@ virNWFilterRuleDefFixupIPSet(ipHdrDataDefPtr ipHdr)
>      }
>  }
>  
> +
> +/*
> + * virNWFilterRuleValidate
> + *
> + * Perform some basic rule validation to prevent rules from being
> + * defined that cannot be instantiated.
> + */
> +static int
> +virNWFilterRuleValidate(virNWFilterRuleDefPtr rule)
> +{
> +    int ret = 0;
> +    portDataDefPtr portData = NULL;
> +    nwItemDescPtr dataProtocolID;
> +    const char *protocol;
> +
> +    switch (rule->prtclType) {
> +    case VIR_NWFILTER_RULE_PROTOCOL_IP:
> +        portData = &rule->p.ipHdrFilter.portData;
> +        protocol = "IP";
> +        dataProtocolID = &rule->p.ipHdrFilter.ipHdr.dataProtocolID;
> +        /* fall through */
> +    case VIR_NWFILTER_RULE_PROTOCOL_IPV6:
> +        if (portData == NULL) {
> +            portData = &rule->p.ipv6HdrFilter.portData;
> +            protocol = "IPv6";
> +            dataProtocolID = &rule->p.ipv6HdrFilter.ipHdr.dataProtocolID;
> +        }
> +        if (HAS_ENTRY_ITEM(&portData->dataSrcPortStart) ||
> +            HAS_ENTRY_ITEM(&portData->dataDstPortStart) ||
> +            HAS_ENTRY_ITEM(&portData->dataSrcPortEnd) ||
> +            HAS_ENTRY_ITEM(&portData->dataDstPortEnd)) {
> +            if (HAS_ENTRY_ITEM(dataProtocolID)) {
> +                switch (dataProtocolID->u.u8) {
> +                case 6:   /* tcp */
> +                case 17:  /* udp */
> +                case 33:  /* dccp */
> +                case 132: /* sctp */
> +                break;
> +                default:
> +                    ret = -1;
> +                }
> +            } else {
> +                ret = -1;
> +            }
> +            if (ret < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("%s rule with port specification requires "
> +                                 "protocol specification with protocol to be "
> +                                 "either one of tcp(6), udp(17), dccp(33), or "
> +                                 "sctp(132)"), protocol);
> +            }
> +        }
> +    break;
> +    default:
> +    break;

Nitpick, the 'break' statements should be indented 4 more spaces.

> +    }
> +
> +    return ret;
> +}
> +
>  static void
>  virNWFilterRuleDefFixup(virNWFilterRuleDefPtr rule)
>  {
> @@ -2389,6 +2449,8 @@ virNWFilterRuleParse(xmlNodePtr node)
>                                                      virAttr[i].att) < 0) {
>                          goto err_exit;
>                      }
> +                    if (virNWFilterRuleValidate(ret) < 0)
> +                        goto err_exit;
>                      break;
>                  }
>                  if (!found) {

ACK  with the indentation fix. Fine to push in freeze IMHO.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|