[PATCH] src: proto: Add missing packet type

[PATCH] src: proto: Add missing packet type

[Shivani Bhardwaj]

Add missing packet type "invalid" for DCCP.

Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
---
 src/proto.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/proto.c b/src/proto.c
index 0ed98ed..4d049f5 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -443,6 +443,7 @@ static const struct symbol_table dccp_pkttype_tbl = {
 		SYMBOL("reset",		DCCP_PKT_RESET),
 		SYMBOL("sync",		DCCP_PKT_SYNC),
 		SYMBOL("syncack",	DCCP_PKT_SYNCACK),
+		SYMBOL("invalid",	DCCP_PKT_INVALID),
 		SYMBOL_LIST_END
 	},
 };
-- 
1.9.1

Re: [PATCH] src: proto: Add missing packet type

[Florian Westphal]

Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> Add missing packet type "invalid" for DCCP.
> 
> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> ---
>  src/proto.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/proto.c b/src/proto.c
> index 0ed98ed..4d049f5 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -443,6 +443,7 @@ static const struct symbol_table dccp_pkttype_tbl = {
>  		SYMBOL("reset",		DCCP_PKT_RESET),
>  		SYMBOL("sync",		DCCP_PKT_SYNC),
>  		SYMBOL("syncack",	DCCP_PKT_SYNCACK),
> +		SYMBOL("invalid",	DCCP_PKT_INVALID),

I don't think this is a good idea -- when user asks to match
'invalid' then this will check type == DCCP_PKT_INVALID; however
the correct way would be to ask for type '> synack' (i.e.,
outside of the range of types specified).

Re: [PATCH] src: proto: Add missing packet type

[Shivani Bhardwaj]

On Tue, Mar 1, 2016 at 6:47 PM, Florian Westphal <fw@strlen.de> wrote:
> Shivani Bhardwaj <shivanib134@gmail.com> wrote:
>> Add missing packet type "invalid" for DCCP.
>>
>> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
>> ---
>>  src/proto.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/proto.c b/src/proto.c
>> index 0ed98ed..4d049f5 100644
>> --- a/src/proto.c
>> +++ b/src/proto.c
>> @@ -443,6 +443,7 @@ static const struct symbol_table dccp_pkttype_tbl = {
>>               SYMBOL("reset",         DCCP_PKT_RESET),
>>               SYMBOL("sync",          DCCP_PKT_SYNC),
>>               SYMBOL("syncack",       DCCP_PKT_SYNCACK),
>> +             SYMBOL("invalid",       DCCP_PKT_INVALID),
>
> I don't think this is a good idea -- when user asks to match
> 'invalid' then this will check type == DCCP_PKT_INVALID; however
> the correct way would be to ask for type '> synack' (i.e.,
> outside of the range of types specified).

OK. So, should this be removed from the enum dccp_pkt_type as well?

And, if following iptables rules, one tries to get code corresponding
to dccp type invalid using iptables-translate utility, then the
corresponding nft rule should be "dccp type gt syncack". Please
correct me if I'm wrong here.

Re: [PATCH] src: proto: Add missing packet type

[Florian Westphal]

Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> On Tue, Mar 1, 2016 at 6:47 PM, Florian Westphal <fw@strlen.de> wrote:
> > Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> >> Add missing packet type "invalid" for DCCP.
> >>
> >> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> >> ---
> >>  src/proto.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/src/proto.c b/src/proto.c
> >> index 0ed98ed..4d049f5 100644
> >> --- a/src/proto.c
> >> +++ b/src/proto.c
> >> @@ -443,6 +443,7 @@ static const struct symbol_table dccp_pkttype_tbl = {
> >>               SYMBOL("reset",         DCCP_PKT_RESET),
> >>               SYMBOL("sync",          DCCP_PKT_SYNC),
> >>               SYMBOL("syncack",       DCCP_PKT_SYNCACK),
> >> +             SYMBOL("invalid",       DCCP_PKT_INVALID),
> >
> > I don't think this is a good idea -- when user asks to match
> > 'invalid' then this will check type == DCCP_PKT_INVALID; however
> > the correct way would be to ask for type '> synack' (i.e.,
> > outside of the range of types specified).
> 
> OK. So, should this be removed from the enum dccp_pkt_type as well?

Yes, but we cannot do this anymore for compatibility reasons :-(

> And, if following iptables rules, one tries to get code corresponding
> to dccp type invalid using iptables-translate utility, then the
> corresponding nft rule should be "dccp type gt syncack". Please
> correct me if I'm wrong here.

Good question.
Pablo, I think -m dccp is buggy here, it should not have allowed
INVALID match in the first place.

Looking at kernel part it doesn't seem to special-case this
and will just check for type == INVALID.  But such type isn't
defined in dccp protocol, it just looks like implementation
helper so DCCP stack can do simpler sanity tests.

So I suggest to not support translation of INVALID to nft and
to document the INVALID shortoming in iptables-extensions manpage.

Any comments?

Re: [PATCH] src: proto: Add missing packet type

[Pablo Neira Ayuso]

On Tue, Mar 01, 2016 at 03:01:06PM +0100, Florian Westphal wrote:
> Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> > On Tue, Mar 1, 2016 at 6:47 PM, Florian Westphal <fw@strlen.de> wrote:
> > > Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> > >> Add missing packet type "invalid" for DCCP.
> > >>
> > >> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> > >> ---
> > >>  src/proto.c | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/src/proto.c b/src/proto.c
> > >> index 0ed98ed..4d049f5 100644
> > >> --- a/src/proto.c
> > >> +++ b/src/proto.c
> > >> @@ -443,6 +443,7 @@ static const struct symbol_table dccp_pkttype_tbl = {
> > >>               SYMBOL("reset",         DCCP_PKT_RESET),
> > >>               SYMBOL("sync",          DCCP_PKT_SYNC),
> > >>               SYMBOL("syncack",       DCCP_PKT_SYNCACK),
> > >> +             SYMBOL("invalid",       DCCP_PKT_INVALID),
> > >
> > > I don't think this is a good idea -- when user asks to match
> > > 'invalid' then this will check type == DCCP_PKT_INVALID; however
> > > the correct way would be to ask for type '> synack' (i.e.,
> > > outside of the range of types specified).
> > 
> > OK. So, should this be removed from the enum dccp_pkt_type as well?
> 
> Yes, but we cannot do this anymore for compatibility reasons :-(
> 
> > And, if following iptables rules, one tries to get code corresponding
> > to dccp type invalid using iptables-translate utility, then the
> > corresponding nft rule should be "dccp type gt syncack". Please
> > correct me if I'm wrong here.
> 
> Good question.
> Pablo, I think -m dccp is buggy here, it should not have allowed
> INVALID match in the first place.

Right.

> Looking at kernel part it doesn't seem to special-case this
> and will just check for type == INVALID.  But such type isn't
> defined in dccp protocol, it just looks like implementation
> helper so DCCP stack can do simpler sanity tests.
>
> So I suggest to not support translation of INVALID to nft and
> to document the INVALID shortoming in iptables-extensions manpage.
> 
> Any comments?

That's fine, we can return 0 (no translation available) in that case.