review needs-fixing On Wed, Oct 19, 2011 at 01:17:30PM +0000, René Hummen wrote: > > For more details, see: > https://code.launchpad.net/~midauth-pisa-devs/hipl/midauth-firewall/+merge/79811 > > This branch re-implements the hipfw-related part of http://tools.ietf.org/html/draft-heer-hip-middle-auth-02. > > Short intro to HIP middlebox authentication: > 1) The firewall intercepts R1, I2, U1 and U2 messages and adds a nonce that needs to be echoed back to the firewall by the respective end-host in the signed part of the reply. This enables the firewall to evaluate the freshness of the exchange. > 2) In order to mitigate DoS attacks targeting the signature verification at the firewall, the nonce is additionally used as the input for a puzzle that the end-host needs to solve and send back to the firewall in the reply as well. > -- > https://code.launchpad.net/~midauth-pisa-devs/hipl/midauth-firewall/+merge/79811 > Your team HIPL core team is requested to review the proposed merge of lp:~midauth-pisa-devs/hipl/midauth-firewall into lp:hipl. > --- Makefile.am 2011-10-17 18:14:10 +0000 > +++ Makefile.am 2011-10-19 13:13:48 +0000 > @@ -253,7 +256,7 @@ > > -test_check_firewall_LDFLAGS = -ldl > +test_check_firewall_LDFLAGS = -ldl -z muldefs We have solved this differently elsewhere by #including the .c file to unit-test static functions. > --- firewall/conntrack.c 2011-10-19 09:51:41 +0000 > +++ firewall/conntrack.c 2011-10-19 13:13:48 +0000 > @@ -998,7 +998,7 @@ > * > * @param common the hip packet to be verified > * @param tuple the corresponding connection tuple > - * @return 1 for valid packets, 0 otherwise > + * @return 0 for valid packets, 1 otherwise > */ > static int hip_fw_verify_packet(struct hip_common *const common, > const struct tuple *const tuple) This looks like a change that could be separated. > @@ -1526,14 +1565,28 @@ > > } else if (esp_info && seq && ack) { > - if (!tuple) { > + if (tuple && tuple->direction == ORIGINAL_DIR) { > + other_dir = &tuple->connection->reply; > + } else if (tuple) { > + other_dir = &tuple->connection->original; > + } else { > HIP_ERROR("Insufficient stored state to process UPDATE\n"); > return 0; > } tuple is checked twice. > - if (!hip_fw_verify_packet(common, tuple)) { > + if (!hipfw_midauth_verify_challenge(ctx, other_dir->midauth_nonce)) { > + HIP_ERROR("failed to verify midauth challenge\n"); > + return 0; > + } 0 is returned in case of error? > @@ -1543,14 +1596,29 @@ > } else if (ack) { > - if (!tuple) { > + if (tuple && tuple->direction == ORIGINAL_DIR) { > + other_dir = &tuple->connection->reply; > + } else if (tuple) { > + other_dir = &tuple->connection->original; > + } else { > HIP_ERROR("Insufficient stored state to process UPDATE\n"); > return 0; > } another redundant check Oh, this code seems to be completely duplicated. > @@ -1842,7 +1910,7 @@ > > -#ifdef CONFIG_HIP_MIDAUTH > +#ifdef HIP_CONFIG_MIDAUTH > if (use_midauth && tuple->connection->pisa_state == PISA_STATE_DISALLOW) { > HIP_DEBUG("PISA: ESP unauthorized -> dropped\n"); > err = 0; This is wrong. > --- firewall/firewall.c 2011-08-16 07:49:25 +0000 > +++ firewall/firewall.c 2011-10-19 13:13:48 +0000 > @@ -76,12 +76,13 @@ > #include "lib/core/prefix.h" > #include "lib/core/util.h" > #include "hipd/hipd.h" > -#include "config.h" > #include "cache.h" > #include "common_types.h" > +#include "config.h" > #include "conntrack.h" > #include "esp_prot_api.h" > #include "esp_prot_conntrack.h" > +#include "firewall.h" > #include "firewall_control.h" > #include "firewall_defines.h" > #include "helpers.h" > @@ -90,9 +91,9 @@ > #include "pisa.h" > #include "port_bindings.h" > #include "reinject.h" > +#include "rewrite.h" > #include "rule_management.h" > #include "user_ipsec_api.h" > -#include "firewall.h" The ordering is actually done on purpose. config.h is not in the same directory, same as the other #includes with directory prefixes and firewall.h directly "belongs" to firewall.c, so it made sense at the time to put it last and list all dependencies before. I have this feeling in the back of my head that the latter solved some real issue. > --- firewall/firewall_defines.h 2011-09-07 13:33:42 +0000 > +++ firewall/firewall_defines.h 2011-10-19 13:13:48 +0000 > @@ -50,6 +50,13 @@ > > +/** > + * > + * @note When adding new members, check if hip_fw_context_enable_write() needs > + * to be updated as well. nit: stray empty line > @@ -74,6 +81,8 @@ > int modified; > }; > > +#include "midauth.h" Why the peculiar placement of this #include? > --- firewall/midauth.c 2011-08-15 14:11:56 +0000 > +++ firewall/midauth.c 2011-10-19 13:13:48 +0000 > @@ -38,416 +38,246 @@ > > +/** > + * Add a CHELLENGE_REQUEST parameter to a HIP packet passing through the ch_a_llenge > + * @param ctx The packet context. > + * @param[out] midauth_nonce The nonce added by the firewall for this handshake. > + * @return one on success, zero otherwise I'd write actual numbers in the @return for consistency. > + struct hip_challenge_request request; > + static const size_t min_length = sizeof(request) > + - sizeof(request.tlv) > + - sizeof(request.opaque); nit: maybe slightly more readable: static const size_t min_length = sizeof(request) - sizeof(request.tlv) - sizeof(request.opaque); > + if (use_midauth) { > + HIP_ASSERT(midauth_nonce); > + HIP_ASSERT(ctx); > + HIP_ASSERT(ctx->packet_type == HIP_PACKET); Why are these asserts not at the top of the function like everywhere else? > + // fatalness depends on current settings fatality > + /* store nonce per-connection per-peer */ > + memcpy(midauth_nonce, request.opaque, MIDAUTH_DEFAULT_NONCE_LENGTH); This cannot be done via assignment? No, I did not bother to check... > +/** > + * Helper function for verifying a CHALLENGE_RESPONSE parameter. > + * > + * @param ctx The packet context. > + * @param response The CHALLENGE_RESPONSE parameter. > + * @param midauth_nonce The stored nonce for this association. > + * @return ERROR on validation error, > + * RESPONSE_CORRECT if validation was successful, > + * RESPONSE_INCORRECT if nonces match but solution is incorrect, and > + * RESPONSE_NO_MATCH if no matching nonces are stored. > + */ > +static enum { > + ERROR, > + RESPONSE_CORRECT, > + RESPONSE_INCORRECT, > + RESPONSE_NO_MATCH > +} > +verify_response(const struct hip_fw_context *const ctx, > + const struct hip_challenge_response *const response, > + const uint8_t midauth_nonce[MIDAUTH_DEFAULT_NONCE_LENGTH]) Why is the enum static and anonymous? It's also strange to place it between function declaration and function doxygen. > + const bool match = (len == MIDAUTH_DEFAULT_NONCE_LENGTH) && > + (memcmp(response->opaque, midauth_nonce, > + MIDAUTH_DEFAULT_NONCE_LENGTH) == 0); two pointless sets of parentheses. > + memcpy(tmp_puzzle.solution, response->J, PUZZLE_LENGTH); see above about assignment > +int hipfw_midauth_verify_challenge(const struct hip_fw_context *const ctx, > + const uint8_t midauth_nonce[MIDAUTH_DEFAULT_NONCE_LENGTH]) missing doxy > + bool found = false; > + do { > + switch (verify_response(ctx, response, midauth_nonce)) { > + case RESPONSE_CORRECT: > + HIP_DEBUG("Correct CHALLENGE_RESPONSE found\n"); > + return 1; > + > + case RESPONSE_INCORRECT: > + HIP_ERROR("Incorrect CHALLENGE_RESPONSE found\n"); > + break; > + > + case RESPONSE_NO_MATCH: > + break; > + > + default: > + HIP_ERROR("Unable to compute challenge verification.\n"); > + } We usually don't have blank lines between case statements. > + if (!found) { > + HIP_ERROR("CHALLENGE_RESPONSE found, but no matching nonce\n"); > + return ignore_missing_challenge_response ? 1 : 0; > + } > + > + return found ? 1 : 0; You just checked found, no need to do it again. > --- firewall/pisa.h 2011-08-15 14:11:56 +0000 > +++ firewall/pisa.h 2011-10-19 13:13:48 +0000 > @@ -31,12 +31,14 @@ > > +#ifdef CONFIG_HIP_PISA > /** > * Register PISA handlers with midauth and initialize data structures. > * > * @param h pointer to the handlers > */ > void pisa_init(struct midauth_handlers *h); > +#endif This is unnecessary. > --- firewall/rewrite.c 1970-01-01 00:00:00 +0000 > +++ firewall/rewrite.c 2011-10-19 13:13:48 +0000 > @@ -0,0 +1,362 @@ > +/* > + * Copyright (c) 2010 Aalto University and RWTH Aachen University. Happy new year! > +#include > +#include > +#include > +#include > +#include > +#include nit: #includes are ordered elsewhere. > +#include "firewall/rewrite.h" Drop the directory prefix. > +static void *rebase(const void *const field, > + const void *const old, > + void *const new) > +{ > + return (char *) new + ((const char *) field - (const char *) old); pointless parentheses > + * @param ctx The current packet context. The ipq header may have been modified > + * but should be consistent, especially the data_len field. > + * > + * > + * @note It is safe to call this function on the same @a ctx multiple times: the > + * packet will not needlessly be copied again. > + */ nit: stray extra empty line > +/** > + * Add a new parameter to the correct position in the packet. Parameters are > + * ordered by type number. Hence, some parameters might need to be moved in > + * order for the new parameter to find into the right position. fin into the position? > + while ((current = hip_get_next_param_readwrite(hip, current))) { > + if (hip_get_param_type(current) >= param_type) { > + uint8_t *const splice = (uint8_t *const) current; > + > + memmove(splice + param_len, splice, end - splice); > + out = splice; > + break; The splice variable looks unnecessary. > + } else { > + const struct sockaddr_in src = { .sin_family = AF_INET, > + .sin_addr = ipv4->ip_src }; > + const struct sockaddr_in dst = { .sin_family = AF_INET, > + .sin_addr = ipv4->ip_dst }; > + > + HIP_ASSERT(ipv4->ip_p == IPPROTO_HIP); > + > + hip_zero_msg_checksum(hip); > + hip->checksum = hip_checksum_packet((char *) hip, > + (const struct sockaddr *) &src, > + (const struct sockaddr *) &dst); Why not use the right types to begin with instead of casting? > --- firewall/rewrite.h 1970-01-01 00:00:00 +0000 > +++ firewall/rewrite.h 2011-10-19 13:13:48 +0000 > @@ -0,0 +1,39 @@ > +/* > + * Copyright (c) 2010 Aalto University and RWTH Aachen University. Happy new year! > --- lib/tool/checksum.c 2011-08-15 14:11:56 +0000 > +++ lib/tool/checksum.c 2011-10-19 13:13:48 +0000 > @@ -219,23 +222,23 @@ > -uint16_t hip_checksum_packet(char *data, struct sockaddr *src, > - struct sockaddr *dst) > +uint16_t hip_checksum_packet(char *data, const struct sockaddr *src, > + const struct sockaddr *dst) > { > - uint16_t checksum = 0; > - unsigned long sum = 0; > - int count = 0, length = 0; > - unsigned short *p = NULL; /* 16-bit */ > - struct pseudo_header pseudoh; > - struct pseudo_header6 pseudoh6; > - uint32_t src_network, dst_network; > - struct in6_addr *src6, *dst6; > - struct hip_common *hiph = (struct hip_common *) data; > + uint16_t checksum = 0; > + unsigned long sum = 0; > + int count = 0, length = 0; > + unsigned short *p = NULL; /* 16-bit */ > + struct pseudo_header pseudoh; > + struct pseudo_header6 pseudoh6; > + uint32_t src_network, dst_network; > + const struct in6_addr *src6, *dst6; > + struct hip_common *hiph = (struct hip_common *) data; > > if (src->sa_family == AF_INET) { > /* IPv4 checksum based on UDP-- Section 6.1.2 */ > - src_network = ((struct sockaddr_in *) src)->sin_addr.s_addr; > - dst_network = ((struct sockaddr_in *) dst)->sin_addr.s_addr; > + src_network = ((const struct sockaddr_in *) src)->sin_addr.s_addr; > + dst_network = ((const struct sockaddr_in *) dst)->sin_addr.s_addr; This looks like an unrelated const-correctness change, push right away. > @@ -248,8 +251,8 @@ > } else { > /* IPv6 checksum based on IPv6 pseudo-header */ > - src6 = &((struct sockaddr_in6 *) src)->sin6_addr; > - dst6 = &((struct sockaddr_in6 *) dst)->sin6_addr; > + src6 = &((const struct sockaddr_in6 *) src)->sin6_addr; > + dst6 = &((const struct sockaddr_in6 *) dst)->sin6_addr; more > --- test/firewall/midauth.c 1970-01-01 00:00:00 +0000 > +++ test/firewall/midauth.c 2011-10-19 13:13:48 +0000 > @@ -0,0 +1,298 @@ > + > +#ifdef HAVE_TCASE_ADD_EXIT_TEST > +START_TEST(test_hipfw_midauth_add_challenge_NULL_ctx) > +{ > +} > +END_TEST > +#endif /* HAVE_TCASE_ADD_EXIT_TEST */ > + > +START_TEST(test_hip_challenge_response_opaque_len) > +{ > +} > +END_TEST > + > +#ifdef HAVE_TCASE_ADD_EXIT_TEST > +START_TEST(test_hip_challenge_response_opaque_len_NULL) > +{ > +#endif /* HAVE_TCASE_ADD_EXIT_TEST */ You could save one set of #ifdefs by reshuffling the functions. > --- test/firewall/rewrite.c 1970-01-01 00:00:00 +0000 > +++ test/firewall/rewrite.c 2011-10-19 13:13:48 +0000 > @@ -0,0 +1,151 @@ > + > +#define _BSD_SOURCE > + > +#include > +#include > +#include > + > +#include "lib/core/common.h" > +#include "firewall/firewall_defines.h" nit: order Diego