libnm-util: enforce APN character restrictions

APNs can only contain alphanumeric characters, '.', and '-'.  To be
helpful we strip spaces off before setting the APN internally so that
previously (and incorrectly) valid APNs don't cause the whole
connection to fail validation and thus disappear.  The only case seen
in the wild was a Pelephone IL APN which erroneously had a trailing
space in the mobile broadband provider database.  Bad characters
cause the connection to fail with vague error messages about being
unable to activate the PDP context during PPP negotiation.

2 files changed, 99 insertions, 9 deletions

diff --git a/libnm-util/nm-setting-gsm.c b/libnm-util/nm-setting-gsm.c
index 4b4560cd6d..575f94d382 100644
--- a/libnm-util/nm-setting-gsm.c
+++ b/libnm-util/nm-setting-gsm.c
@@ -231,12 +231,30 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
 		return FALSE;
 	}
 
-	if (priv->apn && (strlen (priv->apn) < 1 || strchr (priv->apn, '"'))) {
-		g_set_error (error,
-		             NM_SETTING_GSM_ERROR,
-		             NM_SETTING_GSM_ERROR_INVALID_PROPERTY,
-		             NM_SETTING_GSM_APN);
-		return FALSE;
+	if (priv->apn) {
+		guint32 apn_len = strlen (priv->apn);
+		guint32 i;
+
+		if (apn_len < 1 || apn_len > 20) {
+			g_set_error (error,
+			             NM_SETTING_GSM_ERROR,
+			             NM_SETTING_GSM_ERROR_INVALID_PROPERTY,
+			             NM_SETTING_GSM_APN);
+			return FALSE;
+		}
+
+		/* APNs roughly follow the same rules as DNS domain names.  Allowed
+		 * characters are a-z, 0-9, . and -.  GSM 03.60 Section 14.9.
+		 */
+		for (i = 0; i < apn_len; i++) {
+			if (!isalnum (priv->apn[i]) && (priv->apn[i] != '.') && (priv->apn[i] != '-')) {
+				g_set_error (error,
+				             NM_SETTING_GSM_ERROR,
+				             NM_SETTING_GSM_ERROR_INVALID_PROPERTY,
+				             NM_SETTING_GSM_APN);
+				return FALSE;
+			}
+		}
 	}
 
 	if (priv->username && !strlen (priv->username)) {
@@ -342,11 +360,11 @@ set_property (GObject *object, guint prop_id,
 		break;
 	case PROP_APN:
 		g_free (priv->apn);
-		priv->apn = g_value_dup_string (value);
+		priv->apn = g_strstrip (g_value_dup_string (value));
 		break;
 	case PROP_NETWORK_ID:
 		g_free (priv->network_id);
-		priv->network_id = g_value_dup_string (value);
+		priv->network_id = g_strstrip (g_value_dup_string (value));
 		break;
 	case PROP_NETWORK_TYPE:
 		priv->network_type = g_value_get_int (value);
@@ -503,6 +521,8 @@ nm_setting_gsm_class_init (NMSettingGsmClass *setting_class)
 	 * the user will be billed for their network usage and whether the user has
 	 * access to the Internet or just a provider-specific walled-garden, so it
 	 * is important to use the correct APN for the user's mobile broadband plan.
+	 * The APN may only be composed of the characters a-z, 0-9, ., and - per
+	 * GSM 03.60 Section 14.9.
 	 **/
 	g_object_class_install_property
 		(object_class, PROP_APN,
@@ -515,7 +535,8 @@ nm_setting_gsm_class_init (NMSettingGsmClass *setting_class)
 						  "user has access to the Internet or just a provider-"
 						  "specific walled-garden, so it is important to use "
 						  "the correct APN for the user's mobile broadband "
-						  "plan.",
+						  "plan.  The APN may only be composed of the characters "
+						  "a-z, 0-9, ., and - per GSM 03.60 Section 14.9.",
 						  NULL,
 						  G_PARAM_READWRITE | NM_SETTING_PARAM_SERIALIZE));
 
diff --git a/libnm-util/tests/test-general.c b/libnm-util/tests/test-general.c
index 9530445959..855ee083fb 100644
--- a/libnm-util/tests/test-general.c
+++ b/libnm-util/tests/test-general.c
@@ -28,6 +28,7 @@
 
 #include "nm-setting-connection.h"
 #include "nm-setting-vpn.h"
+#include "nm-setting-gsm.h"
 #include "nm-setting-ip6-config.h"
 #include "nm-dbus-glib-types.h"
 
@@ -222,6 +223,72 @@ test_setting_ip6_config_old_address_array (void)
 	g_object_unref (s_ip6);
 }
 
+static void
+test_setting_gsm_apn_spaces (void)
+{
+	NMSettingGsm *s_gsm;
+	const char *tmp;
+
+	s_gsm = (NMSettingGsm *) nm_setting_gsm_new ();
+	ASSERT (s_gsm != NULL,
+	        "gsm-apn-spaces",
+	        "error creating GSM setting");
+
+	/* Trailing space */
+	g_object_set (s_gsm, NM_SETTING_GSM_APN, "foobar ", NULL);
+	tmp = nm_setting_gsm_get_apn (s_gsm);
+	ASSERT (tmp != NULL,
+	        "gsm-apn-spaces", "empty APN");
+	ASSERT (strcmp (tmp, "foobar") == 0,
+	        "gsm-apn-spaces", "unexpected APN");
+
+	/* Leading space */
+	g_object_set (s_gsm, NM_SETTING_GSM_APN, " foobar", NULL);
+	tmp = nm_setting_gsm_get_apn (s_gsm);
+	ASSERT (tmp != NULL,
+	        "gsm-apn-spaces", "empty APN");
+	ASSERT (strcmp (tmp, "foobar") == 0,
+	        "gsm-apn-spaces", "unexpected APN");
+}
+
+static void
+test_setting_gsm_apn_bad_chars (void)
+{
+	NMSettingGsm *s_gsm;
+
+	s_gsm = (NMSettingGsm *) nm_setting_gsm_new ();
+	ASSERT (s_gsm != NULL,
+	        "gsm-apn-bad-chars",
+	        "error creating GSM setting");
+
+	g_object_set (s_gsm, NM_SETTING_GSM_NUMBER, "*99#", NULL);
+
+	/* Make sure a valid APN works */
+	g_object_set (s_gsm, NM_SETTING_GSM_APN, "foobar123.-baz", NULL);
+	ASSERT (nm_setting_verify (NM_SETTING (s_gsm), NULL, NULL) == TRUE,
+	        "gsm-apn-bad-chars", "unexpectedly invalid GSM setting");
+
+	/* Random invalid chars */
+	g_object_set (s_gsm, NM_SETTING_GSM_APN, "@#%$@#%@#%", NULL);
+	ASSERT (nm_setting_verify (NM_SETTING (s_gsm), NULL, NULL) == FALSE,
+	        "gsm-apn-bad-chars", "unexpectedly valid GSM setting");
+
+	/* Spaces */
+	g_object_set (s_gsm, NM_SETTING_GSM_APN, "foobar baz", NULL);
+	ASSERT (nm_setting_verify (NM_SETTING (s_gsm), NULL, NULL) == FALSE,
+	        "gsm-apn-bad-chars", "unexpectedly valid GSM setting");
+
+	/* 0 characters long */
+	g_object_set (s_gsm, NM_SETTING_GSM_APN, "", NULL);
+	ASSERT (nm_setting_verify (NM_SETTING (s_gsm), NULL, NULL) == FALSE,
+	        "gsm-apn-bad-chars", "unexpectedly valid GSM setting");
+
+	/* 21-character long */
+	g_object_set (s_gsm, NM_SETTING_GSM_APN, "abcdefghijklmnopqrstu", NULL);
+	ASSERT (nm_setting_verify (NM_SETTING (s_gsm), NULL, NULL) == FALSE,
+	        "gsm-apn-bad-chars", "unexpectedly valid GSM setting");
+}
+
 int main (int argc, char **argv)
 {
 	GError *error = NULL;
@@ -237,6 +304,8 @@ int main (int argc, char **argv)
 	/* The tests */
 	test_setting_vpn_items ();
 	test_setting_ip6_config_old_address_array ();
+	test_setting_gsm_apn_spaces ();
+	test_setting_gsm_apn_bad_chars ();
 
 	base = g_path_get_basename (argv[0]);
 	fprintf (stdout, "%s: SUCCESS\n", base);