server UI: --tools 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.