Bug#696424: Possible patch
Hi David I uploaded your package, with the following small change: Version number changed from 2.2-1.1 (NMU version number scheme) to 2.2-2 (as you are maintainer). Not required, but would help: Add patch header to the patches you add in debian/patch, see [1]. [1]: http://dep.debian.net/deps/dep3/ Regards, Salvatore -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#696424: Possible patch
Hi Salvatore, On Wed, Jan 16, 2013 at 08:20:26AM +0100, Salvatore Bonaccorso wrote: > Hi David and Guido > > Thanks for the further update. Guido are you sponsoring also this > upload from David (as you might better know sanlock). If you have not > time at the moment I can try to to upload David's update in the > comming days. I'd be great if you could sponsor this since you ironed out all the details with David! Cheers, -- Guido > > Regards, > Salvatore > -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#696424: Possible patch
Hi David and Guido Thanks for the further update. Guido are you sponsoring also this upload from David (as you might better know sanlock). If you have not time at the moment I can try to to upload David's update in the comming days. Regards, Salvatore -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#696424: Possible patch
Hi Salvatore > Hi David > > On Thu, Jan 10, 2013 at 10:16:35AM +, David Weber wrote: > > > Hi David > > > > > > On Mon, Jan 07, 2013 at 09:06:53AM +, David Weber wrote: > > > > > Attached is the debdiff contianing these three refreshed for the > > > > > version in unstable and testing. But I'm not yet ready to propose a > > > > > NMU. Testing of the resulting package is welcome! > > > > > > > > Thanks for the debdiff! > > > > > > > > It works as expected: It creates the files with the right > > > > permissions without breaking functionality. > > > > > > > > A problem could be that the files aren't freshly created by a simple > > > > restart of the daemon. Should something be done about that? > > > > > > > > Some options could be: > > > > - Notify the user to stop libvirtd and sanlock and run > > > > rm /var/run/sanlock/sanlock.sock; rm /var/log/sanlock.log > > > > > > > > - Change the file permissions through the package update > > > > > > > > - Do nothing because most likely nobody uses sanlock on Debain atm. > > > > > > I have not a final answer here, but it might be easy to implement like > > > libvirt-bin does in postint, mabye only conditionally checking (so > > > doing it during package update from a 'broken' version): > > > > > [...] > > > > if ! dpkg-statoverride --list "/var/log/sanlock.log" >/dev/null 2>&1; > > > > then > > > # fix permissions > > > fi > > > [...] > > > > > > and the same for /var/run/sanlock/sanlock.sock. > > > > Great hint. I modified the patch in that way and also added the > > fix for #689696 > > Btw, after thinking about further on it: As both /var/log/sanlock.log > and /var/run/sanlock/sanlock.sock are not files installed by the > package, I think the check with dpkg-statoverride is in this case > wrong! Sorry about the wrong suggestion. > > So I think it's best to remove this again. Ops, thats right. I now check the permissions and change them in case they are wrong > > Regarding the second: I suggest to include in this upload only fixes > compliant with the freeze policy: > > [1]: http://release.debian.org/wheezy/freeze_policy.html > > (but I have not looked if #689696 can be considered RC). Since it is a build fix, I guess it classifys > > +sanlock (2.2-1.1) unstable; urgency=low > + > + * Fix CVE-2012-5638 sanlock world writable /var/log/sanlock.log. Thanks to > Salvatore Bonaccorso > (Closes: #696424) > > would wrap this line > > + Add patches cherry-picked from git repository: > + - 0001-sanlock-remove-umask-0.patch > + - 0001-sanlock-use-lockfile-mode-644.patch > + - 0001-wdmd-use-lockfile-mode-644.patch > + * Replace restrict field name (Closes: #689696) > + Add patche cherry.picked from git repository: > > ^ s{patche}{patch} and s{cherry.picked}{cherry picked} Ops, fixed > > Again thanks for your work! Thank you too! > > Regards, > Salvatore Cheers, David To: car...@debian.org 696...@bugs.debian.org Cc: martin.quin...@loria.fr j...@inutil.org a...@sigxcpu.org sanlock_cve2.debdiff Description: Binary data
Bug#696424: Possible patch
Hi David On Thu, Jan 10, 2013 at 10:16:35AM +, David Weber wrote: > > Hi David > > > > On Mon, Jan 07, 2013 at 09:06:53AM +, David Weber wrote: > > > > Attached is the debdiff contianing these three refreshed for the > > > > version in unstable and testing. But I'm not yet ready to propose a > > > > NMU. Testing of the resulting package is welcome! > > > > > > Thanks for the debdiff! > > > > > > It works as expected: It creates the files with the right > > > permissions without breaking functionality. > > > > > > A problem could be that the files aren't freshly created by a simple > > > restart of the daemon. Should something be done about that? > > > > > > Some options could be: > > > - Notify the user to stop libvirtd and sanlock and run > > > rm /var/run/sanlock/sanlock.sock; rm /var/log/sanlock.log > > > > > > - Change the file permissions through the package update > > > > > > - Do nothing because most likely nobody uses sanlock on Debain atm. > > > > I have not a final answer here, but it might be easy to implement like > > libvirt-bin does in postint, mabye only conditionally checking (so > > doing it during package update from a 'broken' version): > > > > [...] > > if ! dpkg-statoverride --list "/var/log/sanlock.log" >/dev/null 2>&1; then > > # fix permissions > > fi > > [...] > > > > and the same for /var/run/sanlock/sanlock.sock. > > Great hint. I modified the patch in that way and also added the > fix for #689696 Btw, after thinking about further on it: As both /var/log/sanlock.log and /var/run/sanlock/sanlock.sock are not files installed by the package, I think the check with dpkg-statoverride is in this case wrong! Sorry about the wrong suggestion. So I think it's best to remove this again. Regarding the second: I suggest to include in this upload only fixes compliant with the freeze policy: [1]: http://release.debian.org/wheezy/freeze_policy.html (but I have not looked if #689696 can be considered RC). +sanlock (2.2-1.1) unstable; urgency=low + + * Fix CVE-2012-5638 sanlock world writable /var/log/sanlock.log. Thanks to Salvatore Bonaccorso (Closes: #696424) would wrap this line +Add patches cherry-picked from git repository: + - 0001-sanlock-remove-umask-0.patch + - 0001-sanlock-use-lockfile-mode-644.patch + - 0001-wdmd-use-lockfile-mode-644.patch + * Replace restrict field name (Closes: #689696) +Add patche cherry.picked from git repository: ^ s{patche}{patch} and s{cherry.picked}{cherry picked} Again thanks for your work! Regards, Salvatore signature.asc Description: Digital signature
Bug#696424: Possible patch
> Hi David > > On Mon, Jan 07, 2013 at 09:06:53AM +, David Weber wrote: > > > Attached is the debdiff contianing these three refreshed for the > > > version in unstable and testing. But I'm not yet ready to propose a > > > NMU. Testing of the resulting package is welcome! > > > > Thanks for the debdiff! > > > > It works as expected: It creates the files with the right > > permissions without breaking functionality. > > > > A problem could be that the files aren't freshly created by a simple > > restart of the daemon. Should something be done about that? > > > > Some options could be: > > - Notify the user to stop libvirtd and sanlock and run > > rm /var/run/sanlock/sanlock.sock; rm /var/log/sanlock.log > > > > - Change the file permissions through the package update > > > > - Do nothing because most likely nobody uses sanlock on Debain atm. > > I have not a final answer here, but it might be easy to implement like > libvirt-bin does in postint, mabye only conditionally checking (so > doing it during package update from a 'broken' version): > > [...] > if ! dpkg-statoverride --list "/var/log/sanlock.log" >/dev/null 2>&1; then > # fix permissions > fi > [...] > > and the same for /var/run/sanlock/sanlock.sock. Great hint. I modified the patch in that way and also added the fix for #689696 Guido, can you pull that debdiff directly or should I send you an updated debian.tar.gz? > > Regards, > Salvatore To: car...@debian.org Cc: martin.quin...@loria.fr 696...@bugs.debian.org j...@inutil.org a...@sigxcpu.org sanlock_cve.debdiff Description: Binary data
Bug#696424: Possible patch
Hi David On Mon, Jan 07, 2013 at 09:06:53AM +, David Weber wrote: > > Attached is the debdiff contianing these three refreshed for the > > version in unstable and testing. But I'm not yet ready to propose a > > NMU. Testing of the resulting package is welcome! > > Thanks for the debdiff! > > It works as expected: It creates the files with the right > permissions without breaking functionality. > > A problem could be that the files aren't freshly created by a simple > restart of the daemon. Should something be done about that? > > Some options could be: > - Notify the user to stop libvirtd and sanlock and run > rm /var/run/sanlock/sanlock.sock; rm /var/log/sanlock.log > > - Change the file permissions through the package update > > - Do nothing because most likely nobody uses sanlock on Debain atm. I have not a final answer here, but it might be easy to implement like libvirt-bin does in postint, mabye only conditionally checking (so doing it during package update from a 'broken' version): [...] if ! dpkg-statoverride --list "/var/log/sanlock.log" >/dev/null 2>&1; then # fix permissions fi [...] and the same for /var/run/sanlock/sanlock.sock. Regards, Salvatore signature.asc Description: Digital signature
Bug#696424: Possible patch
> Attached is the debdiff contianing these three refreshed for the > version in unstable and testing. But I'm not yet ready to propose a > NMU. Testing of the resulting package is welcome! Thanks for the debdiff! It works as expected: It creates the files with the right permissions without breaking functionality. A problem could be that the files aren't freshly created by a simple restart of the daemon. Should something be done about that? Some options could be: - Notify the user to stop libvirtd and sanlock and run rm /var/run/sanlock/sanlock.sock; rm /var/log/sanlock.log - Change the file permissions through the package update - Do nothing because most likely nobody uses sanlock on Debain atm. Cheers, David sanlock_2.2-1.1.debdiff Description: Binary data
Bug#696424: Possible patch
Hi Only a small follow-up. David (Maintainer of sanlock) will have a look at this in the upcoming week. Regards, Salvatore -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#696424: Possible patch
Hi On Mon, Dec 24, 2012 at 10:29:24PM +0100, Martin Quinson wrote: > attached is a possible patch for that issue. This is just a starting > point, as I was not able to test the patch myself. Also, I used 660 as > permissions to the file, I'm not sure of whether it's sensible or not. > > Please review and test before applying. I had too a look at this vulnerability during looking open RC bugs for wheezy. I had a look at the upstream git repository and there are at least [1], [2] and [3]. [1]: http://git.fedorahosted.org/cgit/sanlock.git/commit/?id=3a2ba2d0fbe78f4eacd438b708ceff6e96903d37 [2]: http://git.fedorahosted.org/cgit/sanlock.git/commit/?id=1339694c3bad23055f896e90353c81fd65bd4a7e [3]: http://git.fedorahosted.org/cgit/sanlock.git/commit/?id=9b13cb12973fac422423eec1c6a91f21b5257c92 Attached is the debdiff contianing these three refreshed for the version in unstable and testing. But I'm not yet ready to propose a NMU. Testing of the resulting package is welcome! David, are you working too on it? Regards Salvatore diff -Nru sanlock-2.2/debian/changelog sanlock-2.2/debian/changelog --- sanlock-2.2/debian/changelog2012-06-04 15:33:14.0 +0200 +++ sanlock-2.2/debian/changelog2013-01-03 22:12:48.0 +0100 @@ -1,3 +1,14 @@ +sanlock (2.2-1.1) unstable; urgency=low + + * Non-maintainer upload. + * Fix CVE-2012-5638 sanlock world writable /var/log/sanlock.log. +Add patches cherry-picked from git repository: + - 0001-sanlock-remove-umask-0.patch + - 0001-sanlock-use-lockfile-mode-644.patch + - 0001-wdmd-use-lockfile-mode-644.patch (Closes: #696424) + + -- Salvatore Bonaccorso Thu, 03 Jan 2013 22:12:33 +0100 + sanlock (2.2-1) unstable; urgency=low * Initial release. (Closes: #669102) diff -Nru sanlock-2.2/debian/patches/0001-sanlock-remove-umask-0.patch sanlock-2.2/debian/patches/0001-sanlock-remove-umask-0.patch --- sanlock-2.2/debian/patches/0001-sanlock-remove-umask-0.patch 1970-01-01 01:00:00.0 +0100 +++ sanlock-2.2/debian/patches/0001-sanlock-remove-umask-0.patch 2013-01-03 22:12:48.0 +0100 @@ -0,0 +1,23 @@ +From 9b13cb12973fac422423eec1c6a91f21b5257c92 Mon Sep 17 00:00:00 2001 +From: David Teigland +Date: Fri, 3 Aug 2012 14:24:07 -0500 +Subject: [PATCH] sanlock: remove umask 0 + +Remove umask(0) which causes sanlock.log to have mode 666. +It's 644 without the umask. + +Signed-off-by: David Teigland +--- + src/main.c |1 - + 1 file changed, 1 deletion(-) + +--- a/src/main.c b/src/main.c +@@ -1198,7 +1198,6 @@ + log_tool("cannot fork daemon\n"); + exit(EXIT_FAILURE); + } +- umask(0); + } + + /* main task never does disk io, so we don't really need to set diff -Nru sanlock-2.2/debian/patches/0001-sanlock-use-lockfile-mode-644.patch sanlock-2.2/debian/patches/0001-sanlock-use-lockfile-mode-644.patch --- sanlock-2.2/debian/patches/0001-sanlock-use-lockfile-mode-644.patch 1970-01-01 01:00:00.0 +0100 +++ sanlock-2.2/debian/patches/0001-sanlock-use-lockfile-mode-644.patch 2013-01-03 22:12:48.0 +0100 @@ -0,0 +1,21 @@ +From 1339694c3bad23055f896e90353c81fd65bd4a7e Mon Sep 17 00:00:00 2001 +From: David Teigland +Date: Thu, 2 Aug 2012 11:27:54 -0500 +Subject: [PATCH] sanlock: use lockfile mode 644 + +Signed-off-by: David Teigland +--- + src/lockfile.c |2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +--- a/src/lockfile.c b/src/lockfile.c +@@ -47,7 +47,7 @@ + + snprintf(path, PATH_MAX, "%s/%s", dir, name); + +- fd = open(path, O_CREAT|O_WRONLY|O_CLOEXEC, 0666); ++ fd = open(path, O_CREAT|O_WRONLY|O_CLOEXEC, 0644); + if (fd < 0) { + log_error("lockfile open error %s: %s", + path, strerror(errno)); diff -Nru sanlock-2.2/debian/patches/0001-wdmd-use-lockfile-mode-644.patch sanlock-2.2/debian/patches/0001-wdmd-use-lockfile-mode-644.patch --- sanlock-2.2/debian/patches/0001-wdmd-use-lockfile-mode-644.patch 1970-01-01 01:00:00.0 +0100 +++ sanlock-2.2/debian/patches/0001-wdmd-use-lockfile-mode-644.patch 2013-01-03 22:12:48.0 +0100 @@ -0,0 +1,21 @@ +From 3a2ba2d0fbe78f4eacd438b708ceff6e96903d37 Mon Sep 17 00:00:00 2001 +From: David Teigland +Date: Wed, 1 Aug 2012 17:00:53 -0500 +Subject: [PATCH] wdmd: use lockfile mode 644 + +Signed-off-by: David Teigland +--- + wdmd/main.c |2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +--- a/wdmd/main.c b/wdmd/main.c +@@ -819,7 +819,7 @@ + + sprintf(lockfile_path, "%s/wdmd.pid", WDMD_RUN_DIR); + +- fd = open(lockfile_path, O_CREAT|O_WRONLY|O_CLOEXEC, 0666); ++ fd = open(lockfile_path, O_CREAT|O_WRONLY|O_CLOEXEC, 0644); + if (fd < 0) { + log_error("lockfile open error %s: %s", + lockfile_path, strerror(errno)); diff -Nru sanlock-2.2/debian/patches/series sanlock-2.2/debian/patches/series --- sanlock-
Bug#696424: Possible patch
Hello, attached is a possible patch for that issue. This is just a starting point, as I was not able to test the patch myself. Also, I used 660 as permissions to the file, I'm not sure of whether it's sensible or not. Please review and test before applying. HTH anyway, Mt. -- Nous avons neuf mois de vie privée avant de naître, ça devrait nous suffire. -- Heathcote Williams, Actuel n°48, novembre 74. Initial report (https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2012-5638) | The sanlock server creates the /var/log/sanlock.log world writable | allowing any one on the system to wipe the contents of the log file or | to store data within the log file (bypassing any quotas applied to | their account). The affected code (in src/log.c) is: | | int setup_logging(void) { | int fd, rv; | snprintf(logfile_path, PATH_MAX, "%s/%s", SANLK_LOG_DIR, | SANLK_LOGFILE_NAME); | logfile_fp = fopen(logfile_path, "a+"); This patch was proposed by Martin Quinson, but not really tested as I don't use sanlock myself. Also, I used 660 as permissions to the file, I'm not sure of whether it's sensible or not. Index: sanlock-2.2/src/log.c === --- sanlock-2.2.orig/src/log.c 2012-05-07 17:43:52.0 +0200 +++ sanlock-2.2/src/log.c 2012-12-24 22:19:10.437901274 +0100 @@ -252,10 +252,12 @@ snprintf(logfile_path, PATH_MAX, "%s/%s", SANLK_LOG_DIR, SANLK_LOGFILE_NAME); - logfile_fp = fopen(logfile_path, "a+"); - if (logfile_fp) { - fd = fileno(logfile_fp); + fd = open(logfile_path,O_CREAT | O_WRONLY, S_IRUSR|S_IWUSR | S_IRGRP|S_IWGRP); + if (fd != -1) { fcntl(fd, F_SETFD, fcntl(fd, F_GETFD, 0) | FD_CLOEXEC); + logfile_fp = fdopen(fd, "a+"); + } else { + logfile_fp = NULL; } log_ents = malloc(log_num_ents * sizeof(struct entry));