SSS_CLIENT: MC: in case mem-cache file validation fails, #7085

alexey-tikhonov · 2023-12-08T18:04:38Z

don't return anything but EINVAL, because _nss_sss_*() functions can have a special handling for other error codes (for ERANGE in particular).

thalman

Make sense to me. ACK

alexey-tikhonov · 2023-12-15T16:03:44Z

I added one more patch.
Practically, it seems I'm force to change my stance wrt #6987 because of
https://github.com/TigerVNC/tigervnc/blob/effd854bfd19654fa67ff3d39514a91a246b8ae6/unix/xserver/hw/vnc/xvnc.c#L369

alexey-tikhonov · 2023-12-15T16:07:57Z

We probably should also:

use something like

sssd/src/sss_client/common.c

Line 514 in a5270f8

static int make_nonstd_fd_internals(int fd, int limit)

for mem-cache fd
improve make_nonstd_fd_internals() to dup to a fd close to RLIMIT_NOFILE (instead of close to 3)

src/sss_client/nss_mc_common.c

don't return anything but EINVAL, because `_nss_sss_*()` functions can have a special handling for other error codes (for ERANGE in particular).

aplopez

LGTM

aplopez · 2023-12-20T10:50:30Z

src/sss_client/common.c

@@ -767,6 +767,15 @@ static enum sss_status sss_cli_check_socket(int *errnop,
        myself_ino = myself_sb.st_ino;
    }

+    /* check if the socket has beed was hijacked */


... has been was ...

No need to fix this if this is the only change you have to do.

halfline · 2023-12-20T11:32:10Z

src/sss_client/common.c

+    if (sss_cli_sd_get() != -1) {
+        ret = fstat(sss_cli_sd_get(), &mypid_sb);
+        if ((ret != 0) || (!S_ISSOCK(mypid_sb.st_mode))
+            || (mypid_sb.st_ino != sss_cli_sb->st_ino)) {


since inodes are local to a given device, when comparing inodes between two files its usually a good idea to compare st_dev too

Fixed here and in sss_nss_mc_validate()

Real life example would be: https://github.com/TigerVNC/tigervnc/blob/effd854bfd19654fa67ff3d39514a91a246b8ae6/unix/xserver/hw/vnc/xvnc.c#L369 - TigerVNC unconditionally overwrites fd=3 Resolves: SSSD#6986

Real life example would be: https://github.com/TigerVNC/tigervnc/blob/effd854bfd19654fa67ff3d39514a91a246b8ae6/unix/xserver/hw/vnc/xvnc.c#L369 - TigerVNC unconditionally overwrites fd=3

thalman

ACK for the update

sumit-bose

Hi,

thanks for adding the device check as well, ACK.

About using high fd numbers, maybe this can be used if a changes was detected and we have to re-open the file or socket again. This way we can avoid the related overhead for the well-behaving cases.

While speaking about overheads, maybe we should think about really starting to do some measurements if closing the socket/file at the end of a requeist is more expensive than all those checks.

bye,
Sumit

alexey-tikhonov · 2023-12-21T10:49:59Z

While speaking about overheads, maybe we should think about really starting to do some measurements if closing the socket/file at the end of a requeist is more expensive than all those checks.

I tired this in #6746

It doesn't work for enumeration requests.

alexey-tikhonov · 2023-12-21T12:51:35Z

Pushed PR: #7085

master
- 2bcfb7f - SSS_CLIENT: check if reponder socket was hijacked
- 0344c41 - SSS_CLIENT: check if mem-cache fd was hijacked
- 958a5e2 - SSS_CLIENT: MC: in case mem-cache file validation fails,
sssd-2-9
- abb146e - SSS_CLIENT: check if reponder socket was hijacked
- a186224 - SSS_CLIENT: check if mem-cache fd was hijacked
- 160738e - SSS_CLIENT: MC: in case mem-cache file validation fails,

sumit-bose · 2023-12-21T15:20:31Z

While speaking about overheads, maybe we should think about really starting to do some measurements if closing the socket/file at the end of a requeist is more expensive than all those checks.

I tired this in #6746

It doesn't work for enumeration requests.

Hi,

yes, for enumaration requests we have to find a different way of keeping the state, which is currently tight to the client connection.

bye,
Sumit

alexey-tikhonov added Bugzilla backport-to-stable labels Dec 8, 2023

alexey-tikhonov marked this pull request as ready for review December 8, 2023 19:24

alexey-tikhonov added the Waiting for review label Dec 8, 2023

alexey-tikhonov requested a review from sumit-bose December 12, 2023 09:11

andreboscatto assigned sumit-bose and unassigned sumit-bose Dec 12, 2023

andreboscatto requested review from thalman and removed request for sumit-bose December 12, 2023 13:42

andreboscatto assigned thalman Dec 12, 2023

andreboscatto requested a review from sumit-bose December 12, 2023 13:45

andreboscatto assigned sumit-bose Dec 12, 2023

thalman approved these changes Dec 13, 2023

View reviewed changes

alexey-tikhonov force-pushed the nss-mc branch from 7a7b8b7 to 75505c1 Compare December 15, 2023 14:01

alexey-tikhonov requested a review from thalman December 15, 2023 16:03

halfline reviewed Dec 18, 2023

View reviewed changes

src/sss_client/nss_mc_common.c Outdated Show resolved Hide resolved

alexey-tikhonov force-pushed the nss-mc branch from dad956e to 3c3df0b Compare December 18, 2023 14:47

SSS_CLIENT: MC: in case mem-cache file validation fails,

c0258c3

don't return anything but EINVAL, because `_nss_sss_*()` functions can have a special handling for other error codes (for ERANGE in particular).

alexey-tikhonov force-pushed the nss-mc branch from 3c3df0b to b9f8f1c Compare December 20, 2023 08:44

aplopez approved these changes Dec 20, 2023

View reviewed changes

alexey-tikhonov assigned aplopez Dec 20, 2023

alexey-tikhonov mentioned this pull request Dec 20, 2023

Don't use ' dup2(0, 3)' unconditionally TigerVNC/tigervnc#1709

Closed

halfline reviewed Dec 20, 2023

View reviewed changes

alexey-tikhonov force-pushed the nss-mc branch from b9f8f1c to a285b62 Compare December 20, 2023 12:14

alexey-tikhonov added 2 commits December 20, 2023 13:44

SSS_CLIENT: check if mem-cache fd was hijacked

8367658

Real life example would be: https://github.com/TigerVNC/tigervnc/blob/effd854bfd19654fa67ff3d39514a91a246b8ae6/unix/xserver/hw/vnc/xvnc.c#L369 - TigerVNC unconditionally overwrites fd=3 Resolves: SSSD#6986

SSS_CLIENT: check if reponder socket was hijacked

1bb1a09

Real life example would be: https://github.com/TigerVNC/tigervnc/blob/effd854bfd19654fa67ff3d39514a91a246b8ae6/unix/xserver/hw/vnc/xvnc.c#L369 - TigerVNC unconditionally overwrites fd=3

alexey-tikhonov force-pushed the nss-mc branch from a285b62 to 1bb1a09 Compare December 20, 2023 12:44

thalman approved these changes Dec 20, 2023

View reviewed changes

sumit-bose approved these changes Dec 21, 2023

View reviewed changes

alexey-tikhonov added Accepted Ready to push Ready to push and removed Waiting for review labels Dec 21, 2023

alexey-tikhonov added Pushed and removed Accepted Ready to push Ready to push labels Dec 21, 2023

alexey-tikhonov closed this Dec 21, 2023

alexey-tikhonov mentioned this pull request Dec 21, 2023

The sss_nss_mc_destroy_ctx() function will close the TCP socket of the daemon process #6986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSS_CLIENT: MC: in case mem-cache file validation fails, #7085

SSS_CLIENT: MC: in case mem-cache file validation fails, #7085

alexey-tikhonov commented Dec 8, 2023

thalman left a comment

alexey-tikhonov commented Dec 15, 2023

alexey-tikhonov commented Dec 15, 2023 •

edited

Loading

aplopez left a comment

aplopez Dec 20, 2023

alexey-tikhonov Dec 20, 2023

halfline Dec 20, 2023

sumit-bose Dec 20, 2023

alexey-tikhonov Dec 20, 2023

thalman left a comment

sumit-bose left a comment

alexey-tikhonov commented Dec 21, 2023 •

edited

Loading

alexey-tikhonov commented Dec 21, 2023

sumit-bose commented Dec 21, 2023

SSS_CLIENT: MC: in case mem-cache file validation fails, #7085

SSS_CLIENT: MC: in case mem-cache file validation fails, #7085

Conversation

alexey-tikhonov commented Dec 8, 2023

thalman left a comment

Choose a reason for hiding this comment

alexey-tikhonov commented Dec 15, 2023

alexey-tikhonov commented Dec 15, 2023 • edited Loading

aplopez left a comment

Choose a reason for hiding this comment

aplopez Dec 20, 2023

Choose a reason for hiding this comment

alexey-tikhonov Dec 20, 2023

Choose a reason for hiding this comment

halfline Dec 20, 2023

Choose a reason for hiding this comment

sumit-bose Dec 20, 2023

Choose a reason for hiding this comment

alexey-tikhonov Dec 20, 2023

Choose a reason for hiding this comment

thalman left a comment

Choose a reason for hiding this comment

sumit-bose left a comment

Choose a reason for hiding this comment

alexey-tikhonov commented Dec 21, 2023 • edited Loading

alexey-tikhonov commented Dec 21, 2023

sumit-bose commented Dec 21, 2023

alexey-tikhonov commented Dec 15, 2023 •

edited

Loading

alexey-tikhonov commented Dec 21, 2023 •

edited

Loading