Hi guys,
You probably saw the mail from Jan Lieskovsky about the "security issue"
of not escaping filenames and other placeholders in the build commands.
Although I don't really think it's important from the security POV, I
think it would be great to fix it for users not to get weird errors if
their files actually contain some weird characters (even only spaces if
their build command misses quoting around the placeholder).
So, how to fix it?
What comes to mind immediately is to escape the placeholder
replacements. It would work, but we need to take a little more care
than that, because the build command may or may not already have the
placeholder quoted (like gcc -c "%f" -o %e.o).
The other solution I thought about was not to build another string but
directly an `argv` vector, but it's not really doable I think because we
want to be able to replace placeholders not only in argv but also in
directory paths & friends. And actually it doesn't really fix anything
since we don't want a placeholder to correspond to a whole argument
anyway (like int %e.o).
So, I wrote a not-that-trivial replacement of
`build_replace_placeholder()` (patch attached) that takes care of the
replacement quoting (using `g_shell_quote()`) and quotes in the input.
Apart some more testing, I had some doubts about Windows compatibility
here. Will the windows spawn code deal correctly with the escapes? If
not, how to escape for Windows too?
Voila, so could you test, and what do you think?
Cheers,
Colomban
PS: my patch also fixes replacing of a placeholder in an previous
replacement, e.g. if the replacement for %f contains the literal %e it
won't be replaced.
diff --git a/src/build.c b/src/build.c
index 7a2d84a..3c9d18f 100644
--- a/src/build.c
+++ b/src/build.c
@@ -711,55 +711,95 @@ static void parse_build_output(const gchar **output, gint status)
static gchar *build_replace_placeholder(const GeanyDocument *doc, const gchar *src)
{
GString *stack;
- gchar *filename = NULL;
- gchar *replacement;
- gchar *executable = NULL;
- gchar *ret_str; /* to be freed when not in use anymore */
+ gchar quote = 0;
+ /* replacements, %<letter> */
+ gchar *f = NULL; /* %f: basename */
+ gchar *d = NULL; /* %d: dirname */
+ gchar *e = NULL; /* %e: basename without extension */
+ gchar *p = NULL; /* %p: project (absolute) base directory */
+
+ if (src == NULL)
+ return NULL;
- stack = g_string_new(src);
if (doc != NULL && doc->file_name != NULL)
{
- filename = utils_get_utf8_from_locale(doc->file_name);
-
- /* replace %f with the filename (including extension) */
- replacement = g_path_get_basename(filename);
- utils_string_replace_all(stack, "%f", replacement);
- g_free(replacement);
+ gchar *filename = utils_get_utf8_from_locale(doc->file_name);
- /* replace %d with the absolute path of the dir of the current file */
- replacement = g_path_get_dirname(filename);
- utils_string_replace_all(stack, "%d", replacement);
- g_free(replacement);
+ f = g_path_get_basename(filename);
+ d = g_path_get_dirname(filename);
+ e = utils_remove_ext_from_filename(f);
- /* replace %e with the filename (excluding extension) */
- executable = utils_remove_ext_from_filename(filename);
- replacement = g_path_get_basename(executable);
- utils_string_replace_all(stack, "%e", replacement);
- g_free(replacement);
+ g_free(filename);
}
-
- /* replace %p with the current project's (absolute) base directory */
- replacement = NULL; /* prevent double free if no replacement found */
if (app->project)
+ p = project_get_base_path();
+
+ /* quote the replacements */
+ if (f) SETPTR(f, g_shell_quote(f));
+ if (d) SETPTR(d, g_shell_quote(d));
+ if (e) SETPTR(e, g_shell_quote(e));
+ if (p) SETPTR(p, g_shell_quote(p));
+
+ stack = g_string_new(NULL);
+ for (; *src; src++)
{
- replacement = project_get_base_path();
- }
- else if (strstr(stack->str, "%p"))
- { /* fall back to %d */
- ui_set_statusbar(FALSE, _("failed to substitute %%p, no project active"));
- if (doc != NULL && filename != NULL)
- replacement = g_path_get_dirname(filename);
- }
+ switch (*src)
+ {
+ case '"':
+ case '\'':
+ if (! quote)
+ quote = *src;
+ else if (quote == *src)
+ quote = 0;
+ g_string_append_c(stack, *src);
+ break;
- utils_string_replace_all(stack, "%p", replacement);
- g_free(replacement);
+ case '%':
+ src++;
+ if (quote) /* close the quote */
+ g_string_append_c(stack, quote);
+ if (*src == 'f' && f)
+ g_string_append(stack, f);
+ else if (*src == 'd' && d)
+ g_string_append(stack, d);
+ else if (*src == 'e' && e)
+ g_string_append(stack, e);
+ else if (*src == 'p')
+ {
+ if (p)
+ g_string_append(stack, p);
+ else
+ { /* fallback to %d */
+ ui_set_statusbar(FALSE, _("failed to substitute %%p, no project active"));
+ if (d)
+ g_string_append(stack, d);
+ }
+ }
+ else
+ { /* just leave the placeholder */
+ g_string_append_c(stack, '%');
+ g_string_append_c(stack, *src);
+ }
+ if (quote) /* re-open quote */
+ g_string_append_c(stack, quote);
+ break;
- ret_str = utils_get_utf8_from_locale(stack->str);
- g_free(executable);
- g_free(filename);
- g_string_free(stack, TRUE);
+ case '\\':
+ g_string_append_c(stack, *src);
+ src++;
+ /* fallthrough */
+ default:
+ g_string_append_c(stack, *src);
+ break;
+ }
+ }
+
+ g_free (f);
+ g_free (d);
+ g_free (e);
+ g_free (p);
- return ret_str; /* don't forget to free src also if needed */
+ return g_string_free(stack, FALSE);
}
_______________________________________________
Devel mailing list
Devel@lists.geany.org
https://lists.geany.org/cgi-bin/mailman/listinfo/devel
