midauth extensions are broken

Bug #1554072 reported by Miika Komu on 2016-03-07
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
HIPL
Undecided
Christof Mroz

Bug Description

I am trying to run the midauth extensions by running the hipfw at the initiator and responder with the following options:

hipfw/hipfw -kpdAm

However, the base exchange just fails unless a turn on the following flag:

mkomu@gaijin:~/projects/hipl-bzr/trunk$ bzr diff
=== modified file 'hipfw/rewrite.c'
--- hipfw/rewrite.c 2013-08-19 18:30:29 +0000
+++ hipfw/rewrite.c 2016-02-26 10:13:27 +0000
@@ -140,7 +140,7 @@
     HIP_ASSERT(ctx);
     HIP_ASSERT(ctx->ipq_packet);

- if (assume_ipq_buffer_sufficient) {
+ if (!assume_ipq_buffer_sufficient) {
         hip_fw_context_enable_write_inplace(ctx);
         return;
     }

Any ideas to fix it properly would be appreciated.

Miika Komu (miika-iki) wrote :
Christof Mroz (christof-mroz) wrote :

Does the attached patch to hipfw/rewrite.c happen to solve or improve the issue? It seems like the ipq compatibility struct declares "payload" as a pointer, while "payload" was in fact a zero-length array in the original struct. This requires some additional pointer bending.

Has midauth really been tested since merging the nf_queue branch in revision 6420, beyond running the unit tests? Does reverting to 6419 help? I can't test it myself at the moment, but perhaps later this week.

Regarding your temporary solution, I don't recommend leaving the "assume_ipq_buffer_sufficient" flag on permanently at the moment for two reasons:

1. We only have at most HIP_MAX_DATA - x bytes of payload available, where "x" denotes the size of the nf_queue per-packet metadata, but the "splice_param" function currently assumes at least HIP_MAX_DATA bytes. This may be easy to fix by inferring x via pointer arithmetic and passing down the actual remaining space to splice_param via the context structure.

2. It's not clear to me whether or not recv() on the nf_queue socket can in read multiple packets or not. The NFQ docs imply that only a single packet get copied at once, but reading the source code of libnfnl suggests otherwise. Indeed, the callback-driven interface makes it effortless (to the library user) to process multiple packets at once, so why wouldn't they make use of this feature? In any case, multiple packets are a problem because enlarging e.g. the first packet would overwrite the beginning of the second one, if they share the same buffer. So moving memory in one form or another is necessary unless we are manipulating the very last packet in the buffer and the remaining space suffices.

I'd keep the flag around for debug and benchmarking purposes though (e.g. for measuring the impact of copying on effective throughput). Perhaps the purpose of this flag should be more clearly documented?

Miika Komu (miika-iki) wrote :

A compilation issue... scratch_buffer.ipq is not a pointer:

  CC hipfw/rewrite.o
hipfw/rewrite.c: In function ‘hip_fw_context_enable_write’:
hipfw/rewrite.c:150:28: error: incompatible types when assigning to type ‘hip_ipq_packet_msg’ from type ‘struct hip_ipq_packet_msg **’
         scratch_buffer.ipq = &ctx->ipq_packet;

Miika Komu (miika-iki) wrote :
Download full text (7.2 KiB)

If I fix the compilation problem by making the ipq member a pointer in scratch_buffer, it fails as follows:

debug(hipfw/hipfw.c:1980@hipfw_main): received IPv4 packet from iptables queue
debug(hipfw/hipfw.c:1632@fw_handle_packet): Entering netfilter callback for IPv4
debug(hipfw/hipfw.c:1396@fw_init_context): ip_hdr_len is: 20
debug(hipfw/hipfw.c:1397@fw_init_context): total length: 72
debug(hipfw/hipfw.c:1398@fw_init_context): ttl: 64
debug(hipfw/hipfw.c:1399@fw_init_context): packet length (ipq): 72
debug(hipfw/hipfw.c:1405@fw_init_context): packet src: 172.17.0.2
debug(hipfw/hipfw.c:1406@fw_init_context): packet dst: 172.17.0.1
debug(hipfw/hipfw.c:1408@fw_init_context): IPv4 next header protocol number is 17
debug(hipfw/hipfw.c:1510@fw_init_context): UDP header size is 8 (in header: 52)
debug(hipfw/hipfw.c:1511@fw_init_context): UDP src port: 10500
debug(hipfw/hipfw.c:1512@fw_init_context): UDP dst port: 10500
debug(hipfw/hipfw.c:1525@fw_init_context): zero_bytes: 0x00000000
debug(hipfw/hipfw.c:1531@fw_init_context): Zero SPI found
debug(hipfw/hipfw.c:1554@fw_init_context): UDP encapsulated HIP control packet
debug(hipfw/hipfw.c:1640@fw_handle_packet): packet hook=1, packet type=1
debug(hipfw/hipfw.c:902@filter_hip):
debug(hipfw/hipfw.c:907@filter_hip): The list of rules is empty!!!???
debug(hipfw/hipfw.c:910@filter_hip): HIP type number is 1
info(hipfw/hipfw.c:914@filter_hip): received packet type: I1
info(hipfw/hipfw.c:952@filter_hip): src hit: 2001:0017:03b4:b5cc:bad2:26e7:0eb2:8198
info(hipfw/hipfw.c:953@filter_hip): dst hit: 2001:001b:b6ae:fca7:3d97:0ff1:e489:5f83
info(hipfw/hipfw.c:954@filter_hip): src ip: 172.17.0.2
info(hipfw/hipfw.c:955@filter_hip): dst ip: 172.17.0.1
debug(hipfw/hipfw.c:1060@filter_hip): falling back to default HIP/ESP behavior, target 1
debug(hipfw/conntrack.c:2065@get_tuple_by_hits): get_tuple_by_hits: no connection found
debug(hipfw/conntrack.c:294@get_tuple_by_hip): get_tuple_by_hip: no connection found
debug(hipfw/conntrack.c:1736@check_packet): check packet: type 1
debug(hipfw/dlist.c:137@append_to_list): List is empty inserting first node
debug(hipfw/dlist.c:133@append_to_list): List is not empty. Length 1
debug(hipfw/conntrack.c:2059@get_tuple_by_hits): connection found,
debug(hipfw/conntrack.c:1791@check_packet): udp_encap_hdr=0x7fff63f2bb7c tuple=(nil) err=1
debug(hipfw/hipfw.c:1653@fw_handle_packet): === Verdict: allow packet ===
debug(hipfw/hipfw.c:1600@allow_packet): Packet accepted

debug(hipfw/hipfw.c:1980@hipfw_main): received IPv4 packet from iptables queue
debug(hipfw/hipfw.c:1632@fw_handle_packet): Entering netfilter callback for IPv4
debug(hipfw/hipfw.c:1396@fw_init_context): ip_hdr_len is: 20
debug(hipfw/hipfw.c:1397@fw_init_context): total length: 672
debug(hipfw/hipfw.c:1398@fw_init_context): ttl: 64
debug(hipfw/hipfw.c:1399@fw_init_context): packet length (ipq): 672
debug(hipfw/hipfw.c:1405@fw_init_context): packet src: 172.17.0.1
debug(hipfw/hipfw.c:1406@fw_init_context): packet dst: 172.17.0.2
debug(hipfw/hipfw.c:1408@fw_init_context): IPv4 next header protocol number is 17
debug(hipfw/hipfw.c:1510@fw_init_context): UDP header size is 8 (in header: 652)
debug(hipfw/hipfw.c:1511@fw_in...

Read more...

Miika Komu (miika-iki) wrote :

This patch compiles but fails as mentioned in the bug report.

Christof Mroz (christof-mroz) wrote :

You're right; changing this line to the following seems to compile at least (still untested):

scratch_buffer.ipq = *(ctx->ipq_packet);

Copying the ipq struct may not even be necessary at all: perhaps it suffices to reset the payload pointer and overwrite the old struct, since it seems to get reconstructed for every packet anyway.

Miika Komu (miika-iki) wrote :

Christoph, can you commit your code with this the previous modification you suggested? It actually works. Then you can close the bug. Thanks!

Christof Mroz (christof-mroz) wrote :

Ah, good to hear that. I don't feel comfortable commiting directly since I can't test it yet (have to do the hipd/hipfw tutorial again first...) but I'll create a branch so you can test and merge it yourself.

Miika Komu (miika-iki) on 2016-03-10
Changed in hipl:
assignee: nobody → Christof Mroz (christof-mroz)
status: New → Fix Committed
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers