On Sat, Jul 28, 2018 at 12:11:15PM +0200, Pablo Neira Ayuso wrote:
> 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.
I'm sending v5 in minutes.
--
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
