fossil git export does not properly shell-escape inputs:

zTagCmd = mprintf("git tag -f \"%s\" %s", zTagname, zObj);

Furthermore, gitmirror_sanitize_name() is incomplete and doesn't not strip dangerous characters.

This means that the following results in (remote) code execution in fossil git export (remote in the case where someone provides an automatic mirroring service):

fossil tag add '";touch${IFS}code_exec;"' tip

After fossil git export ., there's now a file called code_exec. An attacker could have put an entire reverse shell here.

As last time, I would have prefered to report this privately, but I still contact info for that.

A CVE should be obtained for this bug, so that distributions will be made aware of the need to apply patches for security or update if there will be a new release for this.

As last time, I would have prefered to report this privately, but I still contact info for that.

Security-relevant topics can be emailed directly to project lead Richard Hipp via drh@sqlite.org.

Y'all, please verify that check-in c9a592dde7fe493f is a complete and sufficient fix for this problem. Once it is verified, I will issue a 2.11.1 version with the patch.

It's a little bit hard to review this way. I suggest to switch this to a switch / case - modern compilers will create a LUT anyway (or a partial LUT, which even saves on binary size as well) if it's faster on the target system. And in any case it should be const, so that it can end up in section .rodata instead of section .data (and also allow more optimizations, since the compiler knows it can't change).

It seems (if I made no mistake) that the following characters are allowed:

!#%&()+,-./0123456789<=>ABCDEFGHIJKLMNOPQRSTUVWXYZ]_abcdefghijklmnopqrstuvwxyz{|}

Is it really sensible to allow more than [0-9A-Za-z\-_]+ (regex)? Depending on the shell used, I would not be surprised if there's ways to execute code.

Also, I think it's probably worth reviewing all other uses of fossil_system(). I recommend to deprecate fossil_system() entirely, as system() is bad by design: The escaping is up to the user, and it's unknown to the developer what exaclty needs escaping, as that depends on the user's system. I suggest to migrate to posix_spawn() on UNIX and CreateProcessW() on Windows - these take an array of arguments, and hence don't have any escaping problems, and avoid the extra round trip via the shell.

I have not tried this, but having a quick look at http_transport.c, I'm a little worried: The repository name seems to be copied into the command to be executed. It might therefore be possible to create a repository that, when cloned, executes code. The code in particular that worries me:

zCmd = mprintf("\"%s\" http --in \"%s\" --out \"%s\" --ipaddr 127.0.0.1"
               " \"%s\" --localauth",
   g.nameOfExe, transport.zOutFile, transport.zInFile, pUrlData->name
);

I traced where zInFile and zOutFile come from, and apparently, they contain the repo name:

  transport.zOutFile = mprintf("%s-%llu-out.http",
                                   g.zRepositoryName, iRandId);
  transport.zInFile = mprintf("%s-%llu-in.http",
                                   g.zRepositoryName, iRandId);

I'm not sure how much control attacker has about zRepositoryName - if an attacker can control it, I suspect they can execute code. Similarly in winhttp.c.

Maybe it's safe. But it's hard to prove. It just shows how this code with system() is hard to review for being safe, so I think moving away from fossil_system() could close a lot of potential holes at once.

I suggest to migrate to posix_spawn() on UNIX and CreateProcessW() on Windows - these take an array of arguments, and hence don't have any escaping problems, and avoid the extra round trip via the shell.

Unfortunately, the command line arguments on Windows are always a string, probably for backwards compatibility reasons from CP/M days, so the caller of CreateProcess has to quote them for the runtime of the child process to decode the arguments back into char ** argv. One of the ways to perform that transformation is described in Daniel Colascione's blog.

The original problem has been fixed, and new 2.10.1 and 2.11.1 releases made. All binaries on the download page have this problem fixed.

Multiple passes through the code show no additional problems. But for additional security, the following changes have been made:

1. All calls to fossil_system() also invoke fossil_assert_safe_command_string() on the command string. If the command-string seems unsafe, the process aborts with a fatal error and never invokes system(). This restricts the generality of fossil_system(), but that's ok. Please audit this procedure for correctness and completeness.

2. The new "%$" substitution letter has been added to mprintf(). This works like "%s" except that it does appropriate shell escaping for filenames. The blob_append_escaped_arg() subroutine is used to do the escaping. Many of the places that build command-strings now use "%$" rather than their own home-grown escaping. Please audit this procedure for correctness and completeness.

For testing the fossil_assert_safe_command_string() safety-net, there is a new test-command on trunk:

fossil test-fossil-system

Run the command above and at each prompt, type strings to be run by the shell (/bin/sh on unix or CMD.EXE on windows). If Fossil thinks the command is unsafe, it will show you the unsafe part and refuse to run the command.

Please experiment with this. Use all your cleverness to try to devise commands that will do nefarious things. Report your findings.

What "safe" means in this context

A "safe" command is one that only runs the one command specified at the beginning of the command line. In other words, it is not possible to trick the shell into running a second command.

This is a safety net

To be clear: This mechanism is a safety net. It is a fallback to catch problems that are missed earlier in the system. We should not rely on the fossil_assert_safe_command_string() to prevent mischief. The mischief prevention should occur at a higher level. This mechanism only kicks in if the higher-level mischief prevention malfunctions. This is intended as defense-in-depth.

Any reason to not switch to a fossil_spawn() method instead? It also avoids the extra round-trip to the shell.

E.g. fish shell doesn't need a $ for subcommands, () is enough. That would still not be filtered.

Trying to escape for the shell is just more complex and has more unknowns - why prefer that over a solution that avoids the shell entirely?

Because fossil_spawn() is vaporware. The existing design compiles and runs and works and is portable across all known platforms and is (now) secure as far as anybody can tell. And I have other more pressing problems to solve at the moment.

You are welcomed to contribute code to implement fossil_spawn() if it is important to you.

Note that for many current uses of fossil_system(), a naive fossil_spawn() alternative will not work. For example, the editor used for editing check-in comments is obtained from the VISUAL environment variable. But the value of VISUAL need not be a simple command. It can be a command plus arguments, as long as it it allows the file appended to be edited. So to make VISUAL work as an editor, you either have to:

1. Learn to escape filenames properly (which we already do)

2. Parse the VISUAL variable up into a command-name plus arguments so that they can be fed into fossil_spawn().

Note also that some uses of fossil_system() deliberately run commands in the background. For example, when you run "fossil ui" and it starts up your web-browser for you automatically, that fossil_system() call includes the '&' character at the end of the command-line so that it runs in the background. Other uses of fossil_system() run in the foreground, such as when running the VISUAL editor. If you provide a fossil_spawn() implementation, be sure that it supports both modes of operation.

Yes, the places where you actually want evaluation by the shell of course make no sense to replace. I was talking about those where you don't want evaluation by the shell: Having fossil_spawn() seems easier to get right than having everything escaped properly for every single shell out there.