From 845e57262d1f0472afdd1da336a52f7c4e57e095 Mon Sep 17 00:00:00 2001 From: Daniel Lenski Date: Tue, 14 Dec 2021 16:54:34 -0800 Subject: [PATCH] openconnect_set_reported_os should reject illegal values MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The OS values ('vpninfo->platname') that OpenConnect accepts are historically derived from the Cisco AnyConnect protocol: linux, linux-64, win, mac-intel, android, apple-ios. In the Cisco AnyConnect protocol, the platname was sometimes sent verbatim on the wire (see auth.c), and could perhaps be thought of as a potentially free-form string. However: (a) The platname value is now used in other protocols, requiring lookups to convert to the correct forms for those protocols (e.g., see auth-globalprotect.c). (b) Even in the Cisco AnyConnect protocol, the platname value has has to be handled with switch/if statements which can only correctly handle the limited set of known values. Since only a limited set of values are actually *understood* by OpenConnect, allowing arbitrary values to be provided—but silently ignored or mishandled—leads to confusing errors, such as in https://lists.infradead.org/pipermail/openconnect-devel/2021-December/005079.html. In that case, a user specified '--os=windows' (incorrect) instead of '--os=win' (correct). This likely led to incorrect server behavior, but OpenConnect silently accepted the incorrect value. This patch modifies openconnect_set_reported_os to return -EINVAL if any OS name other than one of the 6 legal values is specified. Future improvements: 1. Replace the numerous repetitions of the literal OS values with something saner, like an enum type. 2. Consider retiring the Cisco-specific values altogether as part of the "Great Renaming" (https://gitlab.com/openconnect/openconnect/-/merge_requests/151). Signed-off-by: Daniel Lenski --- library.c | 11 +++++++++-- main.c | 6 ++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/library.c b/library.c index 37b7f9bc..d4f46838 100644 --- a/library.c +++ b/library.c @@ -378,6 +378,8 @@ int openconnect_make_cstp_connection(struct openconnect_info *vpninfo) int openconnect_set_reported_os(struct openconnect_info *vpninfo, const char *os) { + const char *allowed[] = {"linux", "linux-64", "win", "mac-intel", "android", "apple-ios"}; + if (!os) { #if defined(__APPLE__) os = "mac-intel"; @@ -388,8 +390,13 @@ int openconnect_set_reported_os(struct openconnect_info *vpninfo, #endif } - STRDUP(vpninfo->platname, os); - return 0; + for (int i = 0; i < ARRAY_SIZE(allowed); i++) { + if (!strcmp(os, allowed[i])) { + STRDUP(vpninfo->platname, os); + return 0; + } + } + return -EINVAL; } int openconnect_set_mobile_info(struct openconnect_info *vpninfo, diff --git a/main.c b/main.c index 9034e5c7..67225b50 100644 --- a/main.c +++ b/main.c @@ -948,7 +948,8 @@ static void usage(void) printf("\n%s:\n", _("Local system information")); printf(" --useragent=STRING %s\n", _("HTTP header User-Agent: field")); printf(" --local-hostname=STRING %s\n", _("Local hostname to advertise to server")); - printf(" --os=STRING %s\n", _("OS type (linux,linux-64,win,...) to report")); + printf(" --os=STRING %s\n", _("OS type to report. Allowed values are the following:")); + printf(" %s\n", _("linux, linux-64, win, mac-intel, android, apple-ios")); printf(" --version-string=STRING %s\n", _("reported version string during authentication")); printf(" (%s %s)\n", _("default:"), openconnect_version_str); @@ -2029,7 +2030,8 @@ int main(int argc, char **argv) case OPT_OS: assert_nonnull_config_arg("os", config_arg); if (openconnect_set_reported_os(vpninfo, config_arg)) { - fprintf(stderr, _("Invalid OS identity \"%s\"\n"), + fprintf(stderr, _("Invalid OS identity \"%s\"\n" + "Allowed values: linux, linux-64, win, mac-intel, android, apple-ios\n"), config_arg); exit(1); } -- 2.50.1