server UI: --tools Agent/SKILLS from Markdown and security aspects

[farhi]

Hello,

Thanks to @ngxson for suggesting to initiate this discussion concerning the --tools (which he has mostly contributed).
This discussion is launched after PR attempt #24659, regarding a suggestion to add tools from a Markdown file.

The security aspect is vital for llama.cpp agent features. Hard-coded tools are secured by construction. This is true for all current tools except server_tool_exec_shell_command which calls a shell. This is really dangerous when enabled. The reason is that the current server_tool_exec_shell_command tool implementation is calling the run_processfunction with either args = {"cmd", "/c", command}; or args = {"sh", "-c", command};. This is sent to subprocess_create which uses a string vector as argument, but the command is sent as a single string. So anything in the 'command' will be executed in a new shell context. This allows command injection of all kinds. This tool can be kept, but may be complemented with a new mechanism which ensures that no command injection is possible. This discussion brings a potential solution.

I suggest more tools could be added by reading a Markdown file such as (or something else, but kept simple):

# Tools/Agents Definition (for Linux/MacOS; adapt to Windows equivalents if needed)

**datetime**:   Get the current date and time       (command: `date`)
**grep_text**:  Search for text {pattern} in {file} (command: `grep  "{pattern}" {file}`)
**get_url**:    Get the content of given web {URL}  (command: `lynx -dump {URL}`)
**git_clone**:  Get a git repository from {url}     (command: `git clone {url}`)
**list_files**: List all files in given {directory} (command: `ls {directory}`)
**read_file**:  Get the content of a {file}         (command: `cat {file}`)

The **bold** is just there for readability by humans. MD syntax is subject to discussion. Of course, the list of tools from MD must be properly defined to avoid dangerous commands. This is on the user's responsibility when launching the server.

The steps for implementation could be:

• Read a Markdown file that defines new command templates, such as lynx -dump {URL}. Add new tools as proposed in PR tools/server: support for --tools defined from e.g. Markdown files at start #24659. In my view, SKILLS specifications are quite complex to implement, when compared with a single MD file as above.
• The number of command parts is kept fixed and converted to a string vector. In the given example we have 3 arguments ('lynx','-dump','{URL}').
• Placeholders are replaced from the context in the string vector. Any potential command injection in placeholder such as {URL} would still be part of the same argument e.g. "-cmd_script=FILENAME http://xxx".
• No shell is called (as opposed to server_tool_exec_shell_command), but instead we directly call subprocess_create with the string vector after placeholder replacement in each arg of the string vector. In the example, the lynx call would fail as the URL would be given literally as "-cmd_script=FILENAME http://xxx".
• There is no need to check for syntax, as the Markdown is supposed to be checked before the server start.
• An equivalent for the server_tool_exec_shell_command would be a MD tool command given as bash -c "{command}" where the command is sent as a single string to the shell. This is dangerous.

The issue with PR #24659 is related to the shell call which presents a security breach, whatever checks are done in the command syntax. I suggest to still contribute something like #24659 while removing the shell call e.g. the run_process("sh" "-c" "command") and replace it with run_process(string_vector_command) (no shell). The security check ('is_command_safe') would be removed.

In this way:

• we can extend tools easily (at least easier than SKILLS.md).
• the commands are safe as their structure described in the Markdown is kept.

Cheers.

[willend]

Would indeed be nice @farhi

[jboero]

Funny enough I thought about adding a "container" tool such that Llama.cpp could spin up a podman container or docker container for each conversation and only execute commands in that container (much like Anthropic etc already do) but then it turns out shell_command can already spin up containers. Magic.

Restricting tools to the current conversation container is another story but definitely recommend that for production.

[farhi]

I've been looking at the SKILLS.md specifications. It boils down to MD files which each describe a procedure in natural language. Such procedures rely on atomistic allowed-tools which are described in https://agentskills.io/specification#allowed-tools-field
We find there: Grep Glob Read Write Bash ...
The Bash from SKILLS can be restricted to a subset of commands such as Bash(git:*), but this in no way avoids arbitrary commands.

We find similar matching --tools in llama.cpp/tools/server/server-tools.cpp, e.g.:

build_tools() {
    std::vector<std::unique_ptr<server_tool>> tools;
    tools.push_back(std::make_unique<server_tool_read_file>());
    tools.push_back(std::make_unique<server_tool_file_glob_search>());
    tools.push_back(std::make_unique<server_tool_grep_search>());
    tools.push_back(std::make_unique<server_tool_exec_shell_command>());
    tools.push_back(std::make_unique<server_tool_write_file>());
    tools.push_back(std::make_unique<server_tool_edit_file>());
    tools.push_back(std::make_unique<server_tool_get_datetime>());

All tools are secured, except the exec_shell_command which corresponds with the Bash tools from Anthropic SKILLS.md.

The shell exec is done through:

args = {"cmd", "/c", command}; # Windows
args = {"sh", "-c", command}; # Linux

which is sent to io->run -> subprocess
We can see that the 'command' can be anything, and it is fully interpreted by the shell (cmdor sh). Allowing the tools exposes the users computer and files. There is a warning but I guess most users will simply 'take the risk' and run with --tools all.

I thus have three suggestions:

1. Rename to llama.cpp tools to conform with the SKILLS:allowed-tools.
2. Remove the 'exec_shell_command' from the 'all' set.
3. Use a positive list of allowed commands, rather than restricting the 'exec_shell_command' on keywords. I suggest to use the Markdown solution above. Nothing prevents though to have a dangerous 'bash -c {command}' rule which is equivalent to the exec_shell_command.

Now that we see cases of AI escaping their containers by using clever combinations of commands, we should clearly NOT allow any command to be executed. On a production server the list of allowed commands must be known and carefully chosen. And it is better if this does not require programming in the server-tools for each.