Code review comment for lp:~christof-mroz/hipl/hipfw-timeout

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