Merge lp:~christof-mroz/hipl/hipfw-timeout into lp:hipl

[Christof Mroz]

Addressed the review comments. Pay special attention to the commit message of r5793:
http://bazaar.launchpad.net/~christof-mroz/hipl/hipfw-timeout/revision/5793

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christof!

[Full quote because we forgot to CC the merge proposal on launchpad]

On 03/18/2011 01:41 PM, Christof Mroz wrote:
> On Thu, 17 Mar 2011 19:07:34 +0100
> Stefan Götz <email address hidden> wrote:
>
>>> > > +#ifdef CONFIG_HIP_DEBUG
>>> > > +// this improves our chances of finding bugs in the timeout code
>>> > > +#define DEFAULT_CONNECTION_TIMEOUT 10; // 10 seconds
>>> > > +#define DEFAULT_CLEANUP_INTERVAL 5; // 5 seconds
>>> > > +#else
>>> > > +#define DEFAULT_CONNECTION_TIMEOUT (60 * 5); // 5 minutes
>>> > > +#define DEFAULT_CLEANUP_INTERVAL (60 * 60); // 1 minute
>>> > > +#endif
>> >
>> > [M] Please use typed constants instead of preprocessor macros where
>> > possible
> Unfortunately, defining DEFAULT_{CONNECTION_TIMEOUT,CLEANUP_INTERVAL} as
> constants leads to the following complaints:
>
> firewall/conntrack.c|98| error: initializer element is not constant
> firewall/conntrack.c|109| error: initializer element is not constant

Erch, yes, one can only do that in C++, not in C. Can we move HIPL to C++, pleeeeeease? ;-)

> I could set the globals directly depending on CONFIG_HIP_DEBUG, but that
> wouldn't look so neat in my opinion, especially considering the doxygen
> comments. Do you have a better idea?

No, I think preprocessor macros are our only solution.

>> > [L] Why is gettimeofday() replaced with time()?
> gettimeofday() returns more information than needed.
>
>> > [L] it seems to me that you rely on select() to unblock frequently
>> > enough to keep up with the cleanup schedule. While that is not a
>> > particularly strict schedule, would it make sense to integrate the
>> > value in 'cleanup_interval' with the timeout set for the select()
>> > call?
> The select() timeout is currently fixed to 1 second, I guess that's enough :)
> (It has been that way before I touched the code, so I don't know the reason)

Ah, i didn't check the code that far. It's alright then.

>> > [M] The function declaration and definition have different const
>> > qualifications for the 'list' arguments. I like the 'struct slist
>> > *const list' better :-) Annyoing that the compiler doesn't catch this.
> Fixed, thanks. Can't use const here because the pointer (or rather, the local
> copy of it) is modified during execution.

Ok

Stefan

review: Approve

[René Hummen (rene-hummen)]

Seems that test/firewall/conntrack.c is missing.

review: Needs Fixing

[Christof Mroz (christof-mroz)]

> Seems that test/firewall/conntrack.c is missing.

You're right, sry... just added it now.

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christof!

> === modified file 'firewall/firewall.c'
> --- firewall/firewall.c 2011-03-22 10:31:37 +0000
> +++ firewall/firewall.c 2011-03-22 12:49:26 +0000
> @@ -1669,6 +1669,80 @@
> }
>
> /**
> + * Receive and process one message from hipd.
> + *
> + * @param msg A previously allocated message buffer.
> + * @return Zero on success, non-zero on error.
> + *
> + * @note The buffer @a msg is reused between calls because it is quite
> + * large.
> + */
> +static int hip_fw_handle_hipd_message(struct hip_common *msg)

[M] style: 'struct hip_common *const msg' for const correctness - sorry, I hadn't spotted this before

I know that some of the following is not your code, but it is fairly buggy, and in nasty ways, too - maybe it's something one could address as an aside in a different branch?

> +{
> + struct sockaddr_in6 sock_addr;
> + int msg_type, len, n;
> + int is_root, access_ok;
> + socklen_t alen;
> +
> + alen = sizeof(sock_addr);
> + n = recvfrom(hip_fw_async_sock, msg, sizeof(struct hip_common),
> + MSG_PEEK, (struct sockaddr *) &sock_addr, &alen);
> + if (n < 0) {
> + HIP_ERROR("Error receiving message header from daemon.\n");
> + return -1;
> + }

[H] unrelated: One should definitely check whether 'n' is equal to 'sizeof(struct hip_common)' because 'recvfrom()' might just have read a single byte but the remaining code will happily try to parse it as a valid message.

> +
> + // making sure user messages are received from hipd
> + // resetting vars to 0 because it is a loop
> + msg_type = hip_get_msg_type(msg);
> + is_root = (ntohs(sock_addr.sin6_port) < 1024);

[H] unrelated: Holy moly - the sock_addr handling is screwed up because it is assumed that it contains an IPv6 address. That can not be assumed at all in the general case. First, one should pass a 'struct sockaddr_storage' object to 'recvfrom()'. Second, one should then check whether the returned address is in fact an IPv6 address before treating it as such.

> + if (is_root) {
> + access_ok = 1;
> + } else if (!is_root &&
> + (msg_type >= HIP_MSG_ANY_MIN &&
> + msg_type <= HIP_MSG_ANY_MAX)) {
> + access_ok = 1;
> + }
> + if (!access_ok) {
> + HIP_ERROR("The sender of the message is not trusted.\n");
> + return -1;
> + }

[L] unrelated: The whole is_root and access_ok shebang could be replaced with:

if (ntohs(sock_addr.sin6_port) >= 1024 &&
    (msg_type < HIP_MSG_ANY_MIN || msg_type > HIP_MSG_ANY_MAX)) {
    HIP_ERROR("The sender of the message is not trusted.\n");
    return -1;
}

> +
> + alen = sizeof(sock_addr);
> + len = hip_get_msg_total_len(msg);
> +
> + HIP_DEBUG("Receiving message type %d (%d bytes)\n",
> + hip_get_msg_type(msg), len);
> + n = recvfrom(hip_fw_async_sock, msg, len, 0,
> + (struct sockaddr *) &sock_addr, &alen);

[H] unrelated: 'len' must be sanity checked against the amount of memory allocated for 'msg' in order to prevent a buffer overrun, which, at this point, can be caused by grafted messages from unprivilege...

review: Needs Fixing

[Christof Mroz (christof-mroz)]

> > +static int hip_fw_handle_hipd_message(struct hip_common *msg)
>
> [M] style: 'struct hip_common *const msg' for const correctness - sorry, I
> hadn't spotted this before

Done, thanks.

> I know that some of the following is not your code, but it is fairly buggy,
> and in nasty ways, too - maybe it's something one could address as an
> aside in a different branch?

If so, domain sockets or fifos should be considered for access control (and, really, because they're exactly made for local IPC).

> > +
> > + // making sure user messages are received from hipd
> > + // resetting vars to 0 because it is a loop
> > + msg_type = hip_get_msg_type(msg);
> > + is_root = (ntohs(sock_addr.sin6_port) < 1024);
>
> [H] unrelated: Holy moly - the sock_addr handling is screwed up because it is
> assumed that it contains an IPv6 address. That can not be assumed at all in
> the general case. First, one should pass a 'struct sockaddr_storage' object to
> 'recvfrom()'. Second, one should then check whether the returned address is in
> fact an IPv6 address before treating it as such.

Just curious: how could this happen for an AF_INET6 datagram socket, like in this case?
hipfw maintains both an AF_INET and AF_INET6 at another place socket, for example, and I assume because either one listens only on one kind of address?

> > +/**
> > + * Find an element in the singly linked list.
> > + *
> > + * @param list The linked list.
> > + * @param data The element to find.
> > + * @return The element in the linked list, or NULL if not found.
> > + */
> > +struct slist *find_in_slist(struct slist *list, const void *const data)
>
> [M] would 'const struct slist *list' work for better const correctness?

Unfortunately, the const qualifier would propagate to the return value, which can't be const.

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> > [H] unrelated: Holy moly - the sock_addr handling is screwed up because it
> is
> > assumed that it contains an IPv6 address. That can not be assumed at all in
> > the general case. First, one should pass a 'struct sockaddr_storage' object
> to
> > 'recvfrom()'. Second, one should then check whether the returned address is
> in
> > fact an IPv6 address before treating it as such.
>
> Just curious: how could this happen for an AF_INET6 datagram socket, like in
> this case?

Oh, well I guess they can't. I was assuming the general case of not knowing the type of the source address. Personally, I would still do this sanity check, just in case.

> > > +struct slist *find_in_slist(struct slist *list, const void *const data)
> >
> > [M] would 'const struct slist *list' work for better const correctness?
>
> Unfortunately, the const qualifier would propagate to the return value, which
> can't be const.

Ok

review: Approve

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Tue, Mar 22, 2011 at 12:49:33PM +0000, Christof Mroz wrote:
> Christof Mroz has proposed merging lp:~christof-mroz/hipl/hipfw-timeout into lp:hipl.
>
> --- Makefile.am 2011-03-22 10:31:37 +0000
> +++ Makefile.am 2011-03-22 12:49:26 +0000
> @@ -193,10 +193,28 @@
> endif
>
>
> -test_check_firewall_SOURCES = test/check_firewall.c \
> - test/firewall/file_buffer.c \
> - test/firewall/line_parser.c \
> - test/firewall/port_bindings.c
> +test_check_firewall_SOURCES = test/check_firewall.c \
> + test/firewall/file_buffer.c \
> + test/firewall/line_parser.c \
> + test/firewall/port_bindings.c \
> + test/firewall/conntrack.c \

alphabetical order

> + firewall/cache.c \
> + firewall/dlist.c \
> + firewall/esp_prot_api.c \
> + firewall/esp_prot_config.c \
> + firewall/esp_prot_conntrack.c \
> + firewall/esp_prot_fw_msg.c \
> + firewall/firewall.c \
> + firewall/firewall_control.c \
> + firewall/helpers.c \
> + firewall/hslist.c \
> + firewall/lsi.c \
> + firewall/reinject.c \
> + firewall/rule_management.c \
> + firewall/user_ipsec_api.c \
> + firewall/user_ipsec_esp.c \
> + firewall/user_ipsec_fw_msg.c \
> + firewall/user_ipsec_sadb.c

Doh, why?

> --- firewall/firewall.c 2011-03-22 10:31:37 +0000
> +++ firewall/firewall.c 2011-03-22 12:49:26 +0000
> @@ -1669,6 +1669,80 @@
>
> + // making sure user messages are received from hipd
> + // resetting vars to 0 because it is a loop
> + msg_type = hip_get_msg_type(msg);
> + is_root = (ntohs(sock_addr.sin6_port) < 1024);

pointless parentheses

> --- firewall/main.c 2011-03-22 10:31:37 +0000
> +++ firewall/main.c 2011-03-22 12:49:26 +0000
> --- test/check_firewall.c 2011-02-18 14:39:38 +0000
> +++ test/check_firewall.c 2011-03-22 12:49:26 +0000
> @@ -34,6 +34,7 @@
> SRunner *sr = srunner_create(firewall_file_buffer());
> srunner_add_suite(sr, firewall_line_parser());
> srunner_add_suite(sr, firewall_port_bindings());
> + srunner_add_suite(sr, firewall_conntrack());

nit: alphabetical order

> --- test/firewall/conntrack.c 1970-01-01 00:00:00 +0000
> +++ test/firewall/conntrack.c 2011-03-22 12:49:26 +0000
> @@ -0,0 +1,107 @@
> +
> +static struct connection *setup_connection(void) {

Put the { on the next line.

> --- test/firewall/test_suites.h 2011-02-18 14:39:38 +0000
> +++ test/firewall/test_suites.h 2011-03-22 12:49:26 +...

review: Needs Fixing

[Christof Mroz (christof-mroz)]

On Thu, 24 Mar 2011 15:27:30 +0100, Diego Biurrun <email address hidden> wrote:

>> + firewall/cache.c \
>> + firewall/dlist.c \
>> + firewall/esp_prot_api.c \
>> + firewall/esp_prot_config.c \
>> + firewall/esp_prot_conntrack.c \
>> + firewall/esp_prot_fw_msg.c \
>> + firewall/firewall.c \
>> + firewall/firewall_control.c \
>> + firewall/helpers.c \
>> + firewall/hslist.c \
>> + firewall/lsi.c \
>> + firewall/reinject.c \
>> + firewall/rule_management.c \
>> + firewall/user_ipsec_api.c \
>> + firewall/user_ipsec_esp.c \
>> + firewall/user_ipsec_fw_msg.c \
>> + firewall/user_ipsec_sadb.c
>
> Doh, why?

Ugly, I know... I use a tiny bit of code from firewall/conntrack.c in one
test, but this triggers a whole cascade of dependencies. Do you know of a
way to tell ld to use more "fine-grained" dependencies, ignoring symbols
that aren't actually referenced? As far as I see, it's pure coincidence
that the other tests are more self-contained.

>
>> --- firewall/firewall.c 2011-03-22 10:31:37 +0000
>> +++ firewall/firewall.c 2011-03-22 12:49:26 +0000
>> @@ -1669,6 +1669,80 @@
>>
>> + // making sure user messages are received from hipd
>> + // resetting vars to 0 because it is a loop
>> + msg_type = hip_get_msg_type(msg);
>> + is_root = (ntohs(sock_addr.sin6_port) < 1024);
>
> pointless parentheses

Not fixed: Copy & Pasted that, and as Stefan pointed out it would be
better to fix this function from scratch.

[Diego Biurrun (diego-biurrun)]

On Thu, Mar 24, 2011 at 03:54:47PM +0100, Christof Mroz wrote:
> On Thu, 24 Mar 2011 15:27:30 +0100, Diego Biurrun <email address hidden> wrote:
>
>>> + firewall/cache.c \
>>> + firewall/dlist.c \
>>> + firewall/esp_prot_api.c \
>>> + firewall/esp_prot_config.c \
>>> + firewall/esp_prot_conntrack.c \
>>> + firewall/esp_prot_fw_msg.c \
>>> + firewall/firewall.c \
>>> + firewall/firewall_control.c \
>>> + firewall/helpers.c \
>>> + firewall/hslist.c \
>>> + firewall/lsi.c \
>>> + firewall/reinject.c \
>>> + firewall/rule_management.c \
>>> + firewall/user_ipsec_api.c \
>>> + firewall/user_ipsec_esp.c \
>>> + firewall/user_ipsec_fw_msg.c \
>>> + firewall/user_ipsec_sadb.c
>>
>> Doh, why?
>
> Ugly, I know... I use a tiny bit of code from firewall/conntrack.c in one
> test, but this triggers a whole cascade of dependencies. Do you know of a
> way to tell ld to use more "fine-grained" dependencies, ignoring symbols
> that aren't actually referenced? As far as I see, it's pure coincidence
> that the other tests are more self-contained.

What do you mean by "use a tiny bit of code"?

I don't see why you need to add any of these files at all.
Dependencies should be worked out automatically.

Diego

[Christof Mroz (christof-mroz)]

On Thu, 24 Mar 2011 16:18:47 +0100, Diego Biurrun <email address hidden> wrote:

>> Ugly, I know... I use a tiny bit of code from firewall/conntrack.c in
>> one test, but this triggers a whole cascade of dependencies. Do you
>> know of a way to tell ld to use more "fine-grained" dependencies,
>> ignoring symbols that aren't actually referenced? As far as I see,
>> it's pure coincidence that the other tests are more self-contained.
>
> What do you mean by "use a tiny bit of code"?

I mean that only a fraction of the symbols exported by all those object
files is needed in the unit test.

> I don't see why you need to add any of these files at all.
> Dependencies should be worked out automatically.

Put more simply: If I remove all these other files and run "make check", I
get errors about missing symbols, and I don't know what to do about it
(other than including way more symbols than necessary into the build).
Please try and see for yourself.

What do you think of simply appending firewall_hipfw_SOURCES to the list?
Wouldn't blow up Makefile.am so much.

[Diego Biurrun (diego-biurrun)]

On Thu, Mar 24, 2011 at 04:33:09PM +0100, Christof Mroz wrote:
>
> What do you think of simply appending firewall_hipfw_SOURCES to the list?
> Wouldn't blow up Makefile.am so much.

This would be much cleaner.

Diego

[Christof Mroz (christof-mroz)]

On Thu, 24 Mar 2011 19:14:00 +0100, Diego Biurrun <email address hidden> wrote:

> On Thu, Mar 24, 2011 at 04:33:09PM +0100, Christof Mroz wrote:
>>
>> What do you think of simply appending firewall_hipfw_SOURCES to the
>> list?
>> Wouldn't blow up Makefile.am so much.
>
> This would be much cleaner.

The following happens to work, are you fine with that?

=== modified file 'Makefile.am'
--- Makefile.am 2011-03-24 14:37:48 +0000
+++ Makefile.am 2011-03-24 19:00:18 +0000
@@ -193,28 +193,12 @@
  endif

-test_check_firewall_SOURCES = test/check_firewall.c \
- test/firewall/conntrack.c \
- test/firewall/file_buffer.c \
- test/firewall/line_parser.c \
- test/firewall/port_bindings.c \
- firewall/cache.c \
- firewall/dlist.c \
- firewall/esp_prot_api.c \
- firewall/esp_prot_config.c \
- firewall/esp_prot_conntrack.c \
- firewall/esp_prot_fw_msg.c \
- firewall/firewall.c \
- firewall/firewall_control.c \
- firewall/helpers.c \
- firewall/hslist.c \
- firewall/lsi.c \
- firewall/reinject.c \
- firewall/rule_management.c \
- firewall/user_ipsec_api.c \
- firewall/user_ipsec_esp.c \
- firewall/user_ipsec_fw_msg.c \
- firewall/user_ipsec_sadb.c
+test_check_firewall_SOURCES = test/check_firewall.c \
+ test/firewall/conntrack.c \
+ test/firewall/file_buffer.c \
+ test/firewall/line_parser.c \
+ test/firewall/port_bindings.c \
+ $(firewall_hipfw_SOURCES)

  if HIP_MIDAUTH
  test_check_firewall_SOURCES += firewall/midauth.c \
@@ -246,6 +230,8 @@
  tools_hipconf_LDADD = lib/core/libhipcore.la
  tools_pisacert_LDADD = lib/core/libhipcore.la

+test_check_firewall_LDFLAGS = -Wl,-zmuldefs
+
  dist_sbin_SCRIPTS = tools/hipdnskeyparse/hipdnskeyparse \
                      tools/hipdnsproxy/hipdnsproxy \
                      tools/nsupdate.pl

-zmuldefs is necessary because we have multiple symbol definitions here
(which point to equivalent code though, apart from main()). This option
causes ld to prefer those objects listed first, which in turn belong to
the sources listed first.

One question though, since you know more about the build system than me:
Why are the LDFLAGS passed to gcc verbatim, i.e., why is the -Wl
necessary? Shouldn't gcc's (or make's?) ld child process pick up the
LDFLAGS instead, which would render...

[Diego Biurrun (diego-biurrun)]

On Thu, Mar 24, 2011 at 08:08:37PM +0100, Christof Mroz wrote:
> On Thu, 24 Mar 2011 19:14:00 +0100, Diego Biurrun <email address hidden> wrote:
>
>> On Thu, Mar 24, 2011 at 04:33:09PM +0100, Christof Mroz wrote:
>>>
>>> What do you think of simply appending firewall_hipfw_SOURCES to the
>>> list? Wouldn't blow up Makefile.am so much.
>>
>> This would be much cleaner.
>
> The following happens to work, are you fine with that?

I implemented something better I think. Just pull from trunk.

Diego

[René Hummen (rene-hummen)]

The connection tracking functionality of the hipfw can be turned off with the -F option. How do your changes cope with this fact?

Did you test that "hipfw -F" is also running as expected? Especially, the call "hip_fw_conntrack_periodic_cleanup(time(NULL));" might cause problems in this case.

[Christof Mroz (christof-mroz)]

On Sun, 27 Mar 2011 15:39:29 +0200, René Hummen
<email address hidden> wrote:

> The connection tracking functionality of the hipfw can be turned off
> with the -F option. How do your changes cope with this fact?
>
> Did you test that "hipfw -F" is also running as expected? Especially,
> the call "hip_fw_conntrack_periodic_cleanup(time(NULL));" might cause
> problems in this case.

No connection tracking means no connections to iterate through and time
out.
I'll add an explicit check though, for clarity and forward compatibility.

[lp:~christof-mroz/hipl/hipfw-timeout]

5794. By Christof Mroz on 2011-03-27

Don't document non-existant parameter.

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Sun, Mar 27, 2011 at 03:51:36PM +0000, Christof Mroz wrote:
> Christof Mroz has proposed merging lp:~christof-mroz/hipl/hipfw-timeout into lp:hipl.
>
> --- firewall/conntrack.c 2011-03-24 17:34:21 +0000
> +++ firewall/conntrack.c 2011-03-27 15:51:27 +0000
> @@ -74,8 +74,33 @@
> +
> +#define DEFAULT_CONNECTION_TIMEOUT (60 * 5); // 5 minutes
> +#define DEFAULT_CLEANUP_INTERVAL (60 * 1); // 1 minute
> +
> +time_t cleanup_interval = DEFAULT_CLEANUP_INTERVAL;
> +
> +time_t connection_timeout = DEFAULT_CONNECTION_TIMEOUT;

The defines seem completely redundant to me, just assign the values to
the variables without indirection.

> --- firewall/firewall.c 2011-03-24 11:35:48 +0000
> +++ firewall/firewall.c 2011-03-27 15:51:27 +0000
> @@ -1668,6 +1668,80 @@
>
> /**
> + * Receive and process one message from hipd.
> + *
> + * @param msg A previously allocated message buffer.
> + * @return Zero on success, non-zero on error.

You return -1, not non-zero, please say so. Positive numbers are
non-zero as well, so this is confusing.

> +static int hip_fw_handle_hipd_message(struct hip_common *const msg)
> +{
> + struct sockaddr_in6 sock_addr;
> + int msg_type, len, n;
> + int is_root, access_ok;
> + socklen_t alen;
> +
> + alen = sizeof(sock_addr);
> + n = recvfrom(hip_fw_async_sock, msg, sizeof(struct hip_common),
> + MSG_PEEK, (struct sockaddr *) &sock_addr, &alen);
> + if (n < 0) {
> + HIP_ERROR("Error receiving message header from daemon.\n");
> + return -1;
> + }
> +
> + // making sure user messages are received from hipd
> + // resetting vars to 0 because it is a loop
> + msg_type = hip_get_msg_type(msg);
> + is_root = (ntohs(sock_addr.sin6_port) < 1024);

Please remove the pointless parentheses around the right-hand-side
expression.

> + if (is_root) {
> + access_ok = 1;
> + } else if (!is_root &&
> + (msg_type >= HIP_MSG_ANY_MIN &&
> + msg_type <= HIP_MSG_ANY_MAX)) {
> + access_ok = 1;
> + }

You assign 1 in both branches, this seems suspicious.
The !is_root check is redundant.

> @@ -1888,69 +1959,10 @@
>
> - /*making sure user messages are received from hipd*/
> - //resetting vars to 0 because it is a loop
> - is_root = 0, access_ok = 0, msg_type = 0;
> - msg_type = hip_get_msg_type(msg);
> - is_root = (ntohs(sock_addr.sin6_port) < 1024);
> - if (is_root) {
> - access_ok = 1;
> - } else if (!is_root &&
> - (msg_type >= HIP_MSG_ANY_MIN &&
> - msg_type <= HIP_MSG_ANY_MAX)) {
> - access_ok = 1;
> - }

OMG, now I see where this comes from...

I removed all extra parentheses around complete expressions in HIPL.
Not sure how to handle the rest of this code that you copypasted.
Fixing it may be out of the scope of this branch, but it sure is not
pretty...

> --- test/firewall/conntrack.c 1970-01-01 00:00:00 +0000
> +++ test/firewall/conntrack.c 2011-03-27 15:51:27 +0000
> @@ -0,0 +1,126 @@
> ...

review: Needs Fixing

[Christof Mroz (christof-mroz)]

On Sun, 27 Mar 2011 19:36:27 +0200, Diego Biurrun <email address hidden> wrote:

> review needs-fixing
>
> On Sun, Mar 27, 2011 at 03:51:36PM +0000, Christof Mroz wrote:
>> Christof Mroz has proposed merging lp:~christof-mroz/hipl/hipfw-timeout
>> into lp:hipl.
>>
>> --- firewall/conntrack.c 2011-03-24 17:34:21 +0000
>> +++ firewall/conntrack.c 2011-03-27 15:51:27 +0000
>> @@ -74,8 +74,33 @@
>> +
>> +#define DEFAULT_CONNECTION_TIMEOUT (60 * 5); // 5 minutes
>> +#define DEFAULT_CLEANUP_INTERVAL (60 * 1); // 1 minute
>> +
>> +time_t cleanup_interval = DEFAULT_CLEANUP_INTERVAL;
>> +
>> +time_t connection_timeout = DEFAULT_CONNECTION_TIMEOUT;
>
> The defines seem completely redundant to me, just assign the values to
> the variables without indirection.

I just realized that only a fraction of HIPL does it that way, but it's
not consistent... so given the choice now, I'll drop that indirection.

> Not sure how to handle the rest of this code that you copypasted.
> Fixing it may be out of the scope of this branch, but it sure is not
> pretty...

If the hipfw <-> hipd link is supposed to be purely local, this part
should be rewritten using Domain sockets or FIFOs and proper access
control (also in hipd, of course). Would be nice if someone with more
authority could either decline this idea or file a feature request (or bug
report even?) as a reminder.

[lp:~christof-mroz/hipl/hipfw-timeout]

5795. By Christof Mroz on 2011-03-27

Various cosmetic changes in conntrack test suite.

5796. By Christof Mroz on 2011-03-27

Merged with trunk.

5797. By Christof Mroz on 2011-03-27

Remove preprocessor macro indirection for default value.

5798. By Christof Mroz on 2011-03-27

Document return value more specifically.

5799. By Christof Mroz on 2011-03-27

Move <check.h> in front of <signal.h>.

5800. By Christof Mroz on 2011-03-28

Separate includes from code by two blank lines.

5801. By Christof Mroz on 2011-03-29

Handle clock skews gracefully rather than using an assertion.

The unit tests were updated accordingly.

5802. By Christof Mroz on 2011-03-30

Document time(2) mock function in firewall/conntrack.c test suite.

5803. By Christof Mroz on 2011-03-30

Don't rely on GNU extensions in static array initializers.

5804. By Christof Mroz on 2011-03-31

Documentation typo.

5805. By Christof Mroz on 2011-04-03

Merged trunk revision 5813.

5806. By Christof Mroz on 2011-04-03

Merged mock-functions branch and updated tests accordingly.

5807. By Christof Mroz on 2011-04-03

Merge with lp:~christof-mroz/hipl/mock-functions/ revision 5816.

5808. By Christof Mroz on 2011-04-03

Merged with mock-functions branch.

5809. By Christof Mroz on 2011-04-03

Merged trunk rev 5816.

5810. By Christof Mroz on 2011-04-04

Merged lp:~christof-mroz/hipl/mock-functions rev 5826.

Unmerged revisions

=== modified file 'Makefile.am'
--- Makefile.am	2011-03-25 17:51:40 +0000
+++ Makefile.am	2011-03-27 19:21:33 +0000
@@ -128,7 +128,6 @@
 endif
 
 firewall_hipfw_sources = firewall/cache.c               \
-                         firewall/conntrack.c           \
                          firewall/dlist.c               \
                          firewall/esp_prot_api.c        \
                          firewall/esp_prot_config.c     \
@@ -159,6 +158,7 @@
 # To avoid duplicate symbols during linking some object files need to excluded.
 # Add all files that need to be excluded here.
 firewall_hipfw_SOURCES = $(firewall_hipfw_sources) \
+                         firewall/conntrack.c      \
                          firewall/main.c
 
 lib_core_libhipcore_la_SOURCES = lib/core/builder.c         \
@@ -198,6 +198,7 @@
 
 
 test_check_firewall_SOURCES = test/check_firewall.c          \
+                              test/firewall/conntrack.c      \
                               test/firewall/file_buffer.c    \
                               test/firewall/line_parser.c    \
                               test/firewall/port_bindings.c  \

=== modified file 'firewall/conntrack.c'
--- firewall/conntrack.c	2011-03-24 17:34:21 +0000
+++ firewall/conntrack.c	2011-03-27 19:21:33 +0000
@@ -74,8 +74,30 @@
 #include "reinject.h"
 
 
-static struct dlist *hip_list = NULL;
-static struct dlist *esp_list = NULL;
+static struct dlist *hip_list  = NULL;
+static struct dlist *esp_list  = NULL;
+static struct slist *conn_list = NULL;
+
+/**
+ * Interval between sweeps in hip_fw_conntrack_periodic_cleanup(),
+ * in seconds.
+ * Because all active connections are traversed, this should not be too
+ * low for performance reasons.
+ *
+ * @see hip_fw_conntrack_periodic_cleanup()
+ */
+time_t cleanup_interval = 60; // 1 minute
+
+/**
+ * Connection timeout in seconds, or zero to disable timeout.
+ * This actually specifies the minimum period of inactivity before a
+ * connection is considered stale. Thus, a connection may be inactive for
+ * at most ::connection_timeout plus ::cleanup_interval seconds before
+ * getting removed.
+ *
+ * @see hip_fw_conntrack_periodic_cleanup()
+ */
+time_t connection_timeout = 60 * 5; // 5 minutes
 
 enum {
     STATE_NEW,
@@ -84,9 +106,6 @@
     STATE_CLOSING
 };
 
-int           timeout_checking = 0;
-unsigned long timeout_value    = 0;
-
 /*------------print functions-------------*/
 /**
  * prints out the list of addresses of esp_addr_list
@@ -434,9 +453,8 @@
 
     connection = calloc(1, sizeof(struct connection));
 
-    connection->state = STATE_ESTABLISHED;
-    //set time stamp
-    gettimeofday(&connection->time_stamp, NULL);
+    connection->state     = STATE_ESTABLISHED;
+    connection->timestamp = time(NULL);
 #ifdef HIP_CONFIG_MIDAUTH
     connection->pisa_state = PISA_STATE_DISALLOW;
 #endif
@@ -467,6 +485,7 @@
     hip_list = append_to_list(hip_list, connection->original.hip_tuple);
     hip_list = append_to_list(hip_list, connection->reply.hip_tuple);
     HIP_DEBUG("inserting connection \n");
+    conn_list = append_to_slist(conn_list, connection);
 }
 
 /**
@@ -597,6 +616,8 @@
  */
 static void remove_connection(struct connection *connection)
 {
+    struct slist *conn_link;
+
     HIP_DEBUG("remove_connection: tuple list before: \n");
     print_tuple_list();
 
@@ -604,6 +625,10 @@
     print_esp_list();
 
     if (connection) {
+        HIP_ASSERT(conn_link = find_in_slist(conn_list, connection));
+        conn_list = remove_link_slist(conn_list, conn_link);
+        free(conn_link);
+
         remove_tuple(&connection->original);
         remove_tuple(&connection->reply);
 
@@ -1653,7 +1678,7 @@
         // update time_stamp only on valid packets
         // for new connections time_stamp is set when creating
         if (tuple->connection) {
-            gettimeofday(&tuple->connection->time_stamp, NULL);
+            tuple->connection->timestamp = time(NULL);
         } else {
             HIP_DEBUG("Tuple connection NULL, could not timestamp\n");
         }
@@ -1816,7 +1841,7 @@
 out_err:
     // if we are going to accept the packet, update time stamp of the connection
     if (err > 0) {
-        gettimeofday(&tuple->connection->time_stamp, NULL);
+        tuple->connection->timestamp = time(NULL);
     }
 
     HIP_DEBUG("verdict %d \n", err);
@@ -1957,3 +1982,49 @@
     HIP_DEBUG("get_tuple_by_hits: no connection found\n");
     return NULL;
 }
+
+/**
+ * Do some necessary bookkeeping concerning connection tracking.
+ * Currently, this only makes sure that stale locations will be removed.
+ * The actual tasks will be run at most once per ::connection_timeout
+ * seconds, no matter how often you call the function.
+ *
+ * @note Don't call this from a thread or timer, since most of hipfw is not
+ *       reentrant (and so this function isn't either).
+ */
+void hip_fw_conntrack_periodic_cleanup(void)
+{
+    static time_t      last_check = 0;   // timestamp of last call
+    struct slist      *iter_conn;
+    struct connection *conn;
+
+    if (connection_timeout == 0 || !filter_traffic) {
+        // timeout disabled, or no connections
+        // tracked in the first place
+        return;
+    }
+
+    const time_t now = time(NULL);
+
+    HIP_ASSERT(now >= last_check);
+    if (now - last_check >= cleanup_interval) {
+        HIP_DEBUG("Checking for connection timeouts\n");
+
+        iter_conn = conn_list;
+        while (iter_conn) {
+            conn      = iter_conn->data;
+            iter_conn = iter_conn->next; // iter_conn might get removed
+
+            HIP_ASSERT(now >= conn->timestamp);
+            if (now - conn->timestamp >= connection_timeout) {
+                HIP_DEBUG("Connection timed out:\n");
+                HIP_DEBUG_HIT("src HIT", &conn->original.hip_tuple->data->src_hit);
+                HIP_DEBUG_HIT("dst HIT", &conn->original.hip_tuple->data->dst_hit);
+
+                remove_connection(conn);
+            }
+        }
+
+        last_check = now;
+    }
+}

=== modified file 'firewall/conntrack.h'
--- firewall/conntrack.h	2011-01-04 14:10:46 +0000
+++ firewall/conntrack.h	2011-03-27 19:21:33 +0000
@@ -38,6 +38,10 @@
 
 
 /*-------------- CONNECTION TRACKING ------------*/
+
+extern time_t connection_timeout;
+extern time_t cleanup_interval;
+
 enum {
     ORIGINAL_DIR,
     REPLY_DIR,
@@ -59,4 +63,6 @@
                                 const struct in6_addr *dst_hit);
 int hipfw_relay_esp(const struct hip_fw_context *ctx);
 
+void hip_fw_conntrack_periodic_cleanup(void);
+
 #endif /* HIP_FIREWALL_CONNTRACK_H */

=== modified file 'firewall/firewall.c'
--- firewall/firewall.c	2011-03-27 17:28:34 +0000
+++ firewall/firewall.c	2011-03-27 19:21:33 +0000
@@ -1668,6 +1668,80 @@
 }
 
 /**
+ * Receive and process one message from hipd.
+ *
+ * @param msg A previously allocated message buffer.
+ * @return    Zero on success, -1 on error.
+ *
+ * @note The buffer @a msg is reused between calls because it is quite
+ *       large.
+ */
+static int hip_fw_handle_hipd_message(struct hip_common *const msg)
+{
+    struct sockaddr_in6 sock_addr;
+    int                 msg_type, len, n;
+    int                 is_root, access_ok;
+    socklen_t           alen;
+
+    alen = sizeof(sock_addr);
+    n    = recvfrom(hip_fw_async_sock, msg, sizeof(struct hip_common),
+                    MSG_PEEK, (struct sockaddr *) &sock_addr, &alen);
+    if (n < 0) {
+        HIP_ERROR("Error receiving message header from daemon.\n");
+        return -1;
+    }
+
+    // making sure user messages are received from hipd
+    access_ok = 0;
+    msg_type  = hip_get_msg_type(msg);
+    is_root   = ntohs(sock_addr.sin6_port) < 1024;
+    if (is_root) {
+        access_ok = 1;
+    } else if (!is_root &&
+               (msg_type >= HIP_MSG_ANY_MIN &&
+                msg_type <= HIP_MSG_ANY_MAX)) {
+        access_ok = 1;
+    }
+    if (!access_ok) {
+        HIP_ERROR("The sender of the message is not trusted.\n");
+        return -1;
+    }
+
+    alen = sizeof(sock_addr);
+    len  = hip_get_msg_total_len(msg);
+
+    HIP_DEBUG("Receiving message type %d (%d bytes)\n",
+              hip_get_msg_type(msg), len);
+    n = recvfrom(hip_fw_async_sock, msg, len, 0,
+                 (struct sockaddr *) &sock_addr, &alen);
+
+    if (n < 0) {
+        HIP_ERROR("Error receiving message parameters from daemon.\n");
+        return -1;
+    }
+
+    HIP_ASSERT(n == len);
+
+    if (ntohs(sock_addr.sin6_port) != HIP_DAEMON_LOCAL_PORT) {
+        int type = hip_get_msg_type(msg);
+        if (type == HIP_MSG_FW_BEX_DONE) {
+            HIP_DEBUG("HIP_MSG_FW_BEX_DONE\n");
+            HIP_DEBUG("%d == %d\n", ntohs(sock_addr.sin6_port),
+                      HIP_DAEMON_LOCAL_PORT);
+        }
+        HIP_DEBUG("Drop, message not from hipd\n");
+        return -1;
+    }
+
+    if (hip_handle_msg(msg) < 0) {
+        HIP_ERROR("Error handling message\n");
+        return -1;
+    }
+
+    return 0;
+}
+
+/**
  * Hipfw should be started before hipd to make sure
  * that nobody can bypass ACLs. However, some hipfw
  * extensions (e.g. userspace ipsec) work consistently
@@ -1723,17 +1797,14 @@
                const bool        kill_old,
                const bool        limit_capabilities)
 {
-    int                   err = 0, highest_descriptor, i;
-    int                   n, len;
+    int                   err       = 0, highest_descriptor, i;
     struct ipq_handle    *h4        = NULL, *h6 = NULL;
     struct hip_common    *msg       = NULL;
     struct sockaddr_in6   sock_addr = { 0 };
-    socklen_t             alen;
     fd_set                read_fdset;
     struct timeval        timeout;
     unsigned char         buf[HIP_MAX_PACKET];
     struct hip_fw_context ctx;
-    int                   is_root = 0, access_ok = 0, msg_type = 0; //variables for accepting user messages only from hipd
 
 #ifdef CONFIG_HIP_PERFORMANCE
     HIP_DEBUG("Creating perf set\n");
@@ -1888,69 +1959,10 @@
 
         if (FD_ISSET(hip_fw_async_sock, &read_fdset)) {
             HIP_DEBUG("****** Received HIPD message ******\n");
-            memset(&sock_addr, 0, sizeof(sock_addr));
-            alen = sizeof(sock_addr);
-            n    = recvfrom(hip_fw_async_sock, msg, sizeof(struct hip_common),
-                            MSG_PEEK, (struct sockaddr *) &sock_addr, &alen);
-            if (n < 0) {
-                HIP_ERROR("Error receiving message header from daemon.\n");
-                err = -1;
-                continue;
-            }
-
-
-            /*making sure user messages are received from hipd*/
-            //resetting vars to 0 because it is a loop
-            is_root  = 0, access_ok = 0, msg_type = 0;
-            msg_type = hip_get_msg_type(msg);
-            is_root  = ntohs(sock_addr.sin6_port) < 1024;
-            if (is_root) {
-                access_ok = 1;
-            } else if (!is_root &&
-                       (msg_type >= HIP_MSG_ANY_MIN &&
-                        msg_type <= HIP_MSG_ANY_MAX)) {
-                access_ok = 1;
-            }
-            if (!access_ok) {
-                HIP_ERROR("The sender of the message is not trusted.\n");
-                err = -1;
-                continue;
-            }
-
-            alen = sizeof(sock_addr);
-            len  = hip_get_msg_total_len(msg);
-
-            HIP_DEBUG("Receiving message type %d (%d bytes)\n",
-                      hip_get_msg_type(msg), len);
-            n = recvfrom(hip_fw_async_sock, msg, len, 0,
-                         (struct sockaddr *) &sock_addr, &alen);
-
-            if (n < 0) {
-                HIP_ERROR("Error receiving message parameters from daemon.\n");
-                err = -1;
-                continue;
-            }
-
-            HIP_ASSERT(n == len);
-
-            if (ntohs(sock_addr.sin6_port) != HIP_DAEMON_LOCAL_PORT) {
-                int type = hip_get_msg_type(msg);
-                if (type == HIP_MSG_FW_BEX_DONE) {
-                    HIP_DEBUG("HIP_MSG_FW_BEX_DONE\n");
-                    HIP_DEBUG("%d == %d\n", ntohs(sock_addr.sin6_port),
-                              HIP_DAEMON_LOCAL_PORT);
-                }
-                HIP_DEBUG("Drop, message not from hipd\n");
-                err = -1;
-                continue;
-            }
-
-            err = hip_handle_msg(msg);
-            if (err < 0) {
-                HIP_ERROR("Error handling message\n");
-                continue;
-            }
+            err = hip_fw_handle_hipd_message(msg);
         }
+
+        hip_fw_conntrack_periodic_cleanup();
     }
 
 out_err:

=== modified file 'firewall/firewall_defines.h'
--- firewall/firewall_defines.h	2011-01-11 13:59:46 +0000
+++ firewall/firewall_defines.h	2011-03-27 19:21:33 +0000
@@ -136,16 +136,16 @@
 };
 
 struct connection {
-    struct tuple   original;
-    struct tuple   reply;
-    int            verify_responder;
-    int            state;
-    struct timeval time_stamp;
+    struct tuple original;
+    struct tuple reply;
+    int          verify_responder;
+    int          state;
+    time_t       timestamp;
     /* members needed for ESP protection extension */
-    int     num_esp_prot_tfms;
-    uint8_t esp_prot_tfms[MAX_NUM_TRANSFORMS];
+    int          num_esp_prot_tfms;
+    uint8_t      esp_prot_tfms[MAX_NUM_TRANSFORMS];
 #ifdef CONFIG_HIP_MIDAUTH
-    int pisa_state;
+    int          pisa_state;
 #endif
 };
 

=== modified file 'firewall/hslist.c'
--- firewall/hslist.c	2010-12-13 19:09:27 +0000
+++ firewall/hslist.c	2011-03-27 19:21:33 +0000
@@ -129,3 +129,21 @@
 
     return list;
 }
+
+/**
+ * Find an element in the singly linked list.
+ *
+ * @param list The linked list.
+ * @param data The element to find.
+ * @return     The element in the linked list, or NULL if not found.
+ */
+struct slist *find_in_slist(struct slist *list, const void *const data)
+{
+    while (list) {
+        if (list->data == data) {
+            return list;
+        }
+        list = list->next;
+    }
+    return NULL;
+}

=== modified file 'firewall/hslist.h'
--- firewall/hslist.h	2010-12-13 19:09:27 +0000
+++ firewall/hslist.h	2011-03-27 19:21:33 +0000
@@ -32,4 +32,6 @@
 
 struct slist *remove_link_slist(struct slist *list, struct slist *link);
 
+struct slist *find_in_slist(struct slist *list, const void *const data);
+
 #endif /* HIP_FIREWALL_HSLIST_H */

=== modified file 'firewall/main.c'
--- firewall/main.c	2011-03-22 10:31:37 +0000
+++ firewall/main.c	2011-03-27 19:21:33 +0000
@@ -33,6 +33,8 @@
  *       Anything defined here will be unavailable in these cases.
  */
 
+#define _BSD_SOURCE
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -42,6 +44,7 @@
 #include "lib/core/debug.h"
 #include "lib/core/util.h"
 #include "firewall.h"
+#include "conntrack.h"
 
 
 /**
@@ -50,7 +53,7 @@
 static void hipfw_usage(void)
 {
     puts("HIP Firewall");
-    puts("Usage: hipfw [-f file_name] [-d|-v] [-A] [-F] [-H] [-b] [-a] [-c] [-k] [-i|-I|-e] [-l] [-o] [-p] [-h] [-V]");
+    puts("Usage: hipfw [-f file_name] [-d|-v] [-A] [-F] [-H] [-b] [-a] [-c] [-k] [-i|-I|-e] [-l] [-o] [-p] [-t <seconds>] [-h] [-V]");
 #ifdef CONFIG_HIP_MIDAUTH
     puts(" [-m]");
 #endif
@@ -69,6 +72,7 @@
     puts("      -e = use esp protection extension (also sets -i)");
     puts("      -l = activate lsi support");
     puts("      -p = run with lowered privileges. iptables rules will not be flushed on exit");
+    puts("      -t <seconds> = set timeout interval to <seconds>. Disable if <seconds> = 0");
     puts("      -h = print this help");
 #ifdef CONFIG_HIP_MIDAUTH
     puts("      -m = middlebox authentication");
@@ -97,13 +101,14 @@
     bool        limit_capabilities = false;
     const char *rule_file          = NULL;
 
-    int         ch;
+    char *end_of_number;
+    int   ch;
 
     /* Make sure that root path is set up correctly (e.g. on Fedora 9).
      * Otherwise may get warnings from system_print() commands. */
     setenv("PATH", HIP_DEFAULT_EXEC_PATH, 1);
 
-    while ((ch = getopt(argc, argv, "aAbcdef:FhHiIklmpvV")) != -1) {
+    while ((ch = getopt(argc, argv, "aAbcdef:FhHiIklmpt:vV")) != -1) {
         switch (ch) {
         case 'A':
             accept_hip_esp_traffic_by_default = 1;
@@ -154,6 +159,18 @@
         case 'p':
             limit_capabilities = 1;
             break;
+        case 't':
+            connection_timeout = strtoul(optarg, &end_of_number, 10);
+            if (end_of_number == optarg) {
+                fprintf(stderr, "Error: Invalid timeout given\n");
+                hipfw_usage();
+                return EXIT_FAILURE;
+            }
+            if (connection_timeout < cleanup_interval) {
+                /* we must poll at least once per timeout interval */
+                cleanup_interval = connection_timeout;
+            }
+            break;
         case 'v':
             log_level = LOGDEBUG_MEDIUM;
             hip_set_logfmt(LOGFMT_SHORT);
@@ -172,6 +189,11 @@
         }
     }
 
+    if (connection_timeout > 0 && !filter_traffic) {
+        puts("Warning: timeouts (-t) have no effect with connection");
+        puts("         tracking disabled (-F)");
+    }
+
     if (geteuid() != 0) {
         HIP_ERROR("Firewall must be run as root\n");
         exit(-1);

=== modified file 'test/check_firewall.c'
--- test/check_firewall.c	2011-02-18 14:39:38 +0000
+++ test/check_firewall.c	2011-03-27 19:21:33 +0000
@@ -32,6 +32,7 @@
 {
     int      number_failed;
     SRunner *sr = srunner_create(firewall_file_buffer());
+    srunner_add_suite(sr, firewall_conntrack());
     srunner_add_suite(sr, firewall_line_parser());
     srunner_add_suite(sr, firewall_port_bindings());
 

=== added file 'test/firewall/conntrack.c'
--- test/firewall/conntrack.c	1970-01-01 00:00:00 +0000
+++ test/firewall/conntrack.c	2011-03-27 19:21:33 +0000
@@ -0,0 +1,126 @@
+/*
+ * Copyright (c) 2010 Aalto University and RWTH Aachen University.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#define _BSD_SOURCE
+
+#include <net/if.h>
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <netinet/ip.h>
+#include <check.h>
+#include <signal.h>
+#include <time.h>
+
+#include "firewall/conntrack.h"
+#include "firewall/conntrack.c"
+#include "test_suites.h"
+
+static time_t fake_time = 0;
+
+time_t time(time_t *t)
+{
+    if (t) {
+        *t = fake_time;
+    }
+
+    return fake_time;
+}
+
+static struct connection *setup_connection(void)
+{
+    struct hip_data data = {};
+
+    inet_pton(AF_INET6, "2001:12:bd2d:d23e:4a09:b2ab:6414:e110", &data.src_hit);
+    inet_pton(AF_INET6, "2001:10:f039:6bc5:cab3:0727:7fbc:9dcb", &data.dst_hit);
+
+    insert_new_connection(&data);
+
+    fail_if(conn_list       == NULL, "No connection inserted.");
+    fail_if(conn_list->next != NULL, "More than one connection inserted.");
+    fail_if(conn_list->data == NULL, "No connection allocated.");
+    return conn_list->data;
+}
+
+START_TEST(test_hip_fw_conntrack_periodic_cleanup_timeout)
+{
+    struct connection *conn;
+
+    cleanup_interval   = 0;
+    connection_timeout = 2;
+
+    conn            = setup_connection();
+    conn->timestamp = 1;
+
+    fake_time = 2;
+    hip_fw_conntrack_periodic_cleanup(); // don't time out yet
+    fail_if(conn_list == NULL, "Connection was removed too early.");
+
+    fake_time = 3;
+    hip_fw_conntrack_periodic_cleanup(); // time out this time
+    fail_unless(conn_list == NULL, "Idle connection was not removed.");
+}
+END_TEST
+
+START_TEST(test_hip_fw_conntrack_periodic_cleanup_glitched_system_time)
+{
+    cleanup_interval = 0;
+
+    fake_time = 2;
+    hip_fw_conntrack_periodic_cleanup(); // OK
+
+    fake_time = 1;
+    hip_fw_conntrack_periodic_cleanup(); // throws assertion
+}
+END_TEST
+
+START_TEST(test_hip_fw_conntrack_periodic_cleanup_glitched_packet_time)
+{
+    struct connection *conn;
+
+    cleanup_interval   = 0;
+    connection_timeout = 2;
+
+    fake_time = 1;
+
+    conn            = setup_connection();
+    conn->timestamp = 1;
+    hip_fw_conntrack_periodic_cleanup(); // OK
+    conn->timestamp = 2;
+    hip_fw_conntrack_periodic_cleanup(); // throws assertion
+}
+END_TEST
+
+Suite *firewall_conntrack(void)
+{
+    Suite *s = suite_create("firewall/conntrack");
+
+    TCase *tc_conntrack = tcase_create("Conntrack");
+    tcase_add_test(tc_conntrack, test_hip_fw_conntrack_periodic_cleanup_timeout);
+    tcase_add_exit_test(tc_conntrack, test_hip_fw_conntrack_periodic_cleanup_glitched_system_time, 1);
+    tcase_add_exit_test(tc_conntrack, test_hip_fw_conntrack_periodic_cleanup_glitched_packet_time, 1);
+    suite_add_tcase(s, tc_conntrack);
+
+    return s;
+}

=== modified file 'test/firewall/test_suites.h'
--- test/firewall/test_suites.h	2011-02-18 14:39:38 +0000
+++ test/firewall/test_suites.h	2011-03-27 19:21:33 +0000
@@ -28,6 +28,7 @@
 
 #include <check.h>
 
+Suite *firewall_conntrack(void);
 Suite *firewall_file_buffer(void);
 Suite *firewall_line_parser(void);
 Suite *firewall_port_bindings(void);