Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(RHEL-55266) Return ESRCH if userdb service refuses a user/group name as invalid #302

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

(RHEL-55266) Return ESRCH if userdb service refuses a user/group name as invalid #302

wants to merge 5 commits into from

Conversation

dtardon
Copy link
Member

@dtardon dtardon commented Oct 24, 2024 •
edited by github-actions bot
Loading

Resolves: RHEL-55266

@github-actions github-actions bot added tracker/missing Formerly needs-bz pr/needs-ci Formerly needs-ci pr/needs-review Formerly needs-review labels Oct 24, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 24, 2024 •
edited
Loading

Commit validation

Tracker - RHEL-55266

The following commits meet all requirements

commit upstream
940c99b - nspawn: call json_dispatch() with a correct pointer systemd/systemd@f4e5c04
b17e203 - varlink,json: introduce new varlink_dispatch() helper systemd/systemd@f1b622a
57df958 - json: add json_dispatch_const_user_group_name() systemd/systemd@0376ef3
d892604 - sd-varlink: add new sd_varlink_error_is_invalid_parameter() helper systemd/systemd@12641ec
575675c - userdb: return ESRCH if userdb service refuses a user/group name as in… systemd/systemd@69cc4ee

Tracker validation

Success

🟢 Tracker RHEL-55266 has set desired product: rhel-9.6
🟢 Tracker RHEL-55266 has set desired component: systemd
🟢 Tracker RHEL-55266 has been approved
🟢 Tracker RHEL-55266 has set severity

Pull Request validation

Failed

🔴 Failed or pending checks - CodeQL[neutral],build (GCC, openssl)[failure],build (GCC, auto)[failure],build (GCC_ASAN_UBSAN, auto)[failure],build (CLANG_RELEASE, auto)[failure],build (CLANG, gcrypt)[failure],build (gcc, 12, gold, openssl)[failure],build (CLANG, auto)[failure],build (CLANG_ASAN_UBSAN_NO_DEPS, auto)[failure],build (gcc, 11, bfd, gcrypt)[failure],build (CLANG_ASAN_UBSAN, auto)[failure],build (clang, 15, bfd, auto)[failure],ci (centos, 9)[failure],build (clang, 14, lld, openssl)[failure],build (clang, 13, mold, gcrypt)[failure],Analyze (cpp)[failure]
🔴 Approval - missing or changes were requested

Success

🟢 Review - Reviewed by a member

@jamacku
Copy link
Member

jamacku commented Oct 25, 2024

@dtardon I have noticed that this PR references two issues. Could you please split it into two PRs? Thank you.

mrc0mmand and others added 5 commits October 29, 2024 08:36
@mrc0mmand @dtardon
nspawn: call json_dispatch() with a correct pointer
940c99b 
Otherwise hilarity ensues:

 AddressSanitizer:DEADLYSIGNAL
 =================================================================
 ==722==ERROR: AddressSanitizer: SEGV on unknown address 0xffffffff00000000 (pc 0x7f8d50ca9ffb bp 0x7fff11b0d4a0 sp 0x7fff11b0cc30 T0)
 ==722==The signal is caused by a READ memory access.
     #0 0x7f8d50ca9ffb in __interceptor_strcmp.part.0 (/lib64/libasan.so.8+0xa9ffb)
     redhat-plumbers#1 0x7f8d4f9cf5a1 in strcmp_ptr ../src/fundamental/string-util-fundamental.h:33
     redhat-plumbers#2 0x7f8d4f9cf5f8 in streq_ptr ../src/fundamental/string-util-fundamental.h:46
     redhat-plumbers#3 0x7f8d4f9d74d2 in free_and_strdup ../src/basic/string-util.c:948
     redhat-plumbers#4 0x49139a in free_and_strdup_warn ../src/basic/string-util.h:197
     redhat-plumbers#5 0x4923eb in oci_absolute_path ../src/nspawn/nspawn-oci.c:139
     redhat-plumbers#6 0x7f8d4f6bd359 in json_dispatch ../src/shared/json.c:4395
     redhat-plumbers#7 0x4a8831 in oci_hooks_array ../src/nspawn/nspawn-oci.c:2089
     redhat-plumbers#8 0x7f8d4f6bd359 in json_dispatch ../src/shared/json.c:4395
     redhat-plumbers#9 0x4a8b56 in oci_hooks ../src/nspawn/nspawn-oci.c:2112
     redhat-plumbers#10 0x7f8d4f6bd359 in json_dispatch ../src/shared/json.c:4395
     redhat-plumbers#11 0x4aa298 in oci_load ../src/nspawn/nspawn-oci.c:2197
     redhat-plumbers#12 0x446cec in load_oci_bundle ../src/nspawn/nspawn.c:4744
     redhat-plumbers#13 0x44ffa7 in run ../src/nspawn/nspawn.c:5477
     redhat-plumbers#14 0x4552fb in main ../src/nspawn/nspawn.c:5920
     redhat-plumbers#15 0x7f8d4e04a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
     redhat-plumbers#16 0x7f8d4e04a5c8 in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x275c8)
     redhat-plumbers#17 0x40d284 in _start (/usr/bin/systemd-nspawn+0x40d284)
 AddressSanitizer can not provide additional info.
 SUMMARY: AddressSanitizer: SEGV (/lib64/libasan.so.8+0xa9ffb) in __interceptor_strcmp.part.0
 ==722==ABORTING

(cherry picked from commit f4e5c04)

Related: RHEL-55266
@poettering @dtardon
varlink,json: introduce new varlink_dispatch() helper
b17e203 
varlink_dispatch() is a simple wrapper around json_dispatch() that
returns clean, standards-compliant InvalidParameter error back to
clients, if the specified JSON cannot be parsed properly.

For this json_dispatch() is extended to return the offending field's
name. Because it already has quite a few parameters, I then renamed
json_dispatch() to json_dispatch_full() and made json_dispatch() a
wrapper around it that passes the new argument as NULL. While doing so I
figured we should also get rid of the bad= argument in the short
wrapper, since it's only used in the OCI code.

To simplify the OCI code this adds a second wrapper oci_dispatch()
around json_dispatch_full(), that fills in bad= the way we want.

Net result: instead of one json_dispatch() call there are now:

1. json_dispatch_full() for the fully feature mother of all dispathers.
2. json_dispatch() for the simpler version that you want to use most of
   the time.
3. varlink_dispatch() that generates nice Varlink errors
4. oci_dispatch() that does the OCI specific error handling

And that's all there is.

(cherry picked from commit f1b622a)

Related: RHEL-55266
@poettering @dtardon
json: add json_dispatch_const_user_group_name()
57df958 
This is the same as json_dispatch_user_group_name() but fills in the
string as "const char*" to the JSON field. Or in other words, it's what
sd_json_dispatch_const_string() is to sd_json_dispatch_string().

Note this drops the SD_JSON_STRICT flags from various dispatch tables
for these fields, and replaces this by SD_JSON_RELAX, i.e. the opposite
behaviour. As #34558 correctly suggests we should validate user names
in lookup functions using the lax rules, rather than the strict ones,
since clients not knowing the rules might ask us for arbitrary
resolution.

(SD_JSON_RELAX internally translates to valid_user_group_name() with the
VALID_USER_RELAX flag).

See: #34558
(cherry picked from commit 0376ef36a1ff3768ad0c833f215064e34b40b86c)

Related: RHEL-55266
@poettering @dtardon
sd-varlink: add new sd_varlink_error_is_invalid_parameter() helper
d892604 
(cherry picked from commit 12641ecd67875b7bf18db06c0afa40c37d804750)

Related: RHEL-55266
@poettering @dtardon
userdb: return ESRCH if userdb service refuses a user/group name as i…
575675c 
…nvalid

if a userdb service refuse a user/group name as invalid, let's turn this
into ESRCH client-side following that there definitely is no user/group
record for a completely invalid user/group name.

Replaces: #34558
(cherry picked from commit 69cc4ee134f420dcdd6aac08446bd852d8739694)

Resolves: RHEL-55266
@dtardon
Copy link
Member Author

dtardon commented Oct 29, 2024

@dtardon I have noticed that this PR references two issues. Could you please split it into two PRs? Thank you.

The references to RHEL-55301 are a mistake. Fixed.

@dtardon dtardon force-pushed the RHEL-55266-getgrnam_r branch from f79303d to 575675c Compare October 29, 2024 07:38
@github-actions github-actions bot removed the tracker/missing Formerly needs-bz label Oct 29, 2024
@github-actions github-actions bot changed the title Return ESRCH if userdb service refuses a user/group name as invalid (RHEL-55266) Return ESRCH if userdb service refuses a user/group name as invalid Oct 29, 2024
@github-actions github-actions bot added tracker/unapproved Formerly needs-acks tracker/missing Formerly needs-bz and removed tracker/missing Formerly needs-bz labels Oct 29, 2024
@github-actions github-actions bot added tracker/missing Formerly needs-bz and removed tracker/missing Formerly needs-bz labels Nov 9, 2024
@jamacku jamacku added this to the RHEL-9.6.0 milestone Jan 3, 2025
@github-actions github-actions bot removed the tracker/unapproved Formerly needs-acks label Jan 3, 2025
msekletar
msekletar requested changes Jan 13, 2025
View reviewed changes
Copy link
Member

@msekletar msekletar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per CI results it seems that backport doesn't compile due to issues with json-util.h include. @dtardon can you please double check.

@github-actions github-actions bot added pr/changes-requested and removed pr/needs-review Formerly needs-review labels Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msekletar msekletar msekletar requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
pr/changes-requested pr/needs-ci Formerly needs-ci
Projects
None yet
Milestone
RHEL-9.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@dtardon @jamacku @msekletar @mrc0mmand @poettering