That's very sneaky.

In the suggested fixes, I believe the first option to be best. SQL archives do need to represent symbolic links (just as zip and tar do), and remember that Fossil is one of the big users of SQL archives (not just for manual download, but the fossil get command uses them internally), and version controlled files often include symbolic links. O_NOFOLLOW is probably fine assuming that archives are only created in a single shot and intended to be extracted again, but as archives can be appended at any time, there is a possibility the user stores a file path with a symbolic link component somewhere in the middle. It may be too much of a burden on the user-side if it behaves oddly.

It's also worth noting the true source line for the arExtractCommand function is in file shell.c.in, line 6666. The line 27827 referenced is a generated file.

You can find bad symlink/file combinations pretty cheaply.

select esc.name as "symlink entry", bad.name as "suspect entry"
    from sqlar esc,
        sqlar bad on bad.name>=esc.name||'/' and bad.name<esc.name||'0'
    where esc.mode&0xF000=0xA000;

That'd flag my “there is a possibility the user stores a file path with a symbolic link component somewhere in the middle” scenario as a false positive. It's a contrived example, but I think it's one that ought to be considered safe, given that it lives entirely in the tree instead of trying to escape it. One might even imagine that using the symlink path is a shortcut in lieu of typing out a longer and deeper path.

mkdir -p dir/a
ln -s a dir/b
sqlite3 dir.sqlar -Ac dir
touch dir/b/c
sqlite3 dir.sqlar -Au dir/b/c

Perhaps even in this circumstance, the -u/--update command shouldn't store the dir/b/c path, but dir/a/c instead. Dereference path components except for the last component, and this issue becomes moot, and the -x/--extract command might start refusing to create such entries. (-U/--unsafe override possible, especially for backwards compatibility?)

A single call to realpath is insufficient, since it only works for existing filesystem objects. You'd have to either call it for each component of the destination path (expensive) or write your own variant (complicated and easy to get wrong).

I made a couple of attempts to add a realpath() SQL function to the fileio.c extension, in order to add realpath() tests to the SQL query that actually performs the extraction. But that didn't work out.

Then I tried to simply not extract files that are children of a symlink. That works, but before checking it in, I realized that a case-insensitive filesystem can make it tricky to determine if a file is a child of a symlink or not. Maybe I could work through that, but it would be locale-dependent and error prone.

My next idea was to do symlink creation last, after all files and directories have already been created. That way, a symlink created by the archive cannot be used an escape out of the destination directory. That's the solution currently on trunk.

I acknowledge extracting symlinks last, and raise you: it won't work. TBH it's like the recent Microsoft patch, "I know, we'll fix this security issue of 'users can create the directory' by creating the directory first! That way, they can't create a directory at that path with different permissions!!"

file: sdr.cnf: 'ALL ALL=(ALL) NOPASSWD:ALL'
symlink: sudoers.d -> /etc/sudoers.d
symlink: sudoers.d/sudoers.conf -> /data/base/path/sdr.cnf

(Maybe sudoers doesn't follow symlinks for config - but you could do the same thing with an suid file into /usr/bin, overwriting one that already exists.)

You might be able to fix things if you extract longest symlink paths first:

symlink: biglongnamepath123 -> /etc/sudoers.d
symlink: shortpath/x.conf -> /data/base/path/sdr.cnf
symlink: shortpath -> biglongnamepath123

.... hmm. (Because of the ../ check.) I'm uncertain if there's a work-around here.

Another option: stat the destination director(ies) on each extract, and don't extract through symlinks. This remedies filename insensitivity (assuming that prevents two file/symlink names, with different case, and not the case in the database). However then you have to keep track of every symlink that you created in the database engine (and if you intend to symlink to an existing source, with the intent of extracting data across the symlink -> fail).

This is almost a case of executing binary code included in an untrusted (archive|executable|dbfile): executor beware, why are you doing this? Something is almost certainly wrong with the system design.. but, backward compatibility.

There may be people who (errantly!) depend on this. Create a symlink, ./extract-path, that points to wherever they want items to be placed? extract to extract-path, and things go to the correct location on the filesystem? Again, it's kind of a thing about untrusted data and allowing random/unknown data and directory trees (from an untrusted archive file) to be placed on the filesystem. (Even with realpath, potentially backward compatibility.)

This is almost a case of executing binary code included in an untrusted (archive|executable|dbfile): executor beware, why are you doing this? Something is almost certainly wrong with the system design.. but, backward compatibility.

To be honest, I thought of that with this issue report when I first read it. I know that zip and tar have their mitigations, but on some level, you shouldn't extract archives from untrustworthy sources. That might even include servers that claim to be Fossil (or claim to be running a normal Fossil revision, it wouldn't be hard to fake the version information...).

I've often lived by a rule of listing archives before extracting them. At the minimum, --list --verbose (-tv) doesn't show the symlink target. It really should.

A new attempt, using the original realpath() approach, is now on trunk.

Note that I had to make substantial enhancements to the realpath() SQL function (to do things that the C-library realpath() won't do - but Windows _wfilepath() does do) in order to get this to work.

Please try to break it now.

It passes my contrived “safe” example. I think I'm happy about that. :-)

Using the example I laid out in post 5, SQLite's still able to create an archive in this manner, but it is unable to extract the dir/b/c member, with SQL error: failed to create symlink: dir/b. To a point, one can always craft the sqlar table directly (as the opening post does), but it's counter-intuitive for the .archive/-A commands to be able to create a particular structure without being able to subsequently extract it.