Harden ssh_filter script about btrbk HOT 7 CLOSED

Hi!
Thanks for your contribution, you opened an issue that I wanted to address since the beginning of btrbk development. The reasons why I never implemented this are:

• I wanted to keep the script as simple as possible
• Posix shell is (as far as I know) incapable of doing regular expressions reasonably
• There are people that don't have bash (for good reasons)
• Implementing proper security in bash is still a pain
• Writing bash is always a pain...

Well nevertheless, I took your proposed script, changed the regular expressions to be as readable as possible, and added some more options (you can now specify which type of access is allowed: source/target/delete/info, as well as specify multiple path restrictions using command-line options):

https://github.com/digint/btrbk/blob/harden_ssh_filter_btrbk/ssh_filter_btrbk.sh

example: ssh_filter_btrbk.sh --log --target --delete --info -p /mnt/btr_pool/alpha -p /mnt/btr_pool/beta
(Maybe the command-line option naming should be changed to something more intuitive.)

Let me know what you think, I guess I'll merge this into the next release as I think it's a big improvement, even if it relies on bash now.

from btrbk.

Nice improvement, the possibility of choosing which btrfs command can be launch and multiple path are good addition.

But I've encountered two bugs in the script :

1. The log_cmd raise an error :
   [not exact error : translated from french]
   Bad substitution: no closing « } » in ; Restrict-path: '/${restrict_path//\|/\', \'/
   (what is this part supposed to do ? [not good in bash :) ]
2. The regular expression forces a trailing slash at the end with a filepath:

I've this btrbk configuration :

volume /btrfs/pool_hdd
     snapshot_create  always
     subvolume home/alex
          snapshot_dir               home_snapshots/
          snapshot_name          alex.home
          target send-receive     ssh://nas/btrfs/pool_1/backups/alex-pc

So, btrbk tree and btrbk info send respectively these commands:

btrfs subvolume show /btrfs/pool_1/backups/alex-pc
btrfs filesystem usage /btrfs/pool_1/backups/alex-pc

Which are rejected because not matching the regex.

• I think the regular expression should be something like:

^(${btrfs_allow})( ${option_match})*( /(${restrict_path})(/|/${file_match})*)+$

• and deleting the trailing slash added at line 31 : restrict_path="${restrict_path}|${rpath}"

In this case:

• the path can be the restricted path only without subvolume
  • without trailing slash at the end (like btrbk the commands show and list)
  • or with one optionally (like the receiving command: btrfs receive /btrfs/pool_1/backups/alex-pc/)
• the path can contain optionally a subvolume for deletion command for example

Is it good for you?
Thanks!

from btrbk.

The log_cmd raise an error : [...]
oh, well I hacked this without really testing... guess I'll have to adapt the logging line anyways as it gets really long

Regarding the trailing slash: I basically added this in order to make sure that the restricted path is really a directory and not just a prefix, which I think is a good idea. The regex should read "use exact restrict-path or restrict-path-slash-somefile". Your regex fix above kind of fixes that too, but gets more unreadable again... I'll look into it again, this time with more "real-life" testing.

from btrbk.

Another try with no leading-slash-magic, seems pretty readable and correct to me. Also performed some successful tests:

https://github.com/digint/btrbk/blob/harden_ssh_filter_btrbk/ssh_filter_btrbk.sh

from btrbk.

Seems good to me !
I can't try it these next two days. I'll give you feedback on monday after testing.
Thank you for working this fast on the problem !

from btrbk.

The script seems to be working perfectly.
Command line options tested: log / sudo / restrict path (one path and two paths) / source / delete / info
All commands works correctly with and without trailing slash / with and without subvolume after the path / rejected when out of path, no directory traversal, rejected when path is used as a prefix / logging works
What was not tested: option source (and snapshot / send) but I see no reason why this won't work !

Thank you for working on it !

from btrbk.

Thanks for the testing!

Merged into master: f01304d

from btrbk.