BUG - Cast away of const inside ndpi_detection_process_packet

[bcronje]

In ndpi_detection_process_packet the packet parameter is defined as const unsigned char *packet:

ndpi_protocol ndpi_detection_process_packet(struct ndpi_detection_module_struct *ndpi_struct,
                    struct ndpi_flow_struct *flow,
                    const unsigned char *packet,
                    const unsigned short packetlen,
                    const u_int64_t current_tick_l,
                    struct ndpi_id_struct *src,
                    struct ndpi_id_struct *dst)

However on line https://github.com/ntop/nDPI/blob/dev/src/lib/ndpi_main.c#L3316 you cast this const away:

  flow->packet.iph = (struct ndpi_iphdr *)packet;

Which eventually gets use as a parameter to ndpi_network_ptree_match in line https://github.com/ntop/nDPI/blob/dev/src/lib/ndpi_main.c#L3423 :

struct ndpi_packet_struct *packet = &flow->packet;

if((ret.master_protocol = ndpi_network_ptree_match(ndpi_struct, (struct in_addr *)&packet->iph->saddr)) == NDPI_PROTOCOL_UNKNOWN)
  ret.master_protocol = ndpi_network_ptree_match(ndpi_struct, (struct in_addr *)&packet->iph->daddr);

Inside ndpi_network_ptree_match you write to this memory https://github.com/ntop/nDPI/blob/dev/src/lib/ndpi_main.c#L1673:

pin->s_addr = ntohl(pin->s_addr); /* Make sure all in network byte order otherwise compares wont work */

This causes a segfault if packet is in read-only memory. Surely you should not be writing anything to the library consumer packet buffer? I.e. flow->packet.iph should be read-only at all times?

[lucaderi]

Good. Can you please provide us a patch (merge request?)

[bcronje]

Sure, I'll give it a go. Will send through a pull request sometime today.

[bcronje]

@lucaderi I've send the pull request. That PR only addresses the invalid write to the packet ip header, it ensures that we do not pass flow->packet.iph->saddr or flow->packet.iph->saddr directly to ndpi_network_ptree_match where it will get written to. So the logic hasn't changed.

However, I think there is room for improvement which I could address in another pull request. I'll open a new issue for this.