From 2b8434ea4640454ff27e2312545c8f593681dd21 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 15 Dec 2018 00:45:46 +0100 Subject: [PATCH 1/8] systemd: network: don't return allocated buffer of zero length from deserialize_in_addrs() Imported from systemd: deserialize_in_addrs() allocates the buffer before trying to parse the IP address. Since a parsing error is silently ignored, the returned size might be zero. In such a case we shouldn't return any buffer. Anyway, there was no leak, because there are only two callers like r = deserialize_in_addrs(&lease->dns, dns); which both keep the unused buffer and later release it. Note that deserialize_in_addrs() doesn't free the pointer before reassigning the new output. The caller must take care to to pass "ret" with an allocated buffer that would be leaked when returning the result. https://github.com/systemd/systemd/commit/c24b68216222156a45c5a8a918e7a44c144e9555 --- src/systemd/src/libsystemd-network/network-internal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/systemd/src/libsystemd-network/network-internal.c b/src/systemd/src/libsystemd-network/network-internal.c index f03a30f4b3..cee1085312 100644 --- a/src/systemd/src/libsystemd-network/network-internal.c +++ b/src/systemd/src/libsystemd-network/network-internal.c @@ -456,7 +456,7 @@ int deserialize_in_addrs(struct in_addr **ret, const char *string) { size++; } - *ret = TAKE_PTR(addresses); + *ret = size > 0 ? TAKE_PTR(addresses) : NULL; return size; } From 39ac79c55d0a20fa0e81608a7c1cda6410a8dbd0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 Dec 2018 11:10:57 +0100 Subject: [PATCH 2/8] systemd: dhcp: handle multiple addresses for "Router" (option 3) in DHCP library Imported from systemd: The Router DHCP option may contain a list of one or more routers ([1]). Extend the API of sd_dhcp_lease to return a list instead of only the first. Note that networkd still only uses the first router (if present). Aside from extending the internal API of the DHCP client, there is almost no change in behavior. The only visible difference in behavior is that the "ROUTER" variable in the lease file is now a list of addresses. Note how RFC 2132 does not define certain IP addresses as invalid for the router option. Still, previously sd_dhcp_lease_get_router() would never return a "0.0.0.0" address. In fact, the previous API could not differenciate whether no router option was present, whether it was invalid, or whether its first router was "0.0.0.0". No longer let the DHCP client library impose additional restrictions that are not part of RFC. Instead, the caller should handle this. The patch does that, and networkd only consideres the first router entry if it is not "0.0.0.0". [1] https://tools.ietf.org/html/rfc2132#section-3.5 This also required adjusting "src/dhcp/nm-dhcp-systemd.c" due to the changed internal API. https://github.com/systemd/systemd/commit/f8862395e8f802e4106a07ceaaf02b6a1faa5a6d --- src/dhcp/nm-dhcp-systemd.c | 10 ++-- .../libsystemd-network/dhcp-lease-internal.h | 4 +- .../src/libsystemd-network/sd-dhcp-lease.c | 50 ++++++++++--------- src/systemd/src/systemd/sd-dhcp-lease.h | 2 +- 4 files changed, 37 insertions(+), 29 deletions(-) diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index edb2dbbfd6..6c7447f3dc 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -267,7 +267,7 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, gint64 ts_time = time (NULL); struct in_addr a_address; struct in_addr a_netmask; - struct in_addr a_router; + const struct in_addr *a_router; guint32 a_plen; guint32 a_lifetime; @@ -482,8 +482,10 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, } /* FIXME: internal client only supports returning the first router. */ - if (sd_dhcp_lease_get_router (lease, &a_router) >= 0) { - nm_utils_inet4_ntop (a_router.s_addr, addr_str); + num = sd_dhcp_lease_get_router (lease, &a_router); + if ( num > 0 + && a_router[0].s_addr) { + nm_utils_inet4_ntop (a_router[0].s_addr, addr_str); LOG_LEASE (LOGD_DHCP4, "gateway %s", addr_str); add_option (options, dhcp4_requests, SD_DHCP_OPTION_ROUTER, addr_str); @@ -497,7 +499,7 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, nm_ip4_config_add_route (ip4_config, &((const NMPlatformIP4Route) { .rt_source = NM_IP_CONFIG_SOURCE_DHCP, - .gateway = a_router.s_addr, + .gateway = a_router[0].s_addr, .table_coerced = nm_platform_route_table_coerce (route_table), .metric = route_metric, }), diff --git a/src/systemd/src/libsystemd-network/dhcp-lease-internal.h b/src/systemd/src/libsystemd-network/dhcp-lease-internal.h index 9d245a9059..122042ab58 100644 --- a/src/systemd/src/libsystemd-network/dhcp-lease-internal.h +++ b/src/systemd/src/libsystemd-network/dhcp-lease-internal.h @@ -41,7 +41,6 @@ struct sd_dhcp_lease { /* each 0 if unset */ be32_t address; be32_t server_address; - be32_t router; be32_t next_server; bool have_subnet_mask; @@ -50,6 +49,9 @@ struct sd_dhcp_lease { bool have_broadcast; be32_t broadcast; + struct in_addr *router; + size_t router_size; + struct in_addr *dns; size_t dns_size; diff --git a/src/systemd/src/libsystemd-network/sd-dhcp-lease.c b/src/systemd/src/libsystemd-network/sd-dhcp-lease.c index 8359d077ee..c4fa62715e 100644 --- a/src/systemd/src/libsystemd-network/sd-dhcp-lease.c +++ b/src/systemd/src/libsystemd-network/sd-dhcp-lease.c @@ -152,15 +152,15 @@ int sd_dhcp_lease_get_root_path(sd_dhcp_lease *lease, const char **root_path) { return 0; } -int sd_dhcp_lease_get_router(sd_dhcp_lease *lease, struct in_addr *addr) { +int sd_dhcp_lease_get_router(sd_dhcp_lease *lease, const struct in_addr **addr) { assert_return(lease, -EINVAL); assert_return(addr, -EINVAL); - if (lease->router == 0) + if (lease->router_size <= 0) return -ENODATA; - addr->s_addr = lease->router; - return 0; + *addr = lease->router; + return (int) lease->router_size; } int sd_dhcp_lease_get_netmask(sd_dhcp_lease *lease, struct in_addr *addr) { @@ -262,6 +262,7 @@ static sd_dhcp_lease *dhcp_lease_free(sd_dhcp_lease *lease) { } free(lease->root_path); + free(lease->router); free(lease->timezone); free(lease->hostname); free(lease->domainname); @@ -388,7 +389,7 @@ static void filter_bogus_addresses(struct in_addr *addresses, size_t *n) { *n = j; } -static int lease_parse_in_addrs(const uint8_t *option, size_t len, struct in_addr **ret, size_t *n_ret) { +static int lease_parse_in_addrs(const uint8_t *option, size_t len, bool filter_bogus, struct in_addr **ret, size_t *n_ret) { assert(option); assert(ret); assert(n_ret); @@ -409,7 +410,8 @@ static int lease_parse_in_addrs(const uint8_t *option, size_t len, struct in_add if (!addresses) return -ENOMEM; - filter_bogus_addresses(addresses, &n_addresses); + if (filter_bogus) + filter_bogus_addresses(addresses, &n_addresses); free(*ret); *ret = addresses; @@ -555,21 +557,19 @@ int dhcp_lease_parse_options(uint8_t code, uint8_t len, const void *option, void break; case SD_DHCP_OPTION_ROUTER: - if (len >= 4) { - r = lease_parse_be32(option, 4, &lease->router); - if (r < 0) - log_debug_errno(r, "Failed to parse router address, ignoring: %m"); - } + r = lease_parse_in_addrs(option, len, false, &lease->router, &lease->router_size); + if (r < 0) + log_debug_errno(r, "Failed to parse router addresses, ignoring: %m"); break; case SD_DHCP_OPTION_DOMAIN_NAME_SERVER: - r = lease_parse_in_addrs(option, len, &lease->dns, &lease->dns_size); + r = lease_parse_in_addrs(option, len, true, &lease->dns, &lease->dns_size); if (r < 0) log_debug_errno(r, "Failed to parse DNS server, ignoring: %m"); break; case SD_DHCP_OPTION_NTP_SERVER: - r = lease_parse_in_addrs(option, len, &lease->ntp, &lease->ntp_size); + r = lease_parse_in_addrs(option, len, true, &lease->ntp, &lease->ntp_size); if (r < 0) log_debug_errno(r, "Failed to parse NTP server, ignoring: %m"); break; @@ -821,7 +821,6 @@ int dhcp_lease_new(sd_dhcp_lease **ret) { if (!lease) return -ENOMEM; - lease->router = INADDR_ANY; lease->n_ref = 1; *ret = lease; @@ -864,9 +863,12 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { if (r >= 0) fprintf(f, "NETMASK=%s\n", inet_ntoa(address)); - r = sd_dhcp_lease_get_router(lease, &address); - if (r >= 0) - fprintf(f, "ROUTER=%s\n", inet_ntoa(address)); + r = sd_dhcp_lease_get_router(lease, &addresses); + if (r > 0) { + fputs("ROUTER=", f); + serialize_in_addrs(f, addresses, r); + fputc('\n', f); + } r = sd_dhcp_lease_get_server_identifier(lease, &address); if (r >= 0) @@ -900,14 +902,14 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { if (r > 0) { fputs("DNS=", f); serialize_in_addrs(f, addresses, r); - fputs("\n", f); + fputc('\n', f); } r = sd_dhcp_lease_get_ntp(lease, &addresses); if (r > 0) { fputs("NTP=", f); serialize_in_addrs(f, addresses, r); - fputs("\n", f); + fputc('\n', f); } r = sd_dhcp_lease_get_domainname(lease, &string); @@ -918,7 +920,7 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { if (r > 0) { fputs("DOMAIN_SEARCH_LIST=", f); fputstrv(f, search_domains, NULL, NULL); - fputs("\n", f); + fputc('\n', f); } r = sd_dhcp_lease_get_hostname(lease, &string); @@ -1081,9 +1083,11 @@ int dhcp_lease_load(sd_dhcp_lease **ret, const char *lease_file) { } if (router) { - r = inet_pton(AF_INET, router, &lease->router); - if (r <= 0) - log_debug("Failed to parse router %s, ignoring.", router); + r = deserialize_in_addrs(&lease->router, router); + if (r < 0) + log_debug_errno(r, "Failed to deserialize router addresses %s, ignoring: %m", router); + else + lease->router_size = r; } if (netmask) { diff --git a/src/systemd/src/systemd/sd-dhcp-lease.h b/src/systemd/src/systemd/sd-dhcp-lease.h index 4875f10555..d299c79121 100644 --- a/src/systemd/src/systemd/sd-dhcp-lease.h +++ b/src/systemd/src/systemd/sd-dhcp-lease.h @@ -39,7 +39,7 @@ int sd_dhcp_lease_get_t1(sd_dhcp_lease *lease, uint32_t *t1); int sd_dhcp_lease_get_t2(sd_dhcp_lease *lease, uint32_t *t2); int sd_dhcp_lease_get_broadcast(sd_dhcp_lease *lease, struct in_addr *addr); int sd_dhcp_lease_get_netmask(sd_dhcp_lease *lease, struct in_addr *addr); -int sd_dhcp_lease_get_router(sd_dhcp_lease *lease, struct in_addr *addr); +int sd_dhcp_lease_get_router(sd_dhcp_lease *lease, const struct in_addr **addr); int sd_dhcp_lease_get_next_server(sd_dhcp_lease *lease, struct in_addr *addr); int sd_dhcp_lease_get_server_identifier(sd_dhcp_lease *lease, struct in_addr *addr); int sd_dhcp_lease_get_dns(sd_dhcp_lease *lease, const struct in_addr **addr); From 19c3d1f58b57e293cf7d56ce6320b79b7293c6ff Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 Dec 2018 16:25:01 +0100 Subject: [PATCH 3/8] systemd: dhcp: move filtering of bogus DNS/NTP addresses out of DHCP client Imported from systemd: The DHCP client should not pre-filter addresses beyond what RFC requires. If a client's user (like networkd) wishes to skip/filter certain addresses, it's their responsibility. The point of this is that the DHCP library does not hide/abstract information that might be relevant for certain users. For example, NetworkManager exposes DHCP options in its API. When doing that, the options should be close to the actual lease. This is related to commit d9ec2e632df4905201facf76d6a205edc952116a (dhcp4: filter bogus DNS/NTP server addresses silently). https://github.com/systemd/systemd/commit/072320eab04d29247d7eb1b1fc32ae10e25c020f --- shared/systemd/src/basic/in-addr-util.c | 8 +++++ shared/systemd/src/basic/in-addr-util.h | 2 ++ .../src/libsystemd-network/network-internal.c | 27 +++++++++++---- .../src/libsystemd-network/network-internal.h | 6 +++- .../src/libsystemd-network/sd-dhcp-lease.c | 34 ++++--------------- 5 files changed, 43 insertions(+), 34 deletions(-) diff --git a/shared/systemd/src/basic/in-addr-util.c b/shared/systemd/src/basic/in-addr-util.c index 62b3147a59..5ced3501ac 100644 --- a/shared/systemd/src/basic/in-addr-util.c +++ b/shared/systemd/src/basic/in-addr-util.c @@ -70,6 +70,14 @@ bool in4_addr_is_localhost(const struct in_addr *a) { return (be32toh(a->s_addr) & UINT32_C(0xFF000000)) == UINT32_C(127) << 24; } +bool in4_addr_is_non_local(const struct in_addr *a) { + /* Whether the address is not null and not localhost. + * + * As such, it is suitable to configure as DNS/NTP server from DHCP. */ + return !in4_addr_is_null(a) && + !in4_addr_is_localhost(a); +} + int in_addr_is_localhost(int family, const union in_addr_union *u) { assert(u); diff --git a/shared/systemd/src/basic/in-addr-util.h b/shared/systemd/src/basic/in-addr-util.h index 3069790519..c21567122c 100644 --- a/shared/systemd/src/basic/in-addr-util.h +++ b/shared/systemd/src/basic/in-addr-util.h @@ -30,6 +30,8 @@ int in_addr_is_link_local(int family, const union in_addr_union *u); bool in4_addr_is_localhost(const struct in_addr *a); int in_addr_is_localhost(int family, const union in_addr_union *u); +bool in4_addr_is_non_local(const struct in_addr *a); + int in_addr_equal(int family, const union in_addr_union *a, const union in_addr_union *b); int in_addr_prefix_intersect(int family, const union in_addr_union *a, unsigned aprefixlen, const union in_addr_union *b, unsigned bprefixlen); int in_addr_prefix_next(int family, union in_addr_union *u, unsigned prefixlen); diff --git a/src/systemd/src/libsystemd-network/network-internal.c b/src/systemd/src/libsystemd-network/network-internal.c index cee1085312..da8203257e 100644 --- a/src/systemd/src/libsystemd-network/network-internal.c +++ b/src/systemd/src/libsystemd-network/network-internal.c @@ -413,16 +413,31 @@ int config_parse_bridge_port_priority( } #endif /* NM_IGNORED */ -void serialize_in_addrs(FILE *f, const struct in_addr *addresses, size_t size) { - unsigned i; +size_t serialize_in_addrs(FILE *f, + const struct in_addr *addresses, + size_t size, + bool with_leading_space, + bool (*predicate)(const struct in_addr *addr)) { + size_t count; + size_t i; assert(f); assert(addresses); - assert(size); - for (i = 0; i < size; i++) - fprintf(f, "%s%s", inet_ntoa(addresses[i]), - (i < (size - 1)) ? " ": ""); + count = 0; + + for (i = 0; i < size; i++) { + if (predicate && !predicate(&addresses[i])) + continue; + if (with_leading_space) + fputc(' ', f); + else + with_leading_space = true; + fputs(inet_ntoa(addresses[i]), f); + count++; + } + + return count; } int deserialize_in_addrs(struct in_addr **ret, const char *string) { diff --git a/src/systemd/src/libsystemd-network/network-internal.h b/src/systemd/src/libsystemd-network/network-internal.h index dfe4c4244b..b54680f416 100644 --- a/src/systemd/src/libsystemd-network/network-internal.h +++ b/src/systemd/src/libsystemd-network/network-internal.h @@ -42,7 +42,11 @@ int net_get_unique_predictable_data(sd_device *device, uint64_t *result); const char *net_get_name(sd_device *device); #endif /* NM_IGNORED */ -void serialize_in_addrs(FILE *f, const struct in_addr *addresses, size_t size); +size_t serialize_in_addrs(FILE *f, + const struct in_addr *addresses, + size_t size, + bool with_leading_space, + bool (*predicate)(const struct in_addr *addr)); int deserialize_in_addrs(struct in_addr **addresses, const char *string); void serialize_in6_addrs(FILE *f, const struct in6_addr *addresses, size_t size); diff --git a/src/systemd/src/libsystemd-network/sd-dhcp-lease.c b/src/systemd/src/libsystemd-network/sd-dhcp-lease.c index c4fa62715e..18a0987df6 100644 --- a/src/systemd/src/libsystemd-network/sd-dhcp-lease.c +++ b/src/systemd/src/libsystemd-network/sd-dhcp-lease.c @@ -372,24 +372,7 @@ static int lease_parse_domain(const uint8_t *option, size_t len, char **ret) { return 0; } -static void filter_bogus_addresses(struct in_addr *addresses, size_t *n) { - size_t i, j; - - /* Silently filter DNS/NTP servers supplied to us that do not make outside of the local scope. */ - - for (i = 0, j = 0; i < *n; i ++) { - - if (in4_addr_is_null(addresses+i) || - in4_addr_is_localhost(addresses+i)) - continue; - - addresses[j++] = addresses[i]; - } - - *n = j; -} - -static int lease_parse_in_addrs(const uint8_t *option, size_t len, bool filter_bogus, struct in_addr **ret, size_t *n_ret) { +static int lease_parse_in_addrs(const uint8_t *option, size_t len, struct in_addr **ret, size_t *n_ret) { assert(option); assert(ret); assert(n_ret); @@ -410,9 +393,6 @@ static int lease_parse_in_addrs(const uint8_t *option, size_t len, bool filter_b if (!addresses) return -ENOMEM; - if (filter_bogus) - filter_bogus_addresses(addresses, &n_addresses); - free(*ret); *ret = addresses; *n_ret = n_addresses; @@ -557,19 +537,19 @@ int dhcp_lease_parse_options(uint8_t code, uint8_t len, const void *option, void break; case SD_DHCP_OPTION_ROUTER: - r = lease_parse_in_addrs(option, len, false, &lease->router, &lease->router_size); + r = lease_parse_in_addrs(option, len, &lease->router, &lease->router_size); if (r < 0) log_debug_errno(r, "Failed to parse router addresses, ignoring: %m"); break; case SD_DHCP_OPTION_DOMAIN_NAME_SERVER: - r = lease_parse_in_addrs(option, len, true, &lease->dns, &lease->dns_size); + r = lease_parse_in_addrs(option, len, &lease->dns, &lease->dns_size); if (r < 0) log_debug_errno(r, "Failed to parse DNS server, ignoring: %m"); break; case SD_DHCP_OPTION_NTP_SERVER: - r = lease_parse_in_addrs(option, len, true, &lease->ntp, &lease->ntp_size); + r = lease_parse_in_addrs(option, len, &lease->ntp, &lease->ntp_size); if (r < 0) log_debug_errno(r, "Failed to parse NTP server, ignoring: %m"); break; @@ -866,7 +846,7 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { r = sd_dhcp_lease_get_router(lease, &addresses); if (r > 0) { fputs("ROUTER=", f); - serialize_in_addrs(f, addresses, r); + serialize_in_addrs(f, addresses, r, false, NULL); fputc('\n', f); } @@ -901,14 +881,14 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { r = sd_dhcp_lease_get_dns(lease, &addresses); if (r > 0) { fputs("DNS=", f); - serialize_in_addrs(f, addresses, r); + serialize_in_addrs(f, addresses, r, false, NULL); fputc('\n', f); } r = sd_dhcp_lease_get_ntp(lease, &addresses); if (r > 0) { fputs("NTP=", f); - serialize_in_addrs(f, addresses, r); + serialize_in_addrs(f, addresses, r, false, NULL); fputc('\n', f); } From 47123e493adf4c856ab69f328ed597b8177834d7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Dec 2018 12:26:49 +0100 Subject: [PATCH 4/8] shared: add nm_ip4_addr_is_localhost() util --- shared/nm-utils/nm-shared-utils.h | 6 ++++++ shared/nm-utils/tests/test-shared-general.c | 14 ++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index 4758c4a083..4d9f20d073 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -140,6 +140,12 @@ nm_ip_addr_set (int addr_family, gpointer dst, gconstpointer src) : sizeof (struct in6_addr)); } +static inline gboolean +nm_ip4_addr_is_localhost (in_addr_t addr4) +{ + return (addr4 & htonl (0xFF000000u)) == htonl (0x7F000000u); +} + /*****************************************************************************/ #define NM_CMP_RETURN(c) \ diff --git a/shared/nm-utils/tests/test-shared-general.c b/shared/nm-utils/tests/test-shared-general.c index 7d22e56d99..b262757879 100644 --- a/shared/nm-utils/tests/test-shared-general.c +++ b/shared/nm-utils/tests/test-shared-general.c @@ -204,6 +204,19 @@ test_nm_strndup_a (void) /*****************************************************************************/ +static void +test_nm_ip4_addr_is_localhost (void) +{ + g_assert ( nm_ip4_addr_is_localhost (nmtst_inet4_from_string ("127.0.0.0"))); + g_assert ( nm_ip4_addr_is_localhost (nmtst_inet4_from_string ("127.0.0.1"))); + g_assert ( nm_ip4_addr_is_localhost (nmtst_inet4_from_string ("127.5.0.1"))); + g_assert (!nm_ip4_addr_is_localhost (nmtst_inet4_from_string ("126.5.0.1"))); + g_assert (!nm_ip4_addr_is_localhost (nmtst_inet4_from_string ("128.5.0.1"))); + g_assert (!nm_ip4_addr_is_localhost (nmtst_inet4_from_string ("129.5.0.1"))); +} + +/*****************************************************************************/ + NMTST_DEFINE (); int main (int argc, char **argv) @@ -215,6 +228,7 @@ int main (int argc, char **argv) g_test_add_func ("/general/test_nm_make_strv", test_make_strv); g_test_add_func ("/general/test_nm_strdup_int", test_nm_strdup_int); g_test_add_func ("/general/test_nm_strndup_a", test_nm_strndup_a); + g_test_add_func ("/general/test_nm_ip4_addr_is_localhost", test_nm_ip4_addr_is_localhost); return g_test_run (); } From ca540adfeb340cb2f15bdcb9d1fb92ee7f6e9168 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 16 Dec 2018 22:02:21 +0100 Subject: [PATCH 5/8] systemd: network: avoid inet_ntoa() in favor of inet_ntop() Imported from systemd: inet_ntop() is not documented to be thread-safe, so it should not be used in the DHCP library. Arguably, glibc uses a thread local buffer, so indeed there is no problem with a suitable libc. Anyway, just avoid it. https://github.com/systemd/systemd/commit/189255d2b546bc10c280a1d7bd7def702bca1769 --- src/systemd/src/libsystemd-network/network-internal.c | 9 ++++++--- src/systemd/src/libsystemd-network/sd-dhcp-lease.c | 11 ++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/systemd/src/libsystemd-network/network-internal.c b/src/systemd/src/libsystemd-network/network-internal.c index da8203257e..6e9888e804 100644 --- a/src/systemd/src/libsystemd-network/network-internal.c +++ b/src/systemd/src/libsystemd-network/network-internal.c @@ -427,13 +427,15 @@ size_t serialize_in_addrs(FILE *f, count = 0; for (i = 0; i < size; i++) { + char sbuf[INET_ADDRSTRLEN]; + if (predicate && !predicate(&addresses[i])) continue; if (with_leading_space) fputc(' ', f); else with_leading_space = true; - fputs(inet_ntoa(addresses[i]), f); + fputs(inet_ntop(AF_INET, &addresses[i], sbuf, sizeof(sbuf)), f); count++; } @@ -540,6 +542,7 @@ void serialize_dhcp_routes(FILE *f, const char *key, sd_dhcp_route **routes, siz fprintf(f, "%s=", key); for (i = 0; i < size; i++) { + char sbuf[INET_ADDRSTRLEN]; struct in_addr dest, gw; uint8_t length; @@ -547,8 +550,8 @@ void serialize_dhcp_routes(FILE *f, const char *key, sd_dhcp_route **routes, siz assert_se(sd_dhcp_route_get_gateway(routes[i], &gw) >= 0); assert_se(sd_dhcp_route_get_destination_prefix_length(routes[i], &length) >= 0); - fprintf(f, "%s/%" PRIu8, inet_ntoa(dest), length); - fprintf(f, ",%s%s", inet_ntoa(gw), (i < (size - 1)) ? " ": ""); + fprintf(f, "%s/%" PRIu8, inet_ntop(AF_INET, &dest, sbuf, sizeof(sbuf)), length); + fprintf(f, ",%s%s", inet_ntop(AF_INET, &gw, sbuf, sizeof(sbuf)), (i < (size - 1)) ? " ": ""); } fputs("\n", f); diff --git a/src/systemd/src/libsystemd-network/sd-dhcp-lease.c b/src/systemd/src/libsystemd-network/sd-dhcp-lease.c index 18a0987df6..39d2a6d082 100644 --- a/src/systemd/src/libsystemd-network/sd-dhcp-lease.c +++ b/src/systemd/src/libsystemd-network/sd-dhcp-lease.c @@ -815,6 +815,7 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { const struct in_addr *addresses; const void *client_id, *data; size_t client_id_len, data_len; + char sbuf[INET_ADDRSTRLEN]; const char *string; uint16_t mtu; _cleanup_free_ sd_dhcp_route **routes = NULL; @@ -837,11 +838,11 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { r = sd_dhcp_lease_get_address(lease, &address); if (r >= 0) - fprintf(f, "ADDRESS=%s\n", inet_ntoa(address)); + fprintf(f, "ADDRESS=%s\n", inet_ntop(AF_INET, &address, sbuf, sizeof(sbuf))); r = sd_dhcp_lease_get_netmask(lease, &address); if (r >= 0) - fprintf(f, "NETMASK=%s\n", inet_ntoa(address)); + fprintf(f, "NETMASK=%s\n", inet_ntop(AF_INET, &address, sbuf, sizeof(sbuf))); r = sd_dhcp_lease_get_router(lease, &addresses); if (r > 0) { @@ -852,15 +853,15 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { r = sd_dhcp_lease_get_server_identifier(lease, &address); if (r >= 0) - fprintf(f, "SERVER_ADDRESS=%s\n", inet_ntoa(address)); + fprintf(f, "SERVER_ADDRESS=%s\n", inet_ntop(AF_INET, &address, sbuf, sizeof(sbuf))); r = sd_dhcp_lease_get_next_server(lease, &address); if (r >= 0) - fprintf(f, "NEXT_SERVER=%s\n", inet_ntoa(address)); + fprintf(f, "NEXT_SERVER=%s\n", inet_ntop(AF_INET, &address, sbuf, sizeof(sbuf))); r = sd_dhcp_lease_get_broadcast(lease, &address); if (r >= 0) - fprintf(f, "BROADCAST=%s\n", inet_ntoa(address)); + fprintf(f, "BROADCAST=%s\n", inet_ntop(AF_INET, &address, sbuf, sizeof(sbuf))); r = sd_dhcp_lease_get_mtu(lease, &mtu); if (r >= 0) From 1d0b07bcfcf9e8b6c265e05d012d709a8efd2057 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Dec 2018 11:39:32 +0100 Subject: [PATCH 6/8] dhcp/internal: cleanup logging and failure handling in lease_to_ip4_config() ... and lease_to_ip6_config(). - Handle reasons that render the lease invalid first, before logging anything. This way, upon invalid lease we don't have partially logged about the lease. - prefer logging one line for options that contain multiple values, for example for search domains. - reorder statements to consistently log first before calling add_option(). - prefer g_string_append (nm_gstring_add_space_delimiter (str), ... over g_string_append_printf (str, "%s%s", str->len ? " " : "", ... - use @addr_str buffer directly, instead of assigning to another temporary variable. --- src/dhcp/nm-dhcp-systemd.c | 92 +++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index ddba34f392..edb2dbbfd6 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -273,22 +273,29 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, g_return_val_if_fail (lease != NULL, NULL); - ip4_config = nm_ip4_config_new (multi_idx, ifindex); - - options = out_options ? create_options_dict () : NULL; - if (sd_dhcp_lease_get_address (lease, &a_address) < 0) { nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "could not get address from lease"); return NULL; } - nm_utils_inet4_ntop (a_address.s_addr, addr_str); - LOG_LEASE (LOGD_DHCP4, "address %s", addr_str); - add_option (options, dhcp4_requests, DHCP_OPTION_IP_ADDRESS, addr_str); if (sd_dhcp_lease_get_netmask (lease, &a_netmask) < 0) { nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "could not get netmask from lease"); return NULL; } + + if (sd_dhcp_lease_get_lifetime (lease, &a_lifetime) < 0) { + nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "could not get lifetime from lease"); + return NULL; + } + + ip4_config = nm_ip4_config_new (multi_idx, ifindex); + + options = out_options ? create_options_dict () : NULL; + + nm_utils_inet4_ntop (a_address.s_addr, addr_str); + LOG_LEASE (LOGD_DHCP4, "address %s", addr_str); + add_option (options, dhcp4_requests, DHCP_OPTION_IP_ADDRESS, addr_str); + a_plen = nm_utils_ip4_netmask_to_prefix (a_netmask.s_addr); LOG_LEASE (LOGD_DHCP4, "plen %u", (guint) a_plen); add_option (options, @@ -296,10 +303,6 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, SD_DHCP_OPTION_SUBNET_MASK, nm_utils_inet4_ntop (a_netmask.s_addr, addr_str)); - if (sd_dhcp_lease_get_lifetime (lease, &a_lifetime) < 0) { - nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "could not get lifetime from lease"); - return NULL; - } LOG_LEASE (LOGD_DHCP4, "expires in %u seconds (at %lld)", (guint) a_lifetime, (long long) (ts_time + a_lifetime)); @@ -323,41 +326,42 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, if (num > 0) { nm_gstring_prepare (&str); for (i = 0; i < num; i++) { - if (addr_list[i].s_addr) { - nm_ip4_config_add_nameserver (ip4_config, addr_list[i].s_addr); - s = nm_utils_inet4_ntop (addr_list[i].s_addr, addr_str); - LOG_LEASE (LOGD_DHCP4, "nameserver '%s'", s); - g_string_append_printf (str, "%s%s", str->len ? " " : "", s); - } + if (addr_list[i].s_addr == 0) + continue; + + nm_utils_inet4_ntop (addr_list[i].s_addr, addr_str); + g_string_append (nm_gstring_add_space_delimiter (str), addr_str); + nm_ip4_config_add_nameserver (ip4_config, addr_list[i].s_addr); } - if (str->len) + if (str->len) { + LOG_LEASE (LOGD_DHCP4, "nameserver '%s'", str->str); add_option (options, dhcp4_requests, SD_DHCP_OPTION_DOMAIN_NAME_SERVER, str->str); + } } num = sd_dhcp_lease_get_search_domains (lease, (char ***) &search_domains); if (num > 0) { nm_gstring_prepare (&str); for (i = 0; i < num; i++) { + g_string_append (nm_gstring_add_space_delimiter (str), search_domains[i]); nm_ip4_config_add_search (ip4_config, search_domains[i]); - g_string_append_printf (str, "%s%s", str->len ? " " : "", search_domains[i]); - LOG_LEASE (LOGD_DHCP4, "domain search '%s'", search_domains[i]); } + LOG_LEASE (LOGD_DHCP4, "domain search '%s'", str->str); add_option (options, dhcp4_requests, SD_DHCP_OPTION_DOMAIN_SEARCH_LIST, str->str); } - if ( sd_dhcp_lease_get_domainname (lease, &s) >= 0 - && s) { + if (sd_dhcp_lease_get_domainname (lease, &s) >= 0) { gs_strfreev char **domains = NULL; char **d; + LOG_LEASE (LOGD_DHCP4, "domain name '%s'", s); + add_option (options, dhcp4_requests, SD_DHCP_OPTION_DOMAIN_NAME, s); + /* Multiple domains sometimes stuffed into option 15 "Domain Name". * As systemd escapes such characters, split them at \\032. */ domains = g_strsplit (s, "\\032", 0); - for (d = domains; *d; d++) { - LOG_LEASE (LOGD_DHCP4, "domain name '%s'", *d); + for (d = domains; *d; d++) nm_ip4_config_add_domain (ip4_config, *d); - } - add_option (options, dhcp4_requests, SD_DHCP_OPTION_DOMAIN_NAME, s); } if (sd_dhcp_lease_get_hostname (lease, &s) >= 0) { @@ -479,9 +483,9 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, /* FIXME: internal client only supports returning the first router. */ if (sd_dhcp_lease_get_router (lease, &a_router) >= 0) { - s = nm_utils_inet4_ntop (a_router.s_addr, addr_str); - LOG_LEASE (LOGD_DHCP4, "gateway %s", s); - add_option (options, dhcp4_requests, SD_DHCP_OPTION_ROUTER, s); + nm_utils_inet4_ntop (a_router.s_addr, addr_str); + LOG_LEASE (LOGD_DHCP4, "gateway %s", addr_str); + add_option (options, dhcp4_requests, SD_DHCP_OPTION_ROUTER, addr_str); /* If the DHCP server returns both a Classless Static Routes option and a * Router option, the DHCP client MUST ignore the Router option [RFC 3442]. @@ -503,19 +507,19 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, if ( sd_dhcp_lease_get_mtu (lease, &mtu) >= 0 && mtu) { - nm_ip4_config_set_mtu (ip4_config, mtu, NM_IP_CONFIG_SOURCE_DHCP); - add_option_u64 (options, dhcp4_requests, SD_DHCP_OPTION_INTERFACE_MTU, mtu); LOG_LEASE (LOGD_DHCP4, "mtu %u", mtu); + add_option_u64 (options, dhcp4_requests, SD_DHCP_OPTION_INTERFACE_MTU, mtu); + nm_ip4_config_set_mtu (ip4_config, mtu, NM_IP_CONFIG_SOURCE_DHCP); } num = sd_dhcp_lease_get_ntp (lease, &addr_list); if (num > 0) { nm_gstring_prepare (&str); for (i = 0; i < num; i++) { - s = nm_utils_inet4_ntop (addr_list[i].s_addr, addr_str); - LOG_LEASE (LOGD_DHCP4, "ntp server '%s'", s); - g_string_append_printf (str, "%s%s", str->len ? " " : "", s); + nm_utils_inet4_ntop (addr_list[i].s_addr, addr_str); + g_string_append (nm_gstring_add_space_delimiter (str), addr_str); } + LOG_LEASE (LOGD_DHCP4, "ntp server '%s'", str->str); add_option (options, dhcp4_requests, SD_DHCP_OPTION_NTP_SERVER, str->str); } @@ -824,6 +828,7 @@ lease_to_ip6_config (NMDedupMultiIndex *multi_idx, sd_dhcp6_lease_reset_address_iter (lease); nm_gstring_prepare (&str); while (sd_dhcp6_lease_get_address (lease, &tmp_addr, &lft_pref, &lft_valid) >= 0) { + char sbuf[400]; const NMPlatformIP6Address address = { .plen = 128, .address = tmp_addr, @@ -836,15 +841,12 @@ lease_to_ip6_config (NMDedupMultiIndex *multi_idx, nm_ip6_config_add_address (ip6_config, &address); nm_utils_inet6_ntop (&tmp_addr, addr_str); - if (str->len) - g_string_append_c (str, ' '); - g_string_append (str, addr_str); + g_string_append (nm_gstring_add_space_delimiter (str), addr_str); LOG_LEASE (LOGD_DHCP6, "address %s", - nm_platform_ip6_address_to_string (&address, NULL, 0)); + nm_platform_ip6_address_to_string (&address, sbuf, sizeof (sbuf))); }; - if (str->len) add_option (options, dhcp6_requests, DHCP6_OPTION_IP_ADDRESS, str->str); @@ -861,13 +863,11 @@ lease_to_ip6_config (NMDedupMultiIndex *multi_idx, if (num > 0) { nm_gstring_prepare (&str); for (i = 0; i < num; i++) { - nm_ip6_config_add_nameserver (ip6_config, &dns[i]); nm_utils_inet6_ntop (&dns[i], addr_str); - if (str->len) - g_string_append_c (str, ' '); - g_string_append (str, addr_str); - LOG_LEASE (LOGD_DHCP6, "nameserver %s", addr_str); + g_string_append (nm_gstring_add_space_delimiter (str), addr_str); + nm_ip6_config_add_nameserver (ip6_config, &dns[i]); } + LOG_LEASE (LOGD_DHCP6, "nameserver %s", str->str); add_option (options, dhcp6_requests, SD_DHCP6_OPTION_DNS_SERVERS, str->str); } @@ -875,10 +875,10 @@ lease_to_ip6_config (NMDedupMultiIndex *multi_idx, if (num > 0) { nm_gstring_prepare (&str); for (i = 0; i < num; i++) { + g_string_append (nm_gstring_add_space_delimiter (str), domains[i]); nm_ip6_config_add_search (ip6_config, domains[i]); - g_string_append_printf (str, "%s%s", str->len ? " " : "", domains[i]); - LOG_LEASE (LOGD_DHCP6, "domain name '%s'", domains[i]); } + LOG_LEASE (LOGD_DHCP6, "domain name '%s'", str->str); add_option (options, dhcp6_requests, SD_DHCP6_OPTION_DOMAIN_LIST, str->str); } From f3e1dea1fef80d8a65142ccae7b008f4936bf063 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Dec 2018 11:31:47 +0100 Subject: [PATCH 7/8] dhcp/internal: handle multiple Router options in internal DHCP clint https://bugzilla.redhat.com/show_bug.cgi?id=1634657 --- src/dhcp/nm-dhcp-systemd.c | 49 ++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index 6c7447f3dc..49c478a504 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -481,30 +481,49 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, add_option (options, dhcp4_requests, SD_DHCP_OPTION_STATIC_ROUTE, str_static->str); } - /* FIXME: internal client only supports returning the first router. */ num = sd_dhcp_lease_get_router (lease, &a_router); - if ( num > 0 - && a_router[0].s_addr) { - nm_utils_inet4_ntop (a_router[0].s_addr, addr_str); - LOG_LEASE (LOGD_DHCP4, "gateway %s", addr_str); - add_option (options, dhcp4_requests, SD_DHCP_OPTION_ROUTER, addr_str); + if (num > 0) { + guint32 default_route_metric = route_metric; + + nm_gstring_prepare (&str); + for (i = 0; i < num; i++) { + guint32 m; + + s = nm_utils_inet4_ntop (a_router[i].s_addr, addr_str); + g_string_append (nm_gstring_add_space_delimiter (str), s); + + if (a_router[i].s_addr == 0) { + /* silently skip 0.0.0.0 */ + continue; + } + + if (has_router_from_classless) { + /* If the DHCP server returns both a Classless Static Routes option and a + * Router option, the DHCP client MUST ignore the Router option [RFC 3442]. + * + * Be more lenient and ignore the Router option only if Classless Static + * Routes contain a default gateway (as other DHCP backends do). + */ + continue; + } + + /* if there are multiple default routes, we add them with differing + * metrics. */ + m = default_route_metric; + if (default_route_metric < G_MAXUINT32) + default_route_metric++; - /* If the DHCP server returns both a Classless Static Routes option and a - * Router option, the DHCP client MUST ignore the Router option [RFC 3442]. - * - * Be more lenient and ignore the Router option only if Classless Static - * Routes contain a default gateway (as other DHCP backends do). - */ - if (!has_router_from_classless) { nm_ip4_config_add_route (ip4_config, &((const NMPlatformIP4Route) { .rt_source = NM_IP_CONFIG_SOURCE_DHCP, - .gateway = a_router[0].s_addr, + .gateway = a_router[i].s_addr, .table_coerced = nm_platform_route_table_coerce (route_table), - .metric = route_metric, + .metric = m, }), NULL); } + LOG_LEASE (LOGD_DHCP4, "router %s", str->str); + add_option (options, dhcp4_requests, SD_DHCP_OPTION_ROUTER, str->str); } if ( sd_dhcp_lease_get_mtu (lease, &mtu) >= 0 From c2b3b9b95582696c044d3d580e40570b6e96a0df Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Dec 2018 12:19:29 +0100 Subject: [PATCH 8/8] dhcp/internal: handle localhost and 0.0.0.0 DNS/NTP servers specially - regarding the DHCP options, we should not suppress them. If the lease contains such bogus(?) addresses, we still want to expose them on D-Bus without modification. - regrading using the DNS server, ignore localhost addresses like done for systemd-networkd ([1], [2]). Until recently, the DHCP library would internally suppress such addresses ([3]). That is no longer the case, and we should handle them specially. [1] https://github.com/systemd/systemd/issues/4524 [2] https://github.com/systemd/systemd/commit/d9ec2e632df4905201facf76d6a205edc952116a [3] 334d5682ae3f1eb89a98c69db3d23140e1552cbd --- src/dhcp/nm-dhcp-systemd.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index 49c478a504..70ed871503 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -326,17 +326,19 @@ lease_to_ip4_config (NMDedupMultiIndex *multi_idx, if (num > 0) { nm_gstring_prepare (&str); for (i = 0; i < num; i++) { - if (addr_list[i].s_addr == 0) - continue; - nm_utils_inet4_ntop (addr_list[i].s_addr, addr_str); g_string_append (nm_gstring_add_space_delimiter (str), addr_str); + + if ( addr_list[i].s_addr == 0 + || nm_ip4_addr_is_localhost (addr_list[i].s_addr)) { + /* Skip localhost addresses, like also networkd does. + * See https://github.com/systemd/systemd/issues/4524. */ + continue; + } nm_ip4_config_add_nameserver (ip4_config, addr_list[i].s_addr); } - if (str->len) { - LOG_LEASE (LOGD_DHCP4, "nameserver '%s'", str->str); - add_option (options, dhcp4_requests, SD_DHCP_OPTION_DOMAIN_NAME_SERVER, str->str); - } + LOG_LEASE (LOGD_DHCP4, "nameserver '%s'", str->str); + add_option (options, dhcp4_requests, SD_DHCP_OPTION_DOMAIN_NAME_SERVER, str->str); } num = sd_dhcp_lease_get_search_domains (lease, (char ***) &search_domains);