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-dist-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 21;
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 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-dist-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-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
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 21; 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 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 21; 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
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 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 21; then # fix permissions fi [...] and the same for /var/run/sanlock/sanlock.sock. Regards, Salvatore signature.asc Description: Digital signature
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-dist-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 car...@debian.org 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 teigl...@redhat.com
+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 teigl...@redhat.com
+---
+ 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 teigl...@redhat.com
+Date: Thu, 2 Aug 2012 11:27:54 -0500
+Subject: [PATCH] sanlock: use lockfile mode 644
+
+Signed-off-by: David Teigland teigl...@redhat.com
+---
+ 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 teigl...@redhat.com
+Date: Wed, 1 Aug 2012 17:00:53 -0500
+Subject: [PATCH] wdmd: use lockfile mode 644
+
+Signed-off-by: David Teigland teigl...@redhat.com
+---
+ 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,
+
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));
