From: Alan T. DeKok Date: Sat, 29 Jun 2024 16:05:04 +0000 (-0400) Subject: implement and document "limit_proxy_state = auto" X-Git-Tag: release_3_0_27~8 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=accb06ac915db351c17e02711eaeebb05fda9371;p=thirdparty%2Ffreeradius-server.git implement and document "limit_proxy_state = auto" Also add a standard function which complains loudly about security issues. --- diff --git a/raddb/radiusd.conf.in b/raddb/radiusd.conf.in index 1ba6a8a6a4..2188b70120 100644 --- a/raddb/radiusd.conf.in +++ b/raddb/radiusd.conf.in @@ -647,17 +647,14 @@ security { require_message_authenticator = yes # - # Global configuration for requiring Message-Authenticator - # Access-Request packets from a NAS, but only if those - # packets also contain Proxy-State. This flag only applies - # to packets sent over UDP or TCP. This flag is ignored for - # TLS. + # Global configuration for limiting the combination of + # Proxy-State and Message-Authenticator. This flag only + # applies to packets sent over UDP or TCP. This flag is + # ignored for TLS. # # This flag sets the global default for all clients. It can # be over-ridden in an individual client definition by adding - # a flag to that section: - # - # limit_proxy_state = no + # the same flag to that section with an appropriate value: # # If "require_message_authenticator" is set to "yes", this # configuration item is ignored. @@ -665,22 +662,62 @@ security { # If "require_message_authenticator" is set to "no", this # configuration item is checked. # - # This configuration item should ALWAYS be set to "yes". - # - # The only reason to set it to "no" is when the client is a - # proxy, AND the proxy does not send Message-Authenticator in - # Access-Request packets. Even then, the best approach to - # fix the issue is to (1) update the proxy to send - # Message-Authenticator, and if that can't be done, then (2) - # set this flag to "no", but ONLY on a per-client basis. - # - # WARNING: Setting both this flag and the - # "require_message_authenticator" flag to "no" will allow - # MITM attackers to create fake Access-Accept packets to the - # NAS! At least one of them MUST be set to "yes" for the - # system to have any protection against the attack. - # - limit_proxy_state = yes + # The possible values and meanings for "limit_proxy_state" are; + # + # * "no" - allow any packets from the client, even packets + # which contain the BlastRADIUS attack. Please be aware + # that in this configuration the server will complain for + # EVERY packet which it receives. + # + # The only reason to set this flag to "no" is when the + # client is a proxy, AND the proxy does not send + # Message-Authenticator in Access-Request packets. Even + # then, the best approach to fix the issue is to (1) update + # the proxy to send Message-Authenticator, and if that + # can't be done, then (2) set this flag to "no", but ONLY + # for that one client. The global flag SHOULD still be set + # to a safe value: "yes". + # + # WARNING: Setting both this flag and the + # "require_message_authenticator" flag to "no" will allow + # MITM attackers to create fake Access-Accept packets to the + # NAS! At least one of them MUST be set to "yes" for the + # system to have any protection against the attack. + # + # * "yes" - Allow packets without Message-Authenticator, + # but only when they do not contain Proxy-State. + # packets which contain Proxy-State MUST also contain + # Message-Authenticator, otherwise they are discarded. + # + # This setting is safe for all NASes, GGSNs, BRAS, etc. + # No known RADIUS client sends Proxy-State for normal + # Access-Request packets. + # + # * "auto" - Automatically determine the value of the flag, + # based on the first packet received from that client. + # + # If the packet contains Proxy-State but no + # Message-Authenticator, then the value of the flag is + # automatically switched to "no". + # + # For all other situations, the value of the flag is + # automatically switched to "yes". + # + # WARNING: This switch is done for the first packet + # received from that client. The change does NOT persist + # across server restarts. You MUST change the to "yes" + # manually, in order to make a permanent change to the + # configuration. + # + # WARNING: If there are multiple NASes with the same source + # IP and client definitions, BUT the NASes have different + # behavior, then this flag WILL LIKELY BREAK YOUR NETWORK. + # + # The only solution to that rare configuration is to set + # this flag to "no", in which case the network will work, + # but will be vulnerable to the attack. + # + limit_proxy_state = auto @openssl_version_check_config@ } diff --git a/src/include/clients.h b/src/include/clients.h index e7601f3aa5..a9b66429cc 100644 --- a/src/include/clients.h +++ b/src/include/clients.h @@ -26,6 +26,13 @@ * @copyright 2015 The FreeRADIUS server project */ +typedef enum { + FR_BOOL_FALSE = 0, + FR_BOOL_TRUE, + FR_BOOL_AUTO, +} fr_bool_auto_t; + + typedef struct radclient_list RADCLIENT_LIST; @@ -45,7 +52,7 @@ typedef struct radclient { bool require_ma; //!< Require RADIUS message authenticator in requests. - bool limit_proxy_state; //!< Limit Proxy-State in requests + fr_bool_auto_t limit_proxy_state; //!< Limit Proxy-State in requests char const *nas_type; //!< Type of client (arbitrary). diff --git a/src/include/libradius.h b/src/include/libradius.h index b5d1db81bd..3faab756b5 100644 --- a/src/include/libradius.h +++ b/src/include/libradius.h @@ -408,6 +408,10 @@ typedef struct radius_packet { int proto; #endif bool tls; //!< uses secure transport + + bool message_authenticator; + bool proxy_state; + bool eap_message; } RADIUS_PACKET; typedef enum { diff --git a/src/include/radiusd.h b/src/include/radiusd.h index e1d90f5b99..36e5741158 100644 --- a/src/include/radiusd.h +++ b/src/include/radiusd.h @@ -176,7 +176,7 @@ typedef struct main_config { bool require_ma; //!< global configuration for all clients and home servers - bool limit_proxy_state; //!< global configuration for all clients + fr_bool_auto_t limit_proxy_state; //!< global configuration for all clients #ifdef ENABLE_OPENSSL_VERSION_CHECK char const *allow_vulnerable_openssl; //!< The CVE number of the last security issue acknowledged. @@ -565,6 +565,8 @@ int main_config_free(void); void main_config_hup(void); void hup_logfile(void); +int fr_bool_auto_parse(CONF_PAIR *cp, fr_bool_auto_t *out, char const *str); + /* listen.c */ void listen_free(rad_listen_t **head); int listen_init(CONF_SECTION *cs, rad_listen_t **head, bool spawn_flag); diff --git a/src/lib/radius.c b/src/lib/radius.c index 10e40bac5e..0962d3d6f7 100644 --- a/src/lib/radius.c +++ b/src/lib/radius.c @@ -2680,6 +2680,7 @@ bool rad_packet_ok(RADIUS_PACKET *packet, int flags, decode_fail_t *reason) case PW_EAP_MESSAGE: require_ma = true; eap = true; + packet->eap_message = true; break; case PW_USER_PASSWORD: @@ -2690,6 +2691,7 @@ bool rad_packet_ok(RADIUS_PACKET *packet, int flags, decode_fail_t *reason) case PW_PROXY_STATE: seen_proxy_state = true; + packet->proxy_state = true; break; case PW_MESSAGE_AUTHENTICATOR: @@ -2703,6 +2705,7 @@ bool rad_packet_ok(RADIUS_PACKET *packet, int flags, decode_fail_t *reason) goto finish; } seen_ma = true; + packet->message_authenticator = true; break; } diff --git a/src/main/client.c b/src/main/client.c index bbc4613412..027839b4f6 100644 --- a/src/main/client.c +++ b/src/main/client.c @@ -491,6 +491,7 @@ static fr_ipaddr_t cl_ipaddr; static uint32_t cl_netmask; static char const *cl_srcipaddr = NULL; static char const *hs_proto = NULL; +static char const *limit_proxy_state = NULL; #ifdef WITH_TCP static CONF_PARSER limit_config[] = { @@ -514,7 +515,7 @@ static const CONF_PARSER client_config[] = { { "src_ipaddr", FR_CONF_POINTER(PW_TYPE_STRING, &cl_srcipaddr), NULL }, { "require_message_authenticator", FR_CONF_OFFSET(PW_TYPE_BOOLEAN | PW_TYPE_IGNORE_DEFAULT, RADCLIENT, require_ma), NULL }, - { "limit_proxy_state", FR_CONF_OFFSET(PW_TYPE_BOOLEAN | PW_TYPE_IGNORE_DEFAULT, RADCLIENT, limit_proxy_state), NULL }, + { "limit_proxy_state", FR_CONF_POINTER(PW_TYPE_STRING| PW_TYPE_IGNORE_DEFAULT, &limit_proxy_state), NULL }, { "secret", FR_CONF_OFFSET(PW_TYPE_STRING | PW_TYPE_SECRET, RADCLIENT, secret), NULL }, { "shortname", FR_CONF_OFFSET(PW_TYPE_STRING, RADCLIENT, shortname), NULL }, @@ -915,6 +916,7 @@ RADCLIENT *client_afrom_cs(TALLOC_CTX *ctx, CONF_SECTION *cs, bool in_server, bo memset(&cl_ipaddr, 0, sizeof(cl_ipaddr)); cl_netmask = 255; + limit_proxy_state = NULL; if (cf_section_parse(cs, c, client_config) < 0) { cf_log_err_cs(cs, "Error parsing client section"); @@ -924,6 +926,7 @@ RADCLIENT *client_afrom_cs(TALLOC_CTX *ctx, CONF_SECTION *cs, bool in_server, bo hs_proto = NULL; cl_srcipaddr = NULL; #endif + limit_proxy_state = NULL; return NULL; } @@ -1182,6 +1185,10 @@ done_coa: } #endif + if (fr_bool_auto_parse(cf_pair_find(cs, "limit_proxy_state"), &c->limit_proxy_state, limit_proxy_state) < 0) { + goto error; + } + return c; } diff --git a/src/main/listen.c b/src/main/listen.c index 2c0fe31db0..8f8e1146fd 100644 --- a/src/main/listen.c +++ b/src/main/listen.c @@ -502,6 +502,79 @@ int rad_status_server(REQUEST *request) return 0; } +static void blastradius_checks(RADIUS_PACKET *packet, RADCLIENT *client) +{ + if (client->require_ma) return; + + /* + * If all of the checks are turned off, then complain for every packet we receive. + */ + if (client->limit_proxy_state == FR_BOOL_FALSE) { + /* + * We have a Message-Authenticator, and it's valid. We don't need to compain. + */ + if (packet->message_authenticator) return; + + DEBUG("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + DEBUG("BlastRADIUS check: Received packet without Message-Authenticator."); + DEBUG("YOU MUST SET \"require_message_authenticator = true\", or"); + DEBUG("YOU MUST SET \"limit_proxy_state = true\" for client %s", client->shortname); + DEBUG("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + DEBUG("The packet does not contain Message-Authenticator, which is a security issue"); + DEBUG("UPGRADE THE CLIENT AS YOUR NETWORK IS VULNERABLE TO THE BLASTRADIUS ATTACK."); + DEBUG("Once the client is upgraded, set \"require_message_authenticator = true\" for this client."); + DEBUG("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + return; + } + + /* + * Don't complain here. rad_packet_ok() will instead + * complain about every packet with Proxy-State but which + * is missing Message-Authenticator. + */ + if (client->limit_proxy_state == FR_BOOL_TRUE) { + return; + } + + if (packet->proxy_state && !packet->message_authenticator) { + ERROR("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + ERROR("BlastRADIUS check: Received packet with Proxy-State, but without Message-Authenticator."); + ERROR("This is either a BlastRADIUS attack, OR"); + ERROR("the client is a proxy RADIUS server which has not been upgraded."); + ERROR("Setting \"limit_proxy_state = false\" for client %s", client->shortname); + ERROR("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + ERROR("UPGRADE THE CLIENT AS YOUR NETWORK IS VULNERABLE TO THE BLASTRADIUS ATTACK."); + ERROR("Once the client is upgraded, set \"require_message_authenticator = true\" for this client."); + ERROR("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + + client->limit_proxy_state = FR_BOOL_FALSE; + + } else { + client->limit_proxy_state = FR_BOOL_TRUE; + + ERROR("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + if (!packet->proxy_state) { + ERROR("BlastRADIUS check: Received packet without Proxy-State."); + } else { + ERROR("BlastRADIUS check: Received packet with Proxy-State and Message-Authenticator."); + } + + ERROR("Setting \"limit_proxy_state = true\" for client %s", client->shortname); + ERROR("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + + if (!packet->message_authenticator) { + ERROR("The packet does not contain Message-Authenticator, which is a security issue."); + ERROR("UPGRADE THE CLIENT AS YOUR NETWORK MAY BE VULNERABLE TO THE BLASTRADIUS ATTACK."); + ERROR("Once the client is upgraded, set \"require_message_authenticator = true\" for this client."); + } else { + ERROR("The packet contains Message-Authenticator."); + if (!packet->eap_message) ERROR("The client has likely been upgraded to protect from the attack."); + ERROR("Please set \"require_message_authenticator = true\" for this client"); + } + ERROR("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + } +} + #ifdef WITH_TCP static int dual_tcp_recv(rad_listen_t *listener) { @@ -582,12 +655,17 @@ static int dual_tcp_recv(rad_listen_t *listener) /* * Enforce BlastRADIUS checks on TCP, too. */ - if (!rad_packet_ok(packet, client->require_ma | (((int) client->limit_proxy_state) << 2), NULL)) { + if (!rad_packet_ok(packet, client->require_ma | ((client->limit_proxy_state == FR_BOOL_TRUE) << 2), NULL)) { FR_STATS_INC(auth, total_malformed_requests); rad_free(&sock->packet); return 0; } + /* + * Perform BlastRADIUS checks and warnings. + */ + if (packet->code == PW_CODE_ACCESS_REQUEST) blastradius_checks(packet, client); + FR_STATS_INC(auth, total_requests); fun = rad_authenticate; break; @@ -1602,7 +1680,6 @@ static int stats_socket_recv(rad_listen_t *listener) } #endif - /* * Check if an incoming request is "ok" * @@ -1678,7 +1755,7 @@ static int auth_socket_recv(rad_listen_t *listener) * Now that we've sanity checked everything, receive the * packet. */ - packet = rad_recv(ctx, listener->fd, client->require_ma | (((int) client->limit_proxy_state) << 2)); + packet = rad_recv(ctx, listener->fd, client->require_ma | ((client->limit_proxy_state == FR_BOOL_TRUE) << 2)); if (!packet) { FR_STATS_INC(auth, total_malformed_requests); if (DEBUG_ENABLED) ERROR("Receive - %s", fr_strerror()); @@ -1686,6 +1763,11 @@ static int auth_socket_recv(rad_listen_t *listener) return 0; } + /* + * Perform BlastRADIUS checks and warnings. + */ + if (packet->code == PW_CODE_ACCESS_REQUEST) blastradius_checks(packet, client); + #ifdef __APPLE__ #ifdef WITH_UDPFROMTO /* diff --git a/src/main/mainconfig.c b/src/main/mainconfig.c index 5d318fafac..2b1c568c33 100644 --- a/src/main/mainconfig.c +++ b/src/main/mainconfig.c @@ -73,6 +73,7 @@ static char const *gid_name = NULL; static char const *chroot_dir = NULL; static bool allow_core_dumps = false; static char const *radlog_dest = NULL; +static char const *limit_proxy_state = NULL; /* * These are not used anywhere else.. @@ -87,6 +88,52 @@ static bool do_colourise = false; static char const *radius_dir = NULL; //!< Path to raddb directory +static const FR_NAME_NUMBER fr_bool_auto_names[] = { + { "false", FR_BOOL_FALSE }, + { "no", FR_BOOL_FALSE }, + { "0", FR_BOOL_FALSE }, + + { "true", FR_BOOL_TRUE }, + { "yes", FR_BOOL_TRUE }, + { "1", FR_BOOL_TRUE }, + + { "auto", FR_BOOL_AUTO }, + + { NULL, 0 } +}; + +/* + * Get decent values for false / true / auto + */ +int fr_bool_auto_parse(CONF_PAIR *cp, fr_bool_auto_t *out, char const *str) +{ + int value; + + /* + * Don't change anything. + */ + if (!str) return 0; + + value = fr_str2int(fr_bool_auto_names, str, -1); + if (value >= 0) { + *out = value; + return 0; + } + + /* + * This should never happen, as the defaults are in the + * source code. If there's no CONF_PAIR, and there's a + * parse error, then the source code is wrong. + */ + if (!cp) { + fprintf(stderr, "%s: Error - Invalid value in configuration", main_config.name); + return -1; + } + + cf_log_err(cf_pair_to_item(cp), "Invalid value for \"%s\"", cf_pair_attr(cp)); + return -1; +} + /********************************************************************** * * We need to figure out where the logs go, before doing anything @@ -161,7 +208,7 @@ static const CONF_PARSER security_config[] = { { "reject_delay", FR_CONF_POINTER(PW_TYPE_TIMEVAL, &main_config.reject_delay), STRINGIFY(0) }, { "status_server", FR_CONF_POINTER(PW_TYPE_BOOLEAN, &main_config.status_server), "no"}, { "require_message_authenticator", FR_CONF_POINTER(PW_TYPE_BOOLEAN, &main_config.require_ma), "yes"}, - { "limit_proxy_state", FR_CONF_POINTER(PW_TYPE_BOOLEAN, &main_config.limit_proxy_state), "yes"}, + { "limit_proxy_state", FR_CONF_POINTER(PW_TYPE_STRING, &limit_proxy_state), "auto"}, #ifdef ENABLE_OPENSSL_VERSION_CHECK { "allow_vulnerable_openssl", FR_CONF_POINTER(PW_TYPE_STRING, &main_config.allow_vulnerable_openssl), "no"}, #endif @@ -857,6 +904,7 @@ int main_config_init(void) if (!main_config.dictionary_dir) { main_config.dictionary_dir = DICTDIR; } + main_config.limit_proxy_state = FR_BOOL_AUTO; /* * About sizeof(REQUEST) + sizeof(RADIUS_PACKET) * 2 + sizeof(VALUE_PAIR) * 400 @@ -1152,6 +1200,18 @@ do {\ main_config.init_delay.tv_sec = 0; main_config.init_delay.tv_usec = 2* (1000000 / 3); + { + CONF_PAIR *cp = NULL; + + subcs = cf_section_sub_find(cs, "security"); + if (subcs) cp = cf_pair_find(subcs, "limit_proxy_state"); + + if (fr_bool_auto_parse(cp, &main_config.limit_proxy_state, limit_proxy_state) < 0) { + cf_file_free(cs); + return -1; + } + } + /* * Free the old configuration items, and replace them * with the new ones.