Bug 600636

Summary: fails lvm-snapshot schroot (device-lock)
Product: [Fedora] Fedora Reporter: Oron Peled <oron>
Component: lockdevAssignee: Jiri Popelka <jpopelka>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: 19CC: damianatorrpm, jpopelka, os, pbonzini, sergio
Target Milestone: ---Keywords: Reopened
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: lockdev-1.0.4-0.18.20111007git.fc22 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-09-18 13:54:06 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Add setuid(0) call before locking the device
none
Replace access() calls with euidaccess()
none
patch to support ACLs in the helper none

Description Oron Peled 2010-06-05 10:43:04 UTC
Created attachment 421414 [details]
Add setuid(0) call before locking the device

Description of problem:
schroot fails entering an lvm-snapshot type chroot (Fails to lock device)

Version-Release number of selected component (if applicable):
schroot-1.2.3-8.fc13.i686

This version includes the PATH fix to bug #58820 so that lvcreate/lvermove
are found now:
 * Since I failed to test it before Zach issued the new release, I didn't
   caught the current bug (my bad).
 * Now I tested it until I made it work. In the process, I found two bugs:
   - The current "locking" bug, with proposed workaround at the bottom.
   - Another trivial "snapshot-name" bug, which I'll open separately and
     list its number here.

How reproducible:
Every time you try to 'schroot -c <lvm-snapshot-chroot>' it fails
with the following error:
  [oron@radon ~]$ schroot -c test-f13
  E: test-f13-2010-06-05T09:54:58Z-507: Failed to lock chroot: /dev/vg_radon/lv_f-13: Failed to lock device: Failed to lock device

Steps to Reproduce:
1. Create a chroot environment in a logical volume
   (e.g: lvcreate, temporary mount to /mnt, febootstrap, umount /mnt)

2. Prepare a schroot definition file. Here is my /etc/schroot/chroot.d/test.conf
   [test-f13]
   type=lvm-snapshot
   description=Schroot test
   priority=3
   groups=oron
   users=oron
   source-root-users=root,oron
   source-root-groups=root
   device=/dev/vg_radon/lv_f-13
   mount-options=-o atime,sync,user_xattr
   lvm-snapshot-options=--size 1G
   run-setup-scripts=true
   run-exec-scripts=true

3. Try to run schroot:
   schroot -c test-f13

Additional info:
The same error occurs if we modify the definition to a 'block-device' type
(removing the "source-..." and "lvm-snapshot-..." lines of course).

Bug analysis:
1. The device locking fails due to a permission problem.
2. This happens in sbuild/sbuild-lock.cc in the device_lock::set_lock() method
3. Insertion of debugging prints into this function shows that at this
   point the EUID is 0 while the RUID is of the calling user (as it should be
   since 'schroot' is setuid root.
4. However, device_lock::set_lock() calls dev_lock() from a lockdev library
   and it fails due to permission problem.

Workaround:
1. The locking logic is called from session::setup_chroot() method in
   sbuild/sbuild-session.cc
2. This function already contains the required call to setuid(0) before
   running the setup scripts (line 1051).
3. However, it tries to acquire the lock before this (line 947)
4. I attached a small patch to add setuid(0) call before trying to lock the
   device.
5. This workaround may need a review from one of the security people, because
   it touches a setuid binary:
   - The code already have EUID==0, so it doesn't look problematic.
   - However, the original code only called setuid(0) withing the sub-process
     running the setup scripts -- it's a cleaner approach.

Questions to lockdev maintainer (open a bug?):
1. Why EUID==0 is not enough and RUID==0 is needed as well?
2. Are there any differences from Debian? (I use lvm-snapshot schroot on
   Debian lenny, so it looks like their lockdev semantics are different).

Comment 1 Oron Peled 2010-06-05 11:10:09 UTC
After applying the workaround attached to this bug report, the lvm-snapshot
has another (trivial) failure which is reported in bug #600639

Fixing also #600639 by adding a BR of libuuid-devel, results in a working
lvm-snapshot schroot.

Comment 2 Zach Carter 2010-06-07 20:16:54 UTC
I reproduced this with schroot 1.4.2.  I will report this upstream.

Comment 4 Zach Carter 2010-06-07 21:49:39 UTC
Comment from upstream:


Hi Zach,

This is not a bug in schroot.  It's a bug in lockdev, specifically a
patch to lockdev added in RedHat/Fedora which breaks setuid programs.
schroot works fine using the vanilla upstream/Debian lockdev without
this patch.

This is the patch:
  http://lists.alioth.debian.org/pipermail/lockdev-devel/2010-February/000013.html
and this is my review with the rationale:
  http://lists.alioth.debian.org/pipermail/lockdev-devel/2010-February/000039.html
and here's a previous finder of this issue in CentOS:
  http://www.nikhef.nl/~dennisvd/schroot.html

To summarise the above: this patch uses access(2) to see if the user
has permission to use the device.  access(2) is racy, but more
importantly uses the real UID, not the effective UID of the calling
process.  Normally this makes no difference, but in a setuid
program such as schroot, this completely breaks things.  The
suggested fix of the setuid call is AFAICT just working around
this problem; schroot itself only has a single setuid call, and
that's when switching to user context prior to invoking a
program or shell on their behalf.  All the locking and other setup
is always done as root with EUID=0.  I'm not sure why this patch was
introduced, but as it stands it's lockdev at fault here, not schroot.


Due to lack of upstream maintenance of lockdev, the Debian and Fedora
lockdev sources have diverged with the accumulation of different
patches over the last few years.  Debian was the upstream for the last
10 years or so since upstream passed away.  Over the last six months,
mainly with the assistance of Ludwig Nussel (though Karel Zak at Red
Hat was also involved initiating this), we have merged all the
outstanding RedHat patches into upstream.  You can find the git repo
and mailing list at alioth.debian.org.  Some bits were not included or
were reworked , and the access(2) checks were one of those.  We haven't
yet made a new upstream release of this unified tree, but I would
hope this is possible soon (the work is basically done, it just
needs review and testing).


Regards,
Roger

Comment 5 Zach Carter 2010-06-07 21:54:38 UTC
Changing component to lockdev, as this is not considered to be a bug in schroot.

Comment 6 Jiri Popelka 2010-06-08 11:30:43 UTC
(In reply to comment #4)
> Some bits were not included or
> were reworked , and the access(2) checks were one of those.
Upstream commit of this "rework" is here
http://git.debian.org/?p=lockdev/lockdev.git;a=commitdiff;h=5a6928053ff6d88dcb3da765748720924f9424fc

Comment 7 Bug Zapper 2011-06-02 11:59:13 UTC
This message is a reminder that Fedora 13 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 13.  It is Fedora's policy to close all
bug reports from releases that are no longer maintained.  At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '13'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 13's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that 
we may not be able to fix it before Fedora 13 is end of life.  If you 
would still like to see this bug fixed and are able to reproduce it 
against a later version of Fedora please change the 'version' of this 
bug to the applicable version.  If you are unable to change the version, 
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events.  Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

The process we are following is described here: 
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 8 Oron Peled 2011-06-19 18:44:37 UTC
We were hopefull for the last year... alas :-(

 * No new commits in the git tree mentioned in comment #6
 * We still cannot use schroot with lvm-snapshot due to this bug

Any chance of sorting this out? (If you need help, just say what)

Comment 9 Oron Peled 2012-04-12 06:50:25 UTC
Created attachment 576972 [details]
Replace access() calls with euidaccess()

 * See comment #4 for details about access() problems

 * The euidaccess() behaves as expected and schroot can be used by
   non-root users on LVM snapshots (after applying another minor build
   fix to schroot [bugzilla #811856])

 * The addition of the access() test specifically to Red-Hat (~2001) was
   not explained (it's not in the upstream source):
   - I think it was done to prevent from users who do not have write
     access to a device to lock it via lockdev (i.e: prevent denial
     of service attack)
   - I added a comment to this effect as part of the patch
   - Is there any other reason for this?

Comment 10 Oron Peled 2012-05-14 22:05:27 UTC
* New release of schroot(1):

  https://admin.fedoraproject.org/updates/FEDORA-2012-7764/schroot-1.4.25-4.fc16

* This release contains again the 'lvm-snapshot' feature. However, this lockdev
  bug prevent this feature from working.

* What about my proposed fix? Good? Bad? Something else?

Comment 11 Jiri Popelka 2012-05-25 11:26:20 UTC
Wow, that's really strange - I never got any email from Bugzilla since my last comment.

In F-17 we have lockdev-1.0.4-0.4.20111007git.fc17 which is the upstream git snapshot containing all the latest upstream changes including the one mentioned in comment #6. Can you test that one ? I think you can safely rebuild&use it in F-16 (I remember testing it in F-16 too). Does that make any change ? Do we still need your patch ?

I appreciate your work and I'm sorry for all the inconvenience.

Comment 12 Zach Carter 2012-05-25 16:30:47 UTC
I tested an lvm-snapshot in a new build of schroot with the latest lockdev in F-17, and it worked just fine.   Currently lvm-snapshot feature is not enabled in F-17, but once I get my new build in, it should be working again.

Comment 13 Zach Carter 2012-05-25 17:31:54 UTC
To test lvm-snapshot in F-17 with newest lockdev, use this build:

http://koji.fedoraproject.org/koji/buildinfo?buildID=320564

Comment 14 Oron Peled 2012-05-27 11:17:49 UTC
Zach, there must be some error in your f17 test. I've just tested it in f17 virtual
machine and it has the same (lockdev) problem.

 - Installed your new f17 build from:
     http://koji.fedoraproject.org/koji/buildinfo?buildID=320564

 - Verified lockdev version:
     $ rpm -q lockdev
     lockdev-1.0.4-0.4.20111007git.fc17.i686

 - Created a logical volume (/dev/data/squeeze)

 - Mounted it temporarily on /mnt

 - debootstrap squeeze /mnt   # so I have Debian/squeeze OS inside

 - umount /mnt

 - Created /etc/schroot/chroot.d/squeeze-i386.conf:
       [squeeze-i386]
       type=lvm-snapshot
       description=Debian squeeze LVM snapshot
       users=oron
       source-root-users=oron
       device=/dev/data/squeeze
       mount-options=-o atime,sync,user_xattr
       lvm-snapshot-options=--size 0.5G

 - Tried using as *root* user -- no problem:
       schroot -c squeeze-i386

 - Tried using as myself -- permission denied

Analayzing the problem:
 - Minor issue: one has to be in the 'lock' group to use lockdev. This isn't clearly
   documented in the man-page, but was easy to find. After adding myself to the
   'lock' group, logout, login, id (shows I'm in the lock group)

 - strace schroot -c squeeze-i386:
   ...
   umask(02)                               = 02
   stat64("/dev/data/squeeze", {st_mode=S_IFBLK|0660, st_rdev=makedev(253, 2), ...}) = 0
   access("/dev/data/squeeze", W_OK)       = -1 EACCES (Permission denied)
   umask(02)                               = 02
   ...

  - This obviously fails, because normal users should not have write (or read) access
    to disk devices. Although schroot is suid, it doesn't help, because access()
    check against 'ruid' and not 'euid'.

To debug lockdev in isolation, I created the following suid-root wrapper,
testlockdev.c:
   #include <stdio.h>
   #include <unistd.h>
   int main()
   {
           execlp("lockdev", "lockdev", "-l", "/dev/data/squeeze", NULL);
           perror("lockdev");
           return 1;
   }

Testing the wrapper:
   $ gcc -Wall -o testlockdev testlockdevs.c
   $ su -c 'chown root testlockdev'
   Password:
   $ su -c 'chmod u+s testlockdev'
   Password:
   $ ./testlockdev
   $ echo $?
   2
   $ strace ./testlockdev
   stat64("/dev/data/squeeze", {st_mode=S_IFBLK|0660, st_rdev=makedev(253, 2), ...}) = 0
   access("/dev/data/squeeze", W_OK)       = -1 EACCES (Permission denied)
   umask(02)                               = 02
   exit_group(2)                           = ?

   Fails :-(

Comment 15 Oron Peled 2012-05-27 11:35:48 UTC
The following upstream change may shed some light:
  - http://lists.alioth.debian.org/pipermail/lockdev-devel/2010-March/000058.html
  - It specifically tries to address setuid programs
  - But that code only applies to a small function that is used only for tty-locks
    (and also enclosed in some #ifdef)

Looking at the code make it clean that current lockdev cannot work for the
general case of setuid programs. Hasn't anybody else bumped into it now
that upstream version includes these buggy access() calls?

Comment 16 Zach Carter 2012-05-29 15:44:36 UTC
Dang it, you are right.   I was testing it as root.   Sorry for the false results.


[zcarter@nudj ~]$ schroot -c test-sch
W: line 12 [test-sch]: Deprecated key ‘run-setup-scripts’ used
I: This option will be removed in the future; please update your configuration
W: line 13 [test-sch]: Deprecated key ‘run-exec-scripts’ used
I: This option will be removed in the future; please update your configuration
W: line 4 [test-sch]: Deprecated key ‘priority’ used
I: This option will be removed in the future; please update your configuration
E: test-sch-fae690ab-3884-44f4-9cc4-e1b46b61bb82: Failed to lock chroot: /dev/fastvg/schtestlv: Failed to lock device: Failed to lock device

Comment 17 Damian Ivanov 2012-07-03 07:11:30 UTC
Maybe this is related but since the yesterday most fedora packages fail for me in the open build service with:
error: unpacking of archive failed on file /var/lock/lockdev: cpio: mkdir failed - No such file or directory
error: lockdev-1.0.4-0.4.20111007git.fc17.i686: install failed

Comment 18 Damian Ivanov 2012-07-04 08:05:15 UTC
nevermind this was an OBS issue https://bugzilla.novell.com/show_bug.cgi?id=769756

Comment 19 Laszlo Nagy 2012-08-09 18:12:58 UTC
Had the same issue with schroot command on F-17. Did changed the 'access' call to 'euidaccess' in lockdev, and it works just fine. Thanks to Roger for the hint! ;)

Comment 20 Fedora End Of Life 2013-04-03 20:06:14 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 19 development cycle.
Changing version to '19'.

(As we did not run this process for some time, it could affect also pre-Fedora 19 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 19 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora19

Comment 21 Sergio Basto 2013-06-02 16:28:31 UTC
Summarizing: 

The problem was discussed upstream
http://lists.alioth.debian.org/pipermail/lockdev-devel/2010-March/000052.html
and this commit
http://lists.alioth.debian.org/pipermail/lockdev-devel/2010-March/000058.html
was supposed to fix it.

or Laszlo Nagy 2012-08-09 14:12:58 EDT
Had the same issue with schroot command on F-17. Did changed the 'access' call to 'euidaccess' in lockdev, and it works just fine. Thanks to Roger for the hint! ;)

any of this is applied to lockdev ?  
this bug still not fixed ?

Comment 22 Oron Peled 2013-06-02 20:57:34 UTC
* At least regarding schroot, there's a light in the end of the tunnel:
  - The new upstream schroot-1.7.x does not depend on lockdev anymore!!!
    (It also dropped libuuid dependency)

  - I've started testing it so I can ask Zach to bump it
    (but currently still have some problems that need resolving)

* Other than that:
  - Let's see who use lockdev in Fedora:
      [root@f19 ~]# repoquery --whatrequires lockdev
      ckermit-0:9.0.302-4.fc19.i686
      dchroot-0:1.4.25-13.fc19.i686
      libcec-0:2.1.3-1.fc19.i686
      libgphoto2-0:2.5.2-1.fc19.i686
      lockdev-devel-0:1.0.4-0.6.20111007git.fc19.i686
      mgetty-0:1.1.36-21.fc19.i686
      mgetty-sendfax-0:1.1.36-21.fc19.i686
      mgetty-voice-0:1.1.36-21.fc19.i686
      minicom-0:2.6.2-1.fc19.i686
      schroot-0:1.4.25-13.fc19.i686
      ttywatch-0:0.14-17.fc19.i686
      uucp-0:1.07-36.fc19.i686
      wvdial-0:1.61-7.fc19.i686

  - IIRC, the bad code-path in lockdev only affect storage devices,
    so if I'm correct, without schroot it only leaves: libgphoto2

* So far this lockdev saga:
  - Doesn't seem to end in my lifetime :-(

  - The buggy patches seem to be some RHEL legacy stuff

  - As a result, probably nobody want to fix them
    (maintainer had 2 relevant comments in 3 years).

  - Let's nominate it "abandonware of the year" ;-)

Comment 23 Sergio Basto 2013-06-02 21:20:52 UTC
but if new schroot solves the problem let focus on update it 
   
   # repoquery  --releasever=rawhide --disablerepo=\*updates\* -q schroot
schroot-0:1.4.25-13.fc20.x86_64

   # repoquery  --releasever=rawhide --disablerepo=\*updates\* -q schroot --whatrequires 
(nothing) 

if say schroot 1.7 seems 1.4 is pretty old

Comment 24 Sergio Basto 2013-06-02 22:38:26 UTC
opened bug #969885

Comment 25 Jiri Popelka 2013-06-03 11:28:16 UTC
(In reply to Sergio Monteiro Basto from comment #21)
> or Laszlo Nagy 2012-08-09 14:12:58 EDT
> Had the same issue with schroot command on F-17. Did changed the 'access'
> call to 'euidaccess' in lockdev, and it works just fine. Thanks to Roger for
> the hint! ;)
> 
> any of this is applied to lockdev ?  

Sorry.

I just applied patch from comment #9.
http://koji.fedoraproject.org/koji/taskinfo?taskID=5458117

Comment 26 Fedora Update System 2013-06-04 08:06:58 UTC
lockdev-1.0.4-0.7.20111007git.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/lockdev-1.0.4-0.7.20111007git.fc19

Comment 27 Zach Carter 2013-06-04 15:48:22 UTC
Tested schroot in F19 with the new lockdev update and it seems to be working.   Cool!

Comment 28 Fedora Update System 2013-06-05 02:30:09 UTC
Package lockdev-1.0.4-0.7.20111007git.fc19:
* should fix your issue,
* was pushed to the Fedora 19 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing lockdev-1.0.4-0.7.20111007git.fc19'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-10002/lockdev-1.0.4-0.7.20111007git.fc19
then log in and leave karma (feedback).

Comment 29 Sergio Basto 2013-06-06 05:03:05 UTC
(In reply to Zach Carter from comment #27)
> Tested schroot in F19 with the new lockdev update and it seems to be
> working.   Cool!

fix the initial problem ? (schroot fails entering an lvm-snapshot type chroot (Fails to lock device) ) 

I think we also should build this package for F18

Comment 30 Fedora Update System 2013-06-06 06:45:41 UTC
lockdev-1.0.4-0.6.20111007git.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/lockdev-1.0.4-0.6.20111007git.fc18

Comment 31 Fedora Update System 2013-06-11 09:03:25 UTC
lockdev-1.0.4-0.6.20111007git.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 32 Fedora Update System 2013-06-12 03:34:16 UTC
lockdev-1.0.4-0.7.20111007git.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 33 Paolo Bonzini 2014-09-18 12:17:25 UTC
Created attachment 938865 [details]
patch to support ACLs in the helper

Unfortunately, the fix breaks another case.  The problem is that euidaccess does not honor ACLs; it just checks against the file uid and gid.

In my setup, I'm /dev/ttyS0 is granted console user access via udev, so that it has these ACLs:

    # file: dev/ttyUSB0
    # owner: root
    # group: dialout
    user::rw-
    user:pbonzini:rw-
    group::rw-
    mask::rw-
    other::---

When ttylock is called, the real and effective uids are pbonzini.  However, euidaccess returns false because it doesn't look at ACLs, and /usr/sbin/lockdev fails.

I think you should use "access" from the helper, since the caller helpfully swaps the real and effective uids for it.  The attached patch (a replacement for the existing lockdev-euidaccess.patch) does it by embedding the lockdev library into the helper, instead of linking to the shared library.

With this patch,

    minicom -D /dev/ttyUSB0

works for me instead of failing with EACCES.

Alternative approaches include:

- instead of euidaccess, make dev_lock/dev_unlock fork a child process and do setreuid+setregid+access.  Communicate the result via the exit code; remove the now redundant setreuid+retregid pair in _spawn_helper.  Same result as euidaccess, but with correct handling of ACLs.  But much more invasive.

- add a function pointer that defaults to euidaccess(), set it to access() from the helper's main().

Comment 34 Jiri Popelka 2014-09-18 13:54:06 UTC
Thanks Paolo !