cache: Check for max_id/min_id in cache_req #449

code-with-amitk · 2017-11-14T05:42:34Z

The cache_req code doesn't check the min_id/max_id
boundaries for requests by ID.
Extending the .lookup_fn function in each plugin
that searches by ID for a check that returns 0
if the entry is out of the range.

Resolves: https://pagure.io/SSSD/sssd/issue/3569

centos-ci · 2017-11-14T05:42:35Z

Can one of the admins verify this patch?

centos-ci · 2017-11-14T05:42:35Z

Can one of the admins verify this patch?

fidencio

@amitkumar50, as usual, thanks a lot for your contribution!

I've asked a few questions inline.

fidencio · 2017-11-14T06:03:40Z

src/db/sysdb_ops.c

@@ -4909,7 +4909,9 @@ errno_t sysdb_search_object_by_id(TALLOC_CTX *mem_ctx,
    if (filter == NULL) {
        return ENOMEM;
    }
-
+    if (id > 20000 || id < 0) {


Why 20000 was chosen? Please, add a new #define with a meaningful name (or check if we already have one and use it) in order to have things more clear.

But why 20000 was chosen? I don't understand where the 20000 comes from.

I've checked the code and seems that the check you'd like to do is based on domain.
So, you'd have to do something like (not tested):

if ((domain->max_id != 0 && id > domain->max_id) || id < domain->min_id ) { ... }

The reason for this check is because:

min_id and max_id may be defined by domain through sssd.conf

defining max_id = 0 means there's no limit for the max_id

Last but not least, I'd expect at least some debug message added warning that min_id/max_id conditions are not match.

Also, I'd like to hear from @pbrezina whether he thinks would be better to do this check at the sysdb level or at the cache_req level.

fidencio · 2017-11-14T06:03:55Z

src/db/sysdb_ops.c

@@ -4909,7 +4909,9 @@ errno_t sysdb_search_object_by_id(TALLOC_CTX *mem_ctx,
    if (filter == NULL) {
        return ENOMEM;
    }
-
+    if (id > 20000 || id < 0) {
+        return 0;


return EOK instead of 0.

As per https://pagure.io/SSSD/sssd/issue/3569
for a check that returns EOK but a NULL result if the entry is out of the range.

This is Error case. I believe returning EOK would not be good?
Also typedef int errno_t; So returning NULL would cause warning.
Again EOK is 0 and NULL is also 0. I believe we should only return 0 value on error path, since on good path we should allow function to go a head.

EOK is 0. But it's way clear in the code if I read EOK instead of 0 as I know that EOK is OK and zero may mean anything else.

fidencio · 2017-11-14T06:04:24Z

src/db/sysdb_search.c

@@ -364,7 +364,7 @@ errno_t sysdb_getpwuid_with_views(TALLOC_CTX *mem_ctx,
                                  uid_t uid,
                                  struct ldb_result **res)
 {
-    int ret;
+    int ret = 0;


Why have you decided to set ret to zero here?

At end of function, inside done label, If we not setting any value of ret, It would throw uninitialized warning.

So, please, set the ret to EOK in the appropriate place (just before calling "goto done")

fidencio · 2017-11-14T06:04:49Z

src/db/sysdb_search.c

@@ -374,7 +374,9 @@ errno_t sysdb_getpwuid_with_views(TALLOC_CTX *mem_ctx,
        DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
        return ENOMEM;
    }
-
+    if (uid > 20000 || uid < 0) {


Why 20000 was chosen? Please, add a new #define with a meaningful name (or check if we already have one and use it) in order to have things more clear.

fidencio · 2017-11-14T06:05:05Z

src/db/sysdb_search.c

@@ -949,7 +951,7 @@ int sysdb_getgrgid_with_views(TALLOC_CTX *mem_ctx,
                              struct ldb_result **res)
 {
    TALLOC_CTX *tmp_ctx;
-    int ret;
+    int ret = 0;


Why have you decided to set ret to zero here?

fidencio · 2017-11-14T06:05:11Z

src/db/sysdb_search.c

@@ -958,7 +960,9 @@ int sysdb_getgrgid_with_views(TALLOC_CTX *mem_ctx,
    if (!tmp_ctx) {
        return ENOMEM;
    }
-
+    if (gid > 20000 || gid < 0) {


Why 20000 was chosen? Please, add a new #define with a meaningful name (or check if we already have one and use it) in order to have things more clear.

fidencio · 2017-11-14T06:14:51Z

ok to test

jhrozek · 2017-11-14T10:21:24Z

I'm sorry, but this patch is incorrect.

As I wrote in the ticket, you want to check the input id (data->id inside cache_req) against the domain min/max limits inside the lookup_fn function of plugins that search by ID.

My initial idea was to return a special error code (ERR_UID_OUTSIDE_RANGE?) so that the lookup wouldn't even continue. But this would only handle requests where we actually search the cache.

Alternatively, we could add another plugin method. Not sure which way is more preferred, what do you think, @pbrezina

pbrezina · 2017-11-14T10:24:44Z

@jhrozek At this point, I think it will be better to wait for your cache req patches to come in. I think this check can be done during the phase where you check if the id is part of the domain. If it is outside the domain range, we can say it is not part of this domain.

code-with-amitk

Wrongly commited on this branch.

jhrozek · 2017-11-15T11:45:33Z

@pbrezina I'm still running some tests with my patches, but in general they are ready and published in my gc_uid branch.

btw see the downtream report - we were already asked for a backport to sssd-1-14, so while in general I'm OK with basing the patches to master atop mine (less work for me I guess :-)) we will also need patches for sssd-1-14.

code-with-amitk · 2017-11-16T12:00:30Z

@jhrozek Thanks for comments.
So Shall I not work on this patch as of now?
Or instead of EOK define ERR_UID_OUTSIDE_RANGE in ./src/util/util_errors.h and return.

jhrozek · 2017-11-27T15:40:15Z

src/db/sysdb_ops.c

@@ -4909,7 +4909,6 @@ errno_t sysdb_search_object_by_id(TALLOC_CTX *mem_ctx,
    if (filter == NULL) {
        return ENOMEM;
    }
-


This line removal does not belong to this patch, can you remove this hunk from the patch?

Sorry dint know how these creapt in. On my local checkout on branch I cannot see these.

Still present in the current series.

jhrozek · 2017-11-27T15:40:25Z

src/db/sysdb_search.c

@@ -374,7 +374,6 @@ errno_t sysdb_getpwuid_with_views(TALLOC_CTX *mem_ctx,
        DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
        return ENOMEM;
    }
-


Also please this line

jhrozek · 2017-11-27T15:42:34Z

src/responder/common/cache_req/plugins/cache_req_group_by_id.c

+   if ((domain->id_max != 0 && data->id > domain->id_max)
+        ||  (data->id < domain->id_min)) {
+        DEBUG(SSSDBG_OP_FAILURE, "gid exceeds min/max boundaries\n");
+        return EOK;


Yes, you need to return some special error code here. Additionally, please check both id_max and id_min for being non-zero, like this:

if ((domain->id_min && (data->id < domain->id_min)) || (domain->id_max && (data->id > domain->id_max))) {

Also, please make the debug message level FUNC_DATA or something as low as that so that the debug message doesn't appear during normal operation

Finally, please add a case handler into the switch statement in cache_req_search_cache() so that we only print a very low-level debug message and don't hit the "default" handler.

jhrozek · 2017-11-27T15:44:33Z

src/responder/common/cache_req/plugins/cache_req_object_by_id.c

+    if ((domain->id_max != 0 && data->id > domain->id_max)
+        || (data->id < domain->id_min)) {
+        DEBUG(SSSDBG_OP_FAILURE, "id exceeded min/max boundaries\n");
+        return EOK;


Same comments as in the group lookup. I wonder if it makes sense to add a common function to avoid duplicating the same lines?

jhrozek · 2017-11-27T15:44:49Z

src/responder/common/cache_req/plugins/cache_req_user_by_id.c

+    if ((domain->id_max != 0 && data->id > domain->id_max)
+        ||  (data->id < domain->id_min)) {
+        DEBUG(SSSDBG_OP_FAILURE, "uid exceeds min/max boundaries\n");
+        return EOK;


Same comment as in the object and group lookup.

jhrozek

@amitkumar50 please see the review comments

jhrozek · 2017-11-29T15:12:17Z

Removing changes requested since a new patch had arrived

fidencio

I have made some comments inline. Adding the "Changes Requested" label but still would like hear from @jhrozek whether the approach of the contributor is okay for him.

fidencio · 2017-11-29T15:39:44Z

src/db/sysdb_ops.c

@@ -4909,7 +4909,6 @@ errno_t sysdb_search_object_by_id(TALLOC_CTX *mem_ctx,
    if (filter == NULL) {
        return ENOMEM;
    }
-


Still present in the current series.

fidencio · 2017-11-29T15:39:52Z

src/db/sysdb_search.c

@@ -374,7 +374,6 @@ errno_t sysdb_getpwuid_with_views(TALLOC_CTX *mem_ctx,
        DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
        return ENOMEM;
    }
-


fidencio · 2017-11-29T15:41:14Z

src/responder/common/cache_req/plugins/cache_req_common.c

+        DEBUG(SSSDBG_FUNC_DATA, "gid exceeds min/max boundaries\n");
+        return ERR_UID_OUTSIDE_RANGE;
+    }
+}


This function as it is will generate a warning due to the missing return for a non-void function. IOW, you need to return EOK in case of success.

fidencio · 2017-11-29T15:42:00Z

src/responder/common/cache_req/plugins/cache_req_group_by_id.c

@@ -64,6 +64,7 @@ cache_req_group_by_id_lookup(TALLOC_CTX *mem_ctx,
                             struct sss_domain_info *domain,
                             struct ldb_result **_result)
 {
+    cache_req_idminmax_check(data, domain);


My understanding is that you want to check the return of this function and then decide what to do based on that.

fidencio · 2017-11-29T15:42:06Z

src/responder/common/cache_req/plugins/cache_req_object_by_id.c

@@ -90,6 +90,7 @@ cache_req_object_by_id_lookup(TALLOC_CTX *mem_ctx,
                              struct sss_domain_info *domain,
                              struct ldb_result **_result)
 {
+    cache_req_idminmax_check(data, domain);


fidencio · 2017-11-29T15:42:14Z

src/responder/common/cache_req/plugins/cache_req_user_by_id.c

@@ -64,6 +64,7 @@ cache_req_user_by_id_lookup(TALLOC_CTX *mem_ctx,
                            struct sss_domain_info *domain,
                            struct ldb_result **_result)
 {
+    cache_req_idminmax_check(data, domain);


fidencio · 2017-11-29T15:43:17Z

src/util/util_errors.h

@@ -137,6 +137,7 @@ enum sssd_errors {
    ERR_SSL_FAILURE,
    ERR_UNABLE_TO_VERIFY_PEER,
    ERR_UNABLE_TO_RESOLVE_HOST,
+    ERR_UID_OUTSIDE_RANGE,


As you added a new error in util_errors.h, please, also to add a new string for this error on util_errors.c

The cache_req code doesn't check the min_id/max_id boundaries for requests by ID. Extending the .lookup_fn function in each plugin that searches by ID for a check that returns 0 if the entry is out of the range. Resolves: https://pagure.io/SSSD/sssd/issue/3569

fidencio reviewed Nov 14, 2017

View reviewed changes

jhrozek added the Changes requested label Nov 14, 2017

code-with-amitk commented Nov 14, 2017

View reviewed changes

code-with-amitk force-pushed the branch-3569 branch from 117fbb6 to 630b9fc Compare November 16, 2017 05:14

jhrozek reviewed Nov 27, 2017

View reviewed changes

code-with-amitk force-pushed the branch-3569 branch from 630b9fc to d9ec950 Compare November 29, 2017 11:36

jhrozek removed the Changes requested label Nov 29, 2017

fidencio reviewed Nov 29, 2017

View reviewed changes

fidencio added the Changes requested label Nov 29, 2017

code-with-amitk force-pushed the branch-3569 branch from d9ec950 to 92db86c Compare November 30, 2017 10:54

code-with-amitk mentioned this pull request Nov 30, 2017

cache: Check for max_id/min_id in cache_req #463

Closed

code-with-amitk closed this Nov 30, 2017

pbrezina mentioned this pull request Apr 28, 2020

The cache_req code doesn't check the min_id/max_id boundaries for requests by ID pbrezina/sssd-test#3555

Closed

sssd-bot mentioned this pull request May 2, 2020

The cache_req code doesn't check the min_id/max_id boundaries for requests by ID #4593

Closed

cache: Check for max_id/min_id in cache_req #449

cache: Check for max_id/min_id in cache_req #449

Conversation

code-with-amitk commented Nov 14, 2017

centos-ci commented Nov 14, 2017

centos-ci commented Nov 14, 2017

fidencio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fidencio commented Nov 14, 2017

jhrozek commented Nov 14, 2017

pbrezina commented Nov 14, 2017

code-with-amitk left a comment

Choose a reason for hiding this comment

jhrozek commented Nov 15, 2017

code-with-amitk commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrozek left a comment

Choose a reason for hiding this comment

jhrozek commented Nov 29, 2017

fidencio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment