On Fri, Jul 27, 2018 at 04:15:29PM +0200, Máté Eckl wrote:
> On Mon, Jul 23, 2018 at 09:28:27AM +0200, Máté Eckl wrote:
> > On Fri, Jul 20, 2018 at 03:28:31PM +0200, Pablo Neira Ayuso wrote:
> > > Hi Mate,
> > >
> > > A few comestic on the _init path, and one concern of probably missing
> > > sanity check, also from the _init path see below.
> > >
> > > On Fri, Jul 20, 2018 at 09:34:14AM +0200, Máté Eckl wrote:
>
> [...]
>
> > > > +static int nft_tproxy_init(const struct nft_ctx *ctx,
> > > > + const struct nft_expr *expr,
> > > > + const struct nlattr * const tb[])
> > > > +{
> > > > + struct nft_tproxy *priv = nft_expr_priv(expr);
> > > > + unsigned int alen = 0;
> > > > + int err;
> > >
> > > Probably check here:
> > >
> > > if (!tb[NFTA_TPROXY_FAMILY])
> > > return -EINVAL;
> > >
> > > family = ...;
> > >
> > > So we can reuse the switch() below...
> > >
> > > > +
> > > > + switch (ctx->family) {
> > > > + case NFPROTO_IPV4:
> > > > +#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
> > > > + case NFPROTO_IPV6:
> > > > +#endif
> > > > + case NFPROTO_INET:
> > >
> > > I think you have to update this to NFPROTO_UNSPEC.
> >
> > No because this is the ctx->family, not the priv->family. This has to be done so
> > that a tproxy statement cannot be added to a netdev (or arp, etc.) table.
> >
> > >
> > > > + break;
> > > > + default:
> > > > + return -EOPNOTSUPP;
> > > > + }
> > > > +
> > > > + if (!tb[NFTA_TPROXY_FAMILY] ||
> > > > + (!tb[NFTA_TPROXY_REG_ADDR] && !tb[NFTA_TPROXY_REG_PORT]))
> > > > + return -EINVAL;
> > > > +
> > > > + priv->family = ntohl(nla_get_be32(tb[NFTA_TPROXY_FAMILY]));
> > > > + switch (ctx->family) {
> > >
> > > To do what we're doing this in this switch() ...
> > >
> > > > + case NFPROTO_IPV4:
> > > > + if (priv->family != NFPROTO_IPV4)
> > > > + return -EINVAL;
> > > > + break;
> > > > + case NFPROTO_IPV6:
> > > > + if (priv->family != NFPROTO_IPV6)
> > > > + return -EINVAL;
> > > > + break;
> > > > + }
> > > > +
> > > > + /* Address is specified but the rule family is not set accordingly */
> > > > + if (priv->family == NFPROTO_UNSPEC && tb[NFTA_TPROXY_REG_ADDR])
> > > > + return -EINVAL;
> > >
> > > With the change I'm proposing above, you can do all these attribute
> > > sanity checks at the very beginning of the function.
> >
> > I see your point. See later.
> >
> > >
> > > > +
> > > > + switch (priv->family) {
> > > > + case NFPROTO_IPV4:
> > >
> > > I'm missing a check like:
> > >
> > > if (priv->family != NFPROTO_UNSPEC &&
> > > ctx->family != priv->family)
> > > return -EINVAL;
> > >
> > > somewhere.
> >
> > This switch basically does the same in a reverse logic, doesn't it?
> >
> > switch (ctx->family) {
> > case NFPROTO_IPV4:
> > if (priv->family != NFPROTO_IPV4)
> > return -EINVAL;
> > break;
> > case NFPROTO_IPV6:
> > if (priv->family != NFPROTO_IPV6)
> > return -EINVAL;
> > break;
> > }
> >
> > >
> > > So we don't allow crazy things like, priv->family == NFPROTO_IPV6 from
> > > ctx->family == NFPROTO_IPV4... I may be wrong but I think it's still
> > > possible with this code.
> >
> > The switch above rejects this with -EINVAL.
> >
> > How about this:
> >
> > static int nft_tproxy_init(const struct nft_ctx *ctx,
> > const struct nft_expr *expr,
> > const struct nlattr * const tb[])
> > {
> > struct nft_tproxy *priv = nft_expr_priv(expr);
> > unsigned int alen = 0;
> > int err;
> >
> > if (!tb[NFTA_TPROXY_FAMILY] ||
> > (!tb[NFTA_TPROXY_REG_ADDR] && !tb[NFTA_TPROXY_REG_PORT]))
> > return -EINVAL;
> >
> > priv->family = ntohl(nla_get_be32(tb[NFTA_TPROXY_FAMILY]));
> >
> > switch (ctx->family) {
> > case NFPROTO_IPV4:
> > if (priv->family != NFPROTO_IPV4)
> > return -EINVAL;
> > break;
> > #if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
> > case NFPROTO_IPV6:
> > if (priv->family != NFPROTO_IPV6)
> > return -EINVAL;
> > break;
> > #endif
> > case NFPROTO_INET:
> > break;
> > default:
> > return -EOPNOTSUPP;
> > }
> >
> > /* Address is specified but the rule family is not set accordingly */
> > if (priv->family == NFPROTO_UNSPEC && tb[NFTA_TPROXY_REG_ADDR])
> > return -EINVAL;
> > [...]
> >
> > I think this addressess all of your concerns.
>
> What do you think? If you are satisfied, I'll send in a new version.
Go ahead send a new version if you need to, otherwise I'll take this
v4. Let me know, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
