On 01/29/2016 01:41 PM, Lukas Slebodnik wrote:
https://fedorahosted.org/sssd/ticket/2931
src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) goto done; }
- ret = open("/etc/krb5.conf", O_RDONLY);
- if (ret == EOK)
I thought that open() returns file descriptor on success and and -1 in case of error. Was I wrong?
{
close(ret);- } else {
ret = errno;if (ret == EPERM) {DEBUG(SSSDBG_CRIT_FAILURE,"User with uid:%"SPRIuid" gid:%"SPRIgid" cannot read ""/etc/krb5.conf. It might cause problems.",geteuid(), getegid());} else {DEBUG(SSSDBG_MINOR_FAILURE,"Cannot open /etc/krb5.conf [%d]: %s\n",ret, strerror(ret));}- }
DEBUG(SSSDBG_TRACE_INTERNAL, "Running as [%"SPRIuid"][%"SPRIgid"].\n", geteuid(), getegid());-- 2.5.0
On (29/01/16 13:51), Pavel Reichl wrote:
On 01/29/2016 01:41 PM, Lukas Slebodnik wrote:
https://fedorahosted.org/sssd/ticket/2931
src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) goto done; }
- ret = open("/etc/krb5.conf", O_RDONLY);
- if (ret == EOK)
I thought that open() returns file descriptor on success and and -1 in case of error. Was I wrong?
That's a fast codding style change swith->if after testing and before sending patch.
LS
On Fri, Jan 29, 2016 at 03:25:56PM +0100, Lukas Slebodnik wrote:
On (29/01/16 13:51), Pavel Reichl wrote:
On 01/29/2016 01:41 PM, Lukas Slebodnik wrote:
https://fedorahosted.org/sssd/ticket/2931
src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) goto done; }
- ret = open("/etc/krb5.conf", O_RDONLY);
- if (ret == EOK)
I thought that open() returns file descriptor on success and and -1 in case of error. Was I wrong?
That's a fast codding style change swith->if after testing and before sending patch.
It's better to test file access with open if you need to actually use the file to avoid TOCTOU-style races, but here I guess it doesn't matter. Nonetheless, I would prefer to use a variable named 'fd' and not 'ret' and close the fd right away, even though krb5_child is short-lived and would close its descriptors later. I also wonder if we should wrap the code in a short function.
On (29/01/16 18:50), Jakub Hrozek wrote:
On Fri, Jan 29, 2016 at 03:25:56PM +0100, Lukas Slebodnik wrote:
On (29/01/16 13:51), Pavel Reichl wrote:
On 01/29/2016 01:41 PM, Lukas Slebodnik wrote:
https://fedorahosted.org/sssd/ticket/2931
src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) goto done; }
- ret = open("/etc/krb5.conf", O_RDONLY);
- if (ret == EOK)
I thought that open() returns file descriptor on success and and -1 in case of error. Was I wrong?
That's a fast codding style change swith->if after testing and before sending patch.
It's better to test file access with open if you need to actually use the file to avoid TOCTOU-style races, but here I guess it doesn't matter. Nonetheless, I would prefer to use a variable named 'fd' and not 'ret'
I was just lazy to declare another variable
and close the fd right away,
It's already in patch. We can close file descriptor only in case of success,
LS
On Fri, Jan 29, 2016 at 08:24:00PM +0100, Lukas Slebodnik wrote:
On (29/01/16 18:50), Jakub Hrozek wrote:
On Fri, Jan 29, 2016 at 03:25:56PM +0100, Lukas Slebodnik wrote:
On (29/01/16 13:51), Pavel Reichl wrote:
On 01/29/2016 01:41 PM, Lukas Slebodnik wrote:
https://fedorahosted.org/sssd/ticket/2931
src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) goto done; }
- ret = open("/etc/krb5.conf", O_RDONLY);
- if (ret == EOK)
I thought that open() returns file descriptor on success and and -1 in case of error. Was I wrong?
That's a fast codding style change swith->if after testing and before sending patch.
It's better to test file access with open if you need to actually use the file to avoid TOCTOU-style races, but here I guess it doesn't matter. Nonetheless, I would prefer to use a variable named 'fd' and not 'ret'
I was just lazy to declare another variable
If you don't mind, I would prefer fd. When I quickly scan code, 'ret' is something we normally use for return codes. Here it's something that actually represents a resource that needs to be closed later.
and close the fd right away,
It's already in patch.
You're right, sorry, I overlooked that.
On (30/01/16 00:00), Jakub Hrozek wrote:
On Fri, Jan 29, 2016 at 08:24:00PM +0100, Lukas Slebodnik wrote:
On (29/01/16 18:50), Jakub Hrozek wrote:
On Fri, Jan 29, 2016 at 03:25:56PM +0100, Lukas Slebodnik wrote:
On (29/01/16 13:51), Pavel Reichl wrote:
On 01/29/2016 01:41 PM, Lukas Slebodnik wrote:
https://fedorahosted.org/sssd/ticket/2931
src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) goto done; }
- ret = open("/etc/krb5.conf", O_RDONLY);
- if (ret == EOK)
I thought that open() returns file descriptor on success and and -1 in case of error. Was I wrong?
That's a fast codding style change swith->if after testing and before sending patch.
It's better to test file access with open if you need to actually use the file to avoid TOCTOU-style races, but here I guess it doesn't matter. Nonetheless, I would prefer to use a variable named 'fd' and not 'ret'
I was just lazy to declare another variable
If you don't mind, I would prefer fd. When I quickly scan code, 'ret' is something we normally use for return codes. Here it's something that actually represents a resource that needs to be closed later.
and close the fd right away,
It's already in patch.
You're right, sorry, I overlooked that.
Updated patch is attached.
LS
I thought you were going to use 'fd' for return value of open(). I still think access() would be better function to use. We would not need to care about file descriptor at all.
On Mon, Feb 01, 2016 at 10:45:56AM +0100, Pavel Reichl wrote:
I thought you were going to use 'fd' for return value of open(). I still think access() would be better function to use. We would not need to care about file descriptor at all.
It's a bit nit-picking but access() only checks if you are allowed to access the file in the requested way not if you are really able to do it. E.g. although the file-permission allows you to do so the SELinux policy might prevent you from actually open the file.
Additionally from the access(3) man page "Warning: Using these calls to check if a user is authorized to, for example, open a file before actually doing so using open(2) creates a security hole, because the user might exploit the short time interval between checking and opening the file to manipulate it. For this reason, the use of this system call should be avoided. (In the example just described, a safer alternative would be to temporarily switch the process's effective user ID to the real ID and then call open(2).)"
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On 02/01/2016 11:10 AM, Sumit Bose wrote:
On Mon, Feb 01, 2016 at 10:45:56AM +0100, Pavel Reichl wrote:
I thought you were going to use 'fd' for return value of open(). I still think access() would be better function to use. We would not need to care about file descriptor at all.
It's a bit nit-picking but access() only checks if you are allowed to access the file in the requested way not if you are really able to do it. E.g. although the file-permission allows you to do so the SELinux policy might prevent you from actually open the file.
Additionally from the access(3) man page "Warning: Using these calls to check if a user is authorized to, for example, open a file before actually doing so using open(2) creates a security hole, because the user might exploit the short time interval between checking and opening the file to manipulate it. For this reason, the use of this system call should be avoided. (In the example just described, a safer alternative would be to temporarily switch the process's effective user ID to the real ID and then call open(2).)"
I think that this security hole is not relevant for our case, because we open the file to test if we have access and then we close it. File privileges can be changed before krb child actually access the file the very same way as if we tested by access(), right?
Anyway, I see the advantage of selinux policy being checked when the open() is performed so I no longer push for access().
On (01/02/16 09:31), Lukas Slebodnik wrote:
On (30/01/16 00:00), Jakub Hrozek wrote:
On Fri, Jan 29, 2016 at 08:24:00PM +0100, Lukas Slebodnik wrote:
On (29/01/16 18:50), Jakub Hrozek wrote:
On Fri, Jan 29, 2016 at 03:25:56PM +0100, Lukas Slebodnik wrote:
On (29/01/16 13:51), Pavel Reichl wrote:
On 01/29/2016 01:41 PM, Lukas Slebodnik wrote: >https://fedorahosted.org/sssd/ticket/2931 >--- > src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > >diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c >index 12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc 100644 >--- a/src/providers/krb5/krb5_child.c >+++ b/src/providers/krb5/krb5_child.c >@@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) > goto done; > } > >+ ret = open("/etc/krb5.conf", O_RDONLY); >+ if (ret == EOK)
I thought that open() returns file descriptor on success and and -1 in case of error. Was I wrong?
That's a fast codding style change swith->if after testing and before sending patch.
It's better to test file access with open if you need to actually use the file to avoid TOCTOU-style races, but here I guess it doesn't matter. Nonetheless, I would prefer to use a variable named 'fd' and not 'ret'
I was just lazy to declare another variable
If you don't mind, I would prefer fd. When I quickly scan code, 'ret' is something we normally use for return codes. Here it's something that actually represents a resource that needs to be closed later.
and close the fd right away,
It's already in patch.
You're right, sorry, I overlooked that.
Updated patch is attached.
LS
I accidentally sent wrong patch.
Updated patch us variable fd for open and code was moved to separate function.
LS
On 02/01/2016 02:39 PM, Pavel Reichl wrote:
On 02/01/2016 02:33 PM, Lukas Slebodnik wrote:
}
+void try_open_krb5_conf(void) +{
- int fd;
- int ret;
Is there any reason for the function not to be static? _______________________________________________
Bump, lets finish the review.
Just some nitpicks.
On 02/01/2016 02:33 PM, Lukas Slebodnik wrote:
+void try_open_krb5_conf(void) +{
- int fd;
- int ret;
- fd = open("/etc/krb5.conf", O_RDONLY);
- if (fd != -1) {
close(fd);- } else {
ret = errno;if (ret == EPERM) {DEBUG(SSSDBG_CRIT_FAILURE,"User with uid:%"SPRIuid" gid:%"SPRIgid" cannot read ""/etc/krb5.conf. It might cause problems.",
Missing '\n'
geteuid(), getegid());} else {DEBUG(SSSDBG_MINOR_FAILURE,"Cannot open /etc/krb5.conf [%d]: %s\n",ret, strerror(ret));
I see that we already use sss_strerror() in this module, so please use it as well.
}- }
+}
Thanks.
On (01/02/16 14:33), Lukas Slebodnik wrote:
On (01/02/16 09:31), Lukas Slebodnik wrote:
On (30/01/16 00:00), Jakub Hrozek wrote:
On Fri, Jan 29, 2016 at 08:24:00PM +0100, Lukas Slebodnik wrote:
On (29/01/16 18:50), Jakub Hrozek wrote:
On Fri, Jan 29, 2016 at 03:25:56PM +0100, Lukas Slebodnik wrote:
On (29/01/16 13:51), Pavel Reichl wrote: > > >On 01/29/2016 01:41 PM, Lukas Slebodnik wrote: >>https://fedorahosted.org/sssd/ticket/2931 >>--- >> src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >>diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c >>index 12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc 100644 >>--- a/src/providers/krb5/krb5_child.c >>+++ b/src/providers/krb5/krb5_child.c >>@@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) >> goto done; >> } >> >>+ ret = open("/etc/krb5.conf", O_RDONLY); >>+ if (ret == EOK) > >I thought that open() returns file descriptor on success and and -1 in case of error. Was I wrong? > That's a fast codding style change swith->if after testing and before sending patch.
It's better to test file access with open if you need to actually use the file to avoid TOCTOU-style races, but here I guess it doesn't matter. Nonetheless, I would prefer to use a variable named 'fd' and not 'ret'
I was just lazy to declare another variable
If you don't mind, I would prefer fd. When I quickly scan code, 'ret' is something we normally use for return codes. Here it's something that actually represents a resource that needs to be closed later.
and close the fd right away,
It's already in patch.
You're right, sorry, I overlooked that.
Updated patch is attached.
LS
I accidentally sent wrong patch.
Updated patch us variable fd for open and code was moved to separate function.
Updated version is attached.
LS
On 02/19/2016 02:44 PM, Lukas Slebodnik wrote:
+static void try_open_krb5_conf(void) +{
- int fd;
- int ret;
- fd = open("/etc/krb5.conf", O_RDONLY);
- if (fd != -1) {
close(fd);- } else {
ret = errno;if (ret == EPERM) {
^^^^^ Please change to EACCES || EPERM.
Otherwise LGTM.
Michal
DEBUG(SSSDBG_CRIT_FAILURE,"User with uid:%"SPRIuid" gid:%"SPRIgid" cannot read ""/etc/krb5.conf. It might cause problems\n",geteuid(), getegid());} else {DEBUG(SSSDBG_MINOR_FAILURE,"Cannot open /etc/krb5.conf [%d]: %s\n",ret, strerror(ret));}- }
+}
On (19/02/16 16:06), Michal Židek wrote:
On 02/19/2016 02:44 PM, Lukas Slebodnik wrote:
+static void try_open_krb5_conf(void) +{
- int fd;
- int ret;
- fd = open("/etc/krb5.conf", O_RDONLY);
- if (fd != -1) {
close(fd);- } else {
ret = errno;if (ret == EPERM) {^^^^^Please change to EACCES || EPERM.
Nice catch.
Updated patch is attached.
LS
On 02/19/2016 04:17 PM, Lukas Slebodnik wrote:
On (19/02/16 16:06), Michal Židek wrote:
On 02/19/2016 02:44 PM, Lukas Slebodnik wrote:
+static void try_open_krb5_conf(void) +{
- int fd;
- int ret;
- fd = open("/etc/krb5.conf", O_RDONLY);
- if (fd != -1) {
close(fd);- } else {
ret = errno;if (ret == EPERM) {^^^^^Please change to EACCES || EPERM.
Nice catch.
Updated patch is attached.
LS
ACK.
CI link: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/3756/
Michal
On (19/02/16 17:01), Michal Židek wrote:
On 02/19/2016 04:17 PM, Lukas Slebodnik wrote:
On (19/02/16 16:06), Michal Židek wrote:
On 02/19/2016 02:44 PM, Lukas Slebodnik wrote:
+static void try_open_krb5_conf(void) +{
- int fd;
- int ret;
- fd = open("/etc/krb5.conf", O_RDONLY);
- if (fd != -1) {
close(fd);- } else {
ret = errno;if (ret == EPERM) {^^^^^Please change to EACCES || EPERM.
Nice catch.
Updated patch is attached.
LS
ACK.
CI link: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/3756/
master: * 38f251e531b1c68e70eaa98dfecaf78da5f36ccc
sssd-1-13: * 760d655881e87f52db033a4a56b05fbe91dce146
LS
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Pavel Reichl -
Sumit Bose