6:51 a.m.
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
8:25 a.m.
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
11:50 a.m.
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.
1:24 p.m.
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
5 p.m.
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.
2:31 a.m.
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
3:45 a.m.
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.
4:10 a.m.
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(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
6:34 a.m.
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().
7:33 a.m.
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
6:35 a.m.
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.
7:04 a.m.
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.
7:44 a.m.
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
9:06 a.m.
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));
+ }
+ }
+}
+
9:17 a.m.
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
10:01 a.m.
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
10:19 a.m.
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
2982
days inactive
3003
days old
sssd-devel@lists.fedorahosted.org
19 comments
5 participants
participants (5)
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Pavel Reichl -
Sumit Bose