Attatched a patch that prevents options neato_tool and dot_tool from being set in database.

From 52018acd62058d40bcec0b0d1f5f3ee32c856ad2 Mon Sep 17 00:00:00 2001
From: Roland Becker <roland@atrol.de>
Date: Wed, 28 Aug 2019 11:39:42 +0200
Subject: [PATCH] Prevent arbitrary command execution of Mantis Administrators

Fixes #26091
---
 config_defaults_inc.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config_defaults_inc.php b/config_defaults_inc.php
index 6dc188dd6..516cd520e 100644
--- a/config_defaults_inc.php
+++ b/config_defaults_inc.php
@@ -4361,7 +4361,7 @@ $g_global_settings = array(
 	'ldap_simulation_file_path', 'plugin_path', 'bottom_include_page', 'top_include_page',
 	'default_home_page', 'logout_redirect_page', 'manual_url', 'logo_url', 'wiki_engine_url',
 	'cdn_enabled', 'public_config_names', 'email_login_enabled', 'email_ensure_unique',
-	'impersonate_user_threshold', 'email_retry_in_days'
+	'impersonate_user_threshold', 'email_retry_in_days', 'neato_tool', 'dot_tool'
 );
 
 /**
-- 
2.21.0.windows.1

@permanull can you confirm that the attached patch fixes the issue?

Please let us know the CVE number when you have it.

@atrol That should work, if someone had access to modify the configuration it would obviously still be possible but at that point I guess execution/access is assumed.

@dregad Here's the CVE ID CVE-2019-15715

@atrol It may be worth also implementing escapeshellcmd when looking at the two variables for a more complete fix, https://www.php.net/manual/en/function.escapeshellcmd.php prior to proc_open being executed as there's no instance where I see the user having to put arguments along with the binary path.

This should also be taken into account when using escapeshellcmd or escapeshellarg;
https://gist.github.com/Zenexer/40d02da5e07f151adeaeeaa11af9ab36

My understanding of it however seems as if it wouldn't impact the code in mantis unless proc_open performs some form of escaping which will cancel out the original attempt at doing so, they do provide an alternative escape function though I've personally never been able to bypass the functionality of the original but something to keep in mind considering another escape is used here which I had also looked into but didn't find a way to abuse for the CTF/Pentesting Challenge I was playing.

https://github.com/mantisbt/mantisbt/blob/086f31048ccc882ebbd56c329dc66a7fd329a0a3/core/url_api.php#L77

Don't know if you guys were able to see my previous notes, I'd marked them private.

We can see your private notes, but you would not be able to see our private notes.
So it makes no sense to communicate this way.

I think your notes describe another problem and should be discussed in a separate private issue without using private notes.
After that I will remove your private notes, so no one will see them after the issue is set to public.
At the moment, I am not sure if it's needed / makes sense to fix the problem that is described in your private notes.

@dregad do you agree?

Looking at the problem described by those notes, the json_url function seems unreachable and is called as a last resort so exploitation of it is very unlikely even if character encoding or etc was off... so from the outside looking in I'd say that patching in that way for that specific function is a waste of time but would still suggest adding an escapeshellcmd to the proc_open where the graph system is vulnerable even though you can trust the user input now with the configuration in the panel having it blacklisted.

@permanull apologies for the delay in taking care of this, I got sidetracked and then forgot about it.

As you have already confirmed that @atrol's patch addresses the problem, I'll commit that. Additionally, as an extra safety I'll add escapeshellcmd() call in graphviz_api.php, where the neato & dot tools are being used, even though at this point we could consider these configs' values to be trusted since they can only be set by someone already having access to the server's filesystem.

With regards to the use of escapeshellarg() in the url_get() function, this helper API is currently not used in MantisBT core; to my knowledge, only the Source Integration plugin is relying on it. As you correctly point out, shell_exec() is only a last resort. At some point I think we should make curl extension mandatory and get rid of that call. For now, I don't think it's worth taking any action here.