Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"btrfs send -p" fails if source and parent subvolumes are on different mountpoints (memory corruption) #96

Closed
digint opened this issue Feb 14, 2018 · 4 comments
Closed

"btrfs send -p" fails if source and parent subvolumes are on different mountpoints (memory corruption) #96

digint opened this issue Feb 14, 2018 · 4 comments
Labels
bug
Milestone
v4.16

Comments

@digint
Copy link
Contributor

digint commented Feb 14, 2018

If source and parent subvolumes are on different mountpoints, btrfs send -p <parent> <source> fails with memory corruption.

In cmds-send.c, it is wrongly assumed that source and parent share the same mountpoint [1], leading to subvol_strip_mountpoint() returning a char* pointer which can exceed the length of full_path (no bounds checking) [2].

[1] https://github.com/kdave/btrfs-progs/blob/v4.15/cmds-send.c#L657
[2] https://github.com/kdave/btrfs-progs/blob/v4.15/utils.c#L2490

Steps to reproduce:

# mkdir /tmp/btrfs_unittest
# truncate -s 200M /tmp/btrfs_unittest/btrfs0
# losetup /dev/loop0 /tmp/btrfs_unittest/btrfs0
# mkfs.btrfs -L BTRFS_UNITTEST /dev/loop0

# mkdir /tmp/btrfs_unittest/mnt_root
# mount /dev/loop0 /tmp/btrfs_unittest/mnt_root

# mkdir /tmp/btrfs_unittest/mnt_snapshots
# btrfs sub create /tmp/btrfs_unittest/mnt_root/snapshots
# mount -o subvol=snapshots /dev/loop0 /tmp/btrfs_unittest/mnt_snapshots

# btrfs sub create /tmp/btrfs_unittest/mnt_root/test0
# btrfs sub snap -r /tmp/btrfs_unittest/mnt_root/test0 /tmp/btrfs_unittest/mnt_snapshots/test0_snap_mnt_snapshots
# btrfs sub snap -r /tmp/btrfs_unittest/mnt_root/test0 /tmp/btrfs_unittest/mnt_root/test0_snap_mnt_root

# btrfs send -p /tmp/btrfs_unittest/mnt_snapshots/test0_snap_mnt_snapshots /tmp/btrfs_unittest/mnt_root/test0_snap_mnt_root > /dev/null
ERROR: open hots/test0_snap_mnt_snapshots failed. No such file or directory
ERROR: could not resolve rootid for /tmp/btrfs_unittest/mnt_snapshots/test0_snap_mnt_snapshots
digint added a commit to digint/btrfs-progs that referenced this issue Feb 17, 2018
@digint
btrfs-progs: prevent incorrect use of subvol_strip_mountpoint
34e9bc4 
Add additional bound checks to prevent memory corruption on incorrect
usage of subvol_strip_mountpoint. Assert sane return value by properly
comparing the mount point to the full_path before stripping it off.

Mitigates issue: "btrfs send -p" fails if source and parent subvolumes
are on different mountpoints (memory corruption):

    kdave#96

Note that this does not properly fix this bug, but prevents a possible
security issue by unexpected usage of "btrfs send -p".

Signed-off-by: Axel Burri <axel@tty0.ch>
@digint
Copy link
Contributor Author

digint commented Feb 17, 2018

Added pull request: #98

With the patch applied, the test above gives:

# btrfs send -p /tmp/btrfs_unittest/mnt_snapshots/test0_snap_mnt_snapshots /tmp/btrfs_unittest/mnt_root/test0_snap_mnt_root > /dev/null
ERROR: not on mount point: /tmp/btrfs_unittest/mnt_root

Still an error, but at least the memory corruption is gone.

@digint digint mentioned this issue Feb 17, 2018
Open
@kdave kdave added the bug label Feb 19, 2018
@kdave kdave added this to the v4.15.2 milestone Feb 19, 2018
kdave pushed a commit that referenced this issue Feb 19, 2018
@digint @kdave
btrfs-progs: prevent incorrect use of subvol_strip_mountpoint
c5dc299 
Add additional bound checks to prevent memory corruption on incorrect
usage of subvol_strip_mountpoint. Assert sane return value by properly
comparing the mount point to the full_path before stripping it off.

Mitigates issue: "btrfs send -p" fails if source and parent subvolumes
are on different mountpoints (memory corruption):

    #96

Note that this does not properly fix this bug, but prevents a possible
security issue by unexpected usage of "btrfs send -p".

Issue: #96
Pull-request: #98
Signed-off-by: Axel Burri <axel@tty0.ch>
Signed-off-by: David Sterba <dsterba@suse.com>
kdave added a commit that referenced this issue Feb 19, 2018
@kdave
btrfs-progs: tests: add test for send -p on 2 mont points
ea0956b 
Add testcase from issue, use reproducer from Axel Burri.

Issue: #96
Signed-off-by: David Sterba <dsterba@suse.com>
@kdave
Copy link
Owner

kdave commented Feb 19, 2018

I've created a testcase from the instruction you provided. Closing, thanks.

@kdave kdave closed this as completed Feb 19, 2018
kdave pushed a commit that referenced this issue Mar 9, 2018
@digint @kdave
btrfs-progs: prevent incorrect use of subvol_strip_mountpoint
a8a33c8 
Add additional bound checks to prevent memory corruption on incorrect
usage of subvol_strip_mountpoint. Assert sane return value by properly
comparing the mount point to the full_path before stripping it off.

Mitigates issue: "btrfs send -p" fails if source and parent subvolumes
are on different mountpoints (memory corruption):

    #96

Note that this does not properly fix this bug, but prevents a possible
security issue by unexpected usage of "btrfs send -p".

Issue: #96
Pull-request: #98
Signed-off-by: Axel Burri <axel@tty0.ch>
Signed-off-by: David Sterba <dsterba@suse.com>
kdave added a commit that referenced this issue Mar 9, 2018
@kdave
btrfs-progs: tests: add test for send -p on 2 mont points
e2831e7 
Add testcase from issue, use reproducer from Axel Burri.

Issue: #96
Signed-off-by: David Sterba <dsterba@suse.com>
@kdave kdave modified the milestones: v4.15.2, v4.16 Mar 21, 2018
digint added a commit to digint/btrbk that referenced this issue May 10, 2018
@digint
btrbk: always read mountpoints; include all snapshots from mountpoint…
b37ef84 
… as candidates for best common parent

Dropped readin of subvolid and realpath by btrfs_subvolume_show(), we
now always read /proc/self/mounts (and call readlink).

When picking the best common parent in get_best_parent(), we want to
list as many snapshots as possible. For now, we list all from the
mountpoint of snaproot ($sroot/<snapshot_dir>), due to a bug in
btrfs-progs [1]. Also added code (commented out) to list snapshots
from all known mountpoints.

  [1] kdave/btrfs-progs#96
@niner
Copy link

niner commented May 11, 2018

This workaround seems to have killed btrfs send for me completely.
I have snapshots of / in /backup/ like /backup/sphinx-2018-04-29 and /backup/sphinx-2018-05-09
btrfs send -p /backup/sphinx-2018-04-29 /backup/sphinx-2018-05-09
always fails with "ERROR: not on mount point: /" which seems wrong since both snapshots definitely are on the same mount point. Only /sys, /proc, /dev, /run, /boot, /data and /backup/target are different file systems. Moving the snapshots out of /backup and into / did not help.
Downgrading btrfsprogs to 4.15 gave me a working backup again.

@digint
Copy link
Contributor Author

digint commented May 15, 2018

I was able to reproduce this, and added pull request #138 which fixes this problem.

digint added a commit to digint/btrbk that referenced this issue Apr 5, 2019
@digint
btrbk: add "incremental_resolve" configuration option
514e692 
Allowed values for "incremental_resolve":

 - "mountpoint" (default): Use parents in the filesystem tree below
   mount points of source `<volume-directory>/<snapshot-dir>` and
   target `<target-directory>`.

 - "directory": Use parents strictly below source/target
   directories. Useful when restricting access, e.g. when using
   ssh_filter_btrbk.sh.

 - "_all_accessible" (experimental): Use parents from all mount points.

Note that using "_all_accessible" causes btrfs-progs to fail:

  - btrfs send -p: "ERROR: not on mount point: /path/to/mountpoint"
  - btrfs receive: "ERROR: parent subvol is not reachable from inside the root subvol"

see also: kdave/btrfs-progs#96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v4.16
Development

No branches or pull requests
3 participants
@niner @kdave @digint