[netsniff-ng] Re: [RFC 6/7] trafgen: l7: Add DNS header generation API

[Tobias Klauser]

On 2017-03-14 at 20:15:45 +0100, Vadim Kochan  wrote:
> On Tue, Mar 14, 2017 at 12:21 PM, Tobias Klauser  wrote:
> > On 2017-01-30 at 09:33:29 +0100, Vadim Kochan  wrote:
> >> Add trafgen_l7.c module with DNS proto header generation with
> >> support of filling DNS query/answer/authority/additional sections
> >> as sub headers.
> >>
> >> Introcuded new concept as 'sub header' which is needed to easy handle
> >> DNS sections which might be added on-demand, and to simplify using
> >> sub-header as regular header with a fields, offset, etc. There is a
> >> parent header which contains array of pointers of sub-headers, and the
> >> array is ordered as they are located in the parent header. The
> >> sub-headers mostly encapsulated by the parent header which 'knows'
> >> the semantic of them. The new proto_hdr->push_sub_header(...) callback
> >> was added to tell the parent header to push the sub-header's fields,
> >> sub-header also may have proto_ops which must be filled by the parent.
> >> This sub-header concept might be used in the future if it will be needed
> >> to support DHCP, WLAN headers.
> >>
> >> There are 4 kinds of DNS sub-headers - query, answer, authority,
> >> additional. 'id' of each sub-header is used to only differentiate these
> >> types of sections. These sections have strict order inside DNS header,
> >> and there was added the proto_hdr_move_sub_header(...) to sort them in
> >> required order.
> >
> > Might be a bit of a naive question: But wouldn't it be possible to
> > enforce the sub-header order through the parser (i.e. only allow
> > trafgen scripts which specify the respective sections in the right
> > order? This would safe us from doing the whole header sorting/moving
> > dance which looks a bit sacry to me (memmove of payload especially).
> >
> 
> Well, I was thinking about this and decided to do so because:
> 
> 1) I really think that there might be in the future libtrafgen
> which will handle sub-headers
>  automatically (if it is needed by protocol header) both for
> the application and trafgen tool (this
>  reason might be not so strict).

Well, that's a bit hypothetical ;) And a don't quite see how this
prevents us from going with the easier, less scary solution for now ;)

> 2) sub-headers might be a generic way for other protocol headers
> like DHCP, WLAN.

We could enforce sub-header order there too, no?

> Yes, the logic which does sub-headers moving is a bit scary as you
> said, not sure if I can re-write it
> to simplify it, will try.

Ok, let's try that. I'm not strictly against the current solution and I
see that there might be cases in the future where this would be
benefitial.  I'd just like to prevent us from introducing a lot of code
with growing complexity which could be prevented if we were a bit more
restrictive in terms of what the user can do.

Thanks
Tobias

-- 
You received this message because you are subscribed to the Google Groups 
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[netsniff-ng] Re: [RFC 6/7] trafgen: l7: Add DNS header generation API

[Vadim Kochan]

On Tue, Mar 14, 2017 at 12:21 PM, Tobias Klauser  wrote:
> On 2017-01-30 at 09:33:29 +0100, Vadim Kochan  wrote:
>> Add trafgen_l7.c module with DNS proto header generation with
>> support of filling DNS query/answer/authority/additional sections
>> as sub headers.
>>
>> Introcuded new concept as 'sub header' which is needed to easy handle
>> DNS sections which might be added on-demand, and to simplify using
>> sub-header as regular header with a fields, offset, etc. There is a
>> parent header which contains array of pointers of sub-headers, and the
>> array is ordered as they are located in the parent header. The
>> sub-headers mostly encapsulated by the parent header which 'knows'
>> the semantic of them. The new proto_hdr->push_sub_header(...) callback
>> was added to tell the parent header to push the sub-header's fields,
>> sub-header also may have proto_ops which must be filled by the parent.
>> This sub-header concept might be used in the future if it will be needed
>> to support DHCP, WLAN headers.
>>
>> There are 4 kinds of DNS sub-headers - query, answer, authority,
>> additional. 'id' of each sub-header is used to only differentiate these
>> types of sections. These sections have strict order inside DNS header,
>> and there was added the proto_hdr_move_sub_header(...) to sort them in
>> required order.
>
> Might be a bit of a naive question: But wouldn't it be possible to
> enforce the sub-header order through the parser (i.e. only allow
> trafgen scripts which specify the respective sections in the right
> order? This would safe us from doing the whole header sorting/moving
> dance which looks a bit sacry to me (memmove of payload especially).
>

Well, I was thinking about this and decided to do so because:

1) I really think that there might be in the future libtrafgen
which will handle sub-headers
 automatically (if it is needed by protocol header) both for
the application and trafgen tool (this
 reason might be not so strict).

2) sub-headers might be a generic way for other protocol headers
like DHCP, WLAN.

Yes, the logic which does sub-headers moving is a bit scary as you
said, not sure if I can re-write it
to simplify it, will try.

Regards,
Vadim Kochan

-- 
You received this message because you are subscribed to the Google Groups 
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[netsniff-ng] Re: [RFC 6/7] trafgen: l7: Add DNS header generation API

[Tobias Klauser]

On 2017-01-30 at 09:33:29 +0100, Vadim Kochan  wrote:
> Add trafgen_l7.c module with DNS proto header generation with
> support of filling DNS query/answer/authority/additional sections
> as sub headers.
> 
> Introcuded new concept as 'sub header' which is needed to easy handle
> DNS sections which might be added on-demand, and to simplify using
> sub-header as regular header with a fields, offset, etc. There is a
> parent header which contains array of pointers of sub-headers, and the
> array is ordered as they are located in the parent header. The
> sub-headers mostly encapsulated by the parent header which 'knows'
> the semantic of them. The new proto_hdr->push_sub_header(...) callback
> was added to tell the parent header to push the sub-header's fields,
> sub-header also may have proto_ops which must be filled by the parent.
> This sub-header concept might be used in the future if it will be needed
> to support DHCP, WLAN headers.
> 
> There are 4 kinds of DNS sub-headers - query, answer, authority,
> additional. 'id' of each sub-header is used to only differentiate these
> types of sections. These sections have strict order inside DNS header,
> and there was added the proto_hdr_move_sub_header(...) to sort them in
> required order.

Might be a bit of a naive question: But wouldn't it be possible to
enforce the sub-header order through the parser (i.e. only allow
trafgen scripts which specify the respective sections in the right
order? This would safe us from doing the whole header sorting/moving
dance which looks a bit sacry to me (memmove of payload especially).

A few minor comments inline below.

> Actually there are only 2 proto_hdr's which describes 4 DNS sections -
> query & rrecord, because rrecord covers another 3 - answer, auhority,
> additional which have the same layout.
> 
> Signed-off-by: Vadim Kochan 
> ---
>  trafgen/Makefile |   1 +
>  trafgen_l4.c |  32 ++
>  trafgen_l7.c | 175 
> +++
>  trafgen_l7.h |  45 ++
>  trafgen_proto.c  | 128 
>  trafgen_proto.h  |  18 +-
>  6 files changed, 398 insertions(+), 1 deletion(-)
>  create mode 100644 trafgen_l7.c
>  create mode 100644 trafgen_l7.h
> 
> diff --git a/trafgen/Makefile b/trafgen/Makefile
> index 876ed93..95a31e0 100644
> --- a/trafgen/Makefile
> +++ b/trafgen/Makefile
> @@ -25,6 +25,7 @@ trafgen-objs =  xmalloc.o \
>   trafgen_l2.o \
>   trafgen_l3.o \
>   trafgen_l4.o \
> + trafgen_l7.o \
>   trafgen_lexer.yy.o \
>   trafgen_parser.tab.o \
>   trafgen.o
> diff --git a/trafgen_l4.c b/trafgen_l4.c
> index 5a694b3..198d622 100644
> --- a/trafgen_l4.c
> +++ b/trafgen_l4.c
> @@ -80,6 +80,21 @@ static void udp_packet_finish(struct proto_hdr *hdr)
>   udp_csum_update(hdr);
>  }
>  
> +static void udp_set_next_proto(struct proto_hdr *hdr, enum proto_id pid)
> +{
> + uint16_t dport;
> +
> + switch (pid) {
> + case PROTO_DNS:
> + dport = 53;
> + break;
> + default:
> + bug();
> + }
> +
> + proto_hdr_field_set_default_be16(hdr, UDP_DPORT, dport);
> +}
> +
>  static const struct proto_ops udp_proto_ops = {
>   .id = PROTO_UDP,
>   .layer  = PROTO_L4,
> @@ -87,6 +102,7 @@ static const struct proto_ops udp_proto_ops = {
>   .packet_update  = udp_csum_update,
>   .packet_finish  = udp_packet_finish,
>   .field_changed  = udp_field_changed,
> + .set_next_proto = udp_set_next_proto,
>  };
>  
>  static struct proto_field tcp_fields[] = {
> @@ -160,6 +176,21 @@ static void tcp_csum_update(struct proto_hdr *hdr)
>   hdr->is_csum_valid = true;
>  }
>  
> +static void tcp_set_next_proto(struct proto_hdr *hdr, enum proto_id pid)
> +{
> + uint16_t dport;
> +
> + switch (pid) {
> + case PROTO_DNS:
> + dport = 53;
> + break;
> + default:
> + bug();
> + }
> +
> + proto_hdr_field_set_default_be16(hdr, TCP_DPORT, dport);
> +}
> +
>  static const struct proto_ops tcp_proto_ops = {
>   .id = PROTO_TCP,
>   .layer  = PROTO_L4,
> @@ -167,6 +198,7 @@ static const struct proto_ops tcp_proto_ops = {
>   .packet_update  = tcp_csum_update,
>   .packet_finish  = tcp_csum_update,
>   .field_changed  = tcp_field_changed,
> + .set_next_proto = tcp_set_next_proto,
>  };
>  
>  static struct proto_field icmpv4_fields[] = {
> diff --git a/trafgen_l7.c b/trafgen_l7.c
> new file mode 100644
> index 000..1e82ccb
> --- /dev/null
> +++ b/trafgen_l7.c
> @@ -0,0 +1,175 @@
> +/*
> + * netsniff-ng - the packet sniffing beast
> + * Subject to the GPL, version 2.
> + */
> +
> +#include 
> +
> +#include "str.h"
> +#include "xmalloc.h"
> +#include "built_in.h"
> +#include "trafgen_l7.h"
> +#include "trafgen_proto.h"
> +
>