change(config): Make the config interface more robust

epicardium/modules/config.c

@@ -127,7 +127,7 @@ static void add_config_pair(
 	slot->value_offset = value_offset;
 }
 
-static char *trim(char *str)
+static void trim(char *str)
 {
 	char *start = str;
 	while (*start && !isgraph((int)*start))
@@ -139,7 +139,8 @@ static char *trim(char *str)
 			end--;
 		end[1] = 0;
 	}
-	return start;
+
+	memmove(str, start, strlen(start) + 1);
 }
 
 // parses one line of the config file
@@ -147,7 +148,7 @@ static void parse_line(char *line, int line_number, size_t line_offset)
 {
 	char *line_start = line;
 
-	line = trim(line);
+	trim(line);
 
 	//printf(line);
 	if (*line == '#') {
@@ -168,13 +169,17 @@ static void parse_line(char *line, int line_number, size_t line_offset)
 	}
 	*eq = 0;
 
-	char *key = trim(line);
+	char *key = line;
+	trim(key);
+
 	if (*key == '\0') {
 		LOG_WARN("card10.cfg", "line %d: empty key", line_number);
 		return;
 	}
 
-	char *value = trim(eq + 1);
+	char *value = eq + 1;
+	trim(value);
+
 	if (*value == '\0') {
 		LOG_WARN(
 			"card10.cfg",
@@ -326,7 +331,7 @@ int epic_config_get_string(const char *key, char *buf, size_t buf_len)
 	}
 
 	char *end = buf;
-	while (!iscntrl(*end))
+	while (*end && !iscntrl(*end))
 		end++;
 	*end = 0;
 	trim(buf);
@@ -384,16 +389,45 @@ bool config_get_boolean_with_default(const char *key, bool default_value)
 }
 
 // TODO: don't allow things like "execute_elf" to be set
-int epic_config_set_string(const char *key, const char *value)
+int epic_config_set_string(const char *key, const char *value_in)
 {
-	config_slot *slot = find_config_slot(key);
-	bool present      = slot && slot->value_offset;
-	int ret           = 0;
+	char value[MAX_LINE_LENGTH + 1];
+
+	if (strlen(key) > MAX_LINE_LENGTH) {
+		return -EINVAL;
+	}
+
+	/* TODO: Change interface of trim to take the buffer and size directly */
+	if (strlen(value_in) > MAX_LINE_LENGTH) {
+		return -EINVAL;
+	}
+
+	strcpy(value, value_in);
+	trim(value);
 
 	if (snprintf(NULL, 0, "\n%s = %s\n", key, value) > MAX_LINE_LENGTH) {
 		return -EINVAL;
 	}
 
+	/* Check if key is sane. No control characters, spaces, equal signs or pounds allowed */
+	for (size_t i = 0; i < strlen(key); i++) {
+		char c = key[i];
+		if (!isgraph(c) || c == '=' || c == '#') {
+			return -EINVAL;
+		}
+	}
+	/* Check if value is sane. No control characters allowed */
+	for (size_t i = 0; i < strlen(value); i++) {
+		char c = value[i];
+		if (!isprint(c)) {
+			return -EINVAL;
+		}
+	}
+
+	config_slot *slot = find_config_slot(key);
+	bool present      = slot && slot->value_offset;
+	int ret           = 0;
+
 	if (!present) {
 		/* Easy case: We simply add the new option at the
 		 * end of the file. */
@@ -467,20 +501,24 @@ int epic_config_set_string(const char *key, const char *value)
 		 * Solution: Copy parts of the file, insert new value, copy
 		 * rest, rename.
 		 */
-		char buf[128];
+		char buf[MAX_LINE_LENGTH + 1];
 		int fd1 = -1;
 		int fd2 = -1;
 		ret     = epic_config_get_string(key, buf, sizeof(buf));
 
-		if (ret < 0) {
-			LOG_DEBUG(
-				"card10.cfg",
-				"could not read old value (%d)",
-				ret
+		size_t nread = read_config_offset(
+			slot->value_offset, buf, sizeof(buf)
 		);
+		if (nread == 0) {
+			LOG_DEBUG("card10.cfg", "could not read old value", );
 			goto out;
 		}
 
+		char *end = buf;
+		while (*end && (!iscntrl(*end) || isblank(*end)))
+			end++;
+		*end = 0;
+
 		int old_len = strlen(buf);
 
 		fd1 = epic_file_open("card10.cfg", "r");

pycardium/modules/sys_config.c

@@ -8,8 +8,9 @@
 
 #include <stdbool.h>
 
-static void extract_string(mp_obj_t obj, char *buf, size_t buf_len, const char *error_message)
-{
+static void extract_string(
+	mp_obj_t obj, char *buf, size_t buf_len, const char *error_message
+) {
 	size_t len;
 	const char *str_ptr = mp_obj_str_get_data(obj, &len);
 	/*
@@ -25,10 +26,20 @@ static void extract_string(mp_obj_t obj, char *buf, size_t buf_len, const char *
 
 static mp_obj_t mp_config_set_string(mp_obj_t key_in, mp_obj_t value_in)
 {
+	const char *const forbidden_key = "execute_elf";
+
 	char key_str[128], value_str[128];
 
 	extract_string(key_in, key_str, sizeof(key_str), "key too long");
-	extract_string(value_in, value_str, sizeof(value_str), "value too long");
+	extract_string(
+		value_in, value_str, sizeof(value_str), "value too long"
+	);
+
+	if (strstr(key_str, forbidden_key) != NULL ||
+	    strstr(value_str, forbidden_key) != NULL) {
+		/* A Permission Error might be nice but is not available in MP */
+		mp_raise_ValueError("Not allowed");
+	}
 
 	int status = epic_config_set_string(key_str, value_str);
 	if (status < 0) {