Merge into trunk : hipfw-timeout : Code : HIPL

Status:	Superseded
Proposed branch:	lp:~christof-mroz/hipl/hipfw-timeout
Merge into:	lp:hipl
Diff against target:	693 lines (+353/-88) 11 files modified Makefile.am (+2/-1) firewall/conntrack.c (+86/-10) firewall/conntrack.h (+6/-0) firewall/firewall.c (+78/-66) firewall/firewall_defines.h (+8/-8) firewall/hslist.c (+18/-0) firewall/hslist.h (+2/-0) firewall/main.c (+25/-3) test/check_firewall.c (+1/-0) test/firewall/conntrack.c (+126/-0) test/firewall/test_suites.h (+1/-0)
To merge this branch:	bzr merge lp:~christof-mroz/hipl/hipfw-timeout
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Diego Biurrun		Needs Fixing on 2011-03-24
Stefan Götz (community)	2011-03-22	Approve on 2011-03-22
René Hummen	2011-03-22	Pending
Review via email: mp+54339@code.launchpad.net

Revision history for this message

Stefan Götz (stefan.goetz-deactivatedaccount) wrote on 2011-03-18: Posted in a previous version of this proposal

#

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

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 <stefan.goetz@cs.rwth-aachen.de> 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

Revision history for this message

René Hummen (rene-hummen) wrote on 2011-03-22: Posted in a previous version of this proposal

#

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

review: Needs Fixing

Revision history for this message

Christof Mroz (christof-mroz) wrote on 2011-03-22: Posted in a previous version of this proposal

#

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

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

lp:~christof-mroz/hipl/hipfw-timeout updated on 2011-03-22

5783. By Christof Mroz on 2011-03-22: Removed spurious newline in usage text.
5784. By Christof Mroz on 2011-03-22: Fixed compilation of firewall tests with midauth enabled.

Revision history for this message

Stefan Götz (stefan.goetz-deactivatedaccount) wrote on 2011-03-22:

#

Download full text (5.8 KiB)

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...

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 unprivileged applications.

[L] unrelated: Since the first 'recvfrom()' was a 'PEEK', we know that the sender address in the first and second call to 'recvfrom()' are the same. Thus, no need to retrieve 'sock_addr' a second time.

> +
> +    if (n < 0) {
> +        HIP_ERROR("Error receiving message parameters from daemon.\n");
> +        return -1;
> +    }
> +
> +    HIP_ASSERT(n == len);

[H] unrelated: A perfect example how 'assert()' should not be used :-( Any unprivileged user can make the firewall terminate by sending a message that passes all the above checks but triggers this assertion. Such a message should be discarded instead.

> +
> +    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);
> +        }

[M] unrelated: There seems little point in having this inner if clause with only debug output. Should be removed. If not, it should be documented why it makes sense to accept this particular message from other ports than 'HIP_DAEMON_LOCAL_PORT'.

> +        HIP_DEBUG("Drop, message not from hipd\n");
> +        return -1;
> +    }

[M] unrelated: As far as I can tell, this whole check for 'HIP_DAEMON_LOCAL_PORT' can and should be done right after the first 'recvfrom()'

[M] unrelated: Also note how the check for 'HIP_DAEMON_LOCAL_PORT' is stricter than the check for a privileged port abover. It seems to me that only the latter check for 'HIP_DAEMON_LOCAL_PORT' is really relevant and effective so the other check is superfluous.

> +
> +    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

> === modified file 'firewall/hslist.c'
> --- firewall/hslist.c	2010-12-13 19:09:27 +0000
> +++ firewall/hslist.c	2011-03-22 12:49:26 +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)

[M] would 'const struct slist *list' work for better const correctness?

> +{
> +    while (list) {
> +        if (list->data == data) {
> +            return list;
> +        }
> +        list = list->next;
> +    }
> +    return NULL;
> +}
>

Stefan

review: Needs Fixing

lp:~christof-mroz/hipl/hipfw-timeout updated on 2011-03-22

5785. By Christof Mroz on 2011-03-22: Const-correctness fix for hip_fw_handle_hipd_message().

Revision history for this message

Christof Mroz (christof-mroz) wrote on 2011-03-22:

#

> > +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.

Revision history for this message

Stefan Götz (stefan.goetz-deactivatedaccount) wrote on 2011-03-22:

#

> > [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

Revision history for this message

Diego Biurrun (diego-biurrun) wrote on 2011-03-24:

#

Download full text (3.3 KiB)

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

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 +0000
> @@ -31,5 +31,6 @@
>  Suite *firewall_file_buffer(void);
>  Suite *firewall_line_parser(void);
>  Suite *firewall_port_bindings(void);
> +Suite *firewall_conntrack(void);

nit: alphabetical order

Diego

review: Needs Fixing

lp:~christof-mroz/hipl/hipfw-timeout updated on 2011-03-24

5786. By Christof Mroz on 2011-03-24: Cosmetic: Fixed brace alignment.
5787. By Christof Mroz on 2011-03-24: Cosmetic: add test suites in alphabetical order.
5788. By Christof Mroz on 2011-03-24: Cosmetic: list check_firewall sources in alphabetical order.
5789. By Christof Mroz on 2011-03-24: Cosmetic: list firewall test suites in alphabetical order, also in header file.

Revision history for this message

Christof Mroz (christof-mroz) wrote on 2011-03-24:

#

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.

On Thu, 24 Mar 2011 15:27:30 +0100, Diego Biurrun <diego@biurrun.de> 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.

Revision history for this message

Diego Biurrun (diego-biurrun) wrote on 2011-03-24:

#

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

Revision history for this message

Christof Mroz (christof-mroz) wrote on 2011-03-24:

#

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.

Revision history for this message

Diego Biurrun (diego-biurrun) wrote on 2011-03-24:

#

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

Revision history for this message

Christof Mroz (christof-mroz) wrote on 2011-03-24:

#

Download full text (3.1 KiB)

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...

HIPL

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers

 === modified file 'Makefile.am'
 --- Makefile.am	2011-03-25 17:51:40 +0000
 +++ Makefile.am	2011-03-27 15:47:24 +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 15:47:24 +0000
@@ -74,8 +74,33 @@
  #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;
++
++#define DEFAULT_CONNECTION_TIMEOUT (60 * 5); // 5 minutes
++#define DEFAULT_CLEANUP_INTERVAL   (60 * 1); // 1 minute
++
++/**
++ * 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 = DEFAULT_CLEANUP_INTERVAL;
++
++/**
++ * 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 = DEFAULT_CONNECTION_TIMEOUT;
  enum {
      STATE_NEW,
@@ -84,9 +109,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 +456,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 +488,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 +619,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 +628,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 +1681,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 +1844,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 +1985,51 @@
      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.
++ *
++ * @param now The current time.
++ *
++ * @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 15:47:24 +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-24 11:35:48 +0000
 +++ firewall/firewall.c	2011-03-27 15:47:24 +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.
++ *
++ * @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
++    // resetting vars to 0 because it is a loop
++    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 15:47:24 +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 15:47:24 +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 15:47:24 +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 15:47:24 +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 15:47:24 +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 15:47:24 +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 <check.h>
++
++#include <net/if.h>
++#include <arpa/inet.h>
++#include <netinet/in.h>
++#include <netinet/ip.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 15:47:24 +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);