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.
[...]
--
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
