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.

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

KRB5: Fix access_provider=krb5 #294

Closed
wants to merge 1 commit into from
Closed

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Jun 1, 2017

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

The domain type (posix or not) was being sent to the krb5_child always, but
the buffer only had enough space in case of authentication, not
authorization.

This patch makes the buffer one uint32_t unit larger.

To reproduce, just set up sssd.conf with:

   access_provider = krb5

Without the patch, you would see messages like:

   ==14111== Invalid write of size 2
   ==14111==    at 0x4C3041B: memcpy@@GLIBC_2.14 (vg_replace_strmem.c:1018)
   ==14111==    by 0xE0EE275: safealign_memcpy (util_safealign.h:51)
   ==14111==    by 0xE0EECB3: create_send_buffer (krb5_child_handler.c:239)
   ==14111==    by 0xE0EFDDE: handle_child_send (krb5_child_handler.c:529)
   ==14111==    by 0xE0EDEDD: krb5_access_send (krb5_access.c:149)
   ==14111==    by 0xE0ED32F: krb5_pam_handler_send (krb5_auth.c:1250)
   ==14111==    by 0x418868: file_dp_request (dp_request.c:254)
   ==14111==    by 0x418976: dp_req_send (dp_request.c:300)
   ==14111==    by 0x41C25F: dp_pam_handler (dp_target_auth.c:219)
   ==14111==    by 0x52B3456: sbus_request_invoke_or_finish
(sssd_dbus_request.c:71)
   ==14111==    by 0x52B0F37: sbus_message_handler_got_caller_id
(sssd_dbus_interface.c:1048)
   ==14111==    by 0x923C923: tevent_common_loop_immediate
(tevent_immediate.c:135)
   ==14111==  Address 0x126ab506 is 150 bytes inside a block of size 151
alloc'd
   ==14111==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
   ==14111==    by 0x944D7F4: __talloc_with_prefix (talloc.c:698)
   ==14111==    by 0x944D7F4: __talloc (talloc.c:739)
   ==14111==    by 0x944D7F4: _talloc_named_const (talloc.c:896)
   ==14111==    by 0x944D7F4: talloc_named_const (talloc.c:1675)
   ==14111==    by 0xE0EE7B6: create_send_buffer (krb5_child_handler.c:185)
   ==14111==    by 0xE0EFDDE: handle_child_send (krb5_child_handler.c:529)
   ==14111==    by 0xE0EDEDD: krb5_access_send (krb5_access.c:149)
   ==14111==    by 0xE0ED32F: krb5_pam_handler_send (krb5_auth.c:1250)
   ==14111==    by 0x418868: file_dp_request (dp_request.c:254)
   ==14111==    by 0x418976: dp_req_send (dp_request.c:300)
   ==14111==    by 0x41C25F: dp_pam_handler (dp_target_auth.c:219)
   ==14111==    by 0x52B3456: sbus_request_invoke_or_finish
(sssd_dbus_request.c:71)
   ==14111==    by 0x52B0F37: sbus_message_handler_got_caller_id
(sssd_dbus_interface.c:1048)
   ==14111==    by 0x923C923: tevent_common_loop_immediate
(tevent_immediate.c:135)

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

The domain type (posix or not) was being sent to the krb5_child always,
but the buffer only had enough space in case of authentication, not
authorization.

This patch makes the buffer one uint32_t unit larger.

To reproduce, just set up sssd.conf with:
    access_provider = krb5

Without the patch, you would see messages like:
    ==14111== Invalid write of size 2
    ==14111==    at 0x4C3041B: memcpy@@GLIBC_2.14 (vg_replace_strmem.c:1018)
    ==14111==    by 0xE0EE275: safealign_memcpy (util_safealign.h:51)
    ==14111==    by 0xE0EECB3: create_send_buffer (krb5_child_handler.c:239)
    ==14111==    by 0xE0EFDDE: handle_child_send (krb5_child_handler.c:529)
    ==14111==    by 0xE0EDEDD: krb5_access_send (krb5_access.c:149)
    ==14111==    by 0xE0ED32F: krb5_pam_handler_send (krb5_auth.c:1250)
    ==14111==    by 0x418868: file_dp_request (dp_request.c:254)
    ==14111==    by 0x418976: dp_req_send (dp_request.c:300)
    ==14111==    by 0x41C25F: dp_pam_handler (dp_target_auth.c:219)
    ==14111==    by 0x52B3456: sbus_request_invoke_or_finish (sssd_dbus_request.c:71)
    ==14111==    by 0x52B0F37: sbus_message_handler_got_caller_id (sssd_dbus_interface.c:1048)
    ==14111==    by 0x923C923: tevent_common_loop_immediate (tevent_immediate.c:135)
    ==14111==  Address 0x126ab506 is 150 bytes inside a block of size 151 alloc'd
    ==14111==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
    ==14111==    by 0x944D7F4: __talloc_with_prefix (talloc.c:698)
    ==14111==    by 0x944D7F4: __talloc (talloc.c:739)
    ==14111==    by 0x944D7F4: _talloc_named_const (talloc.c:896)
    ==14111==    by 0x944D7F4: talloc_named_const (talloc.c:1675)
    ==14111==    by 0xE0EE7B6: create_send_buffer (krb5_child_handler.c:185)
    ==14111==    by 0xE0EFDDE: handle_child_send (krb5_child_handler.c:529)
    ==14111==    by 0xE0EDEDD: krb5_access_send (krb5_access.c:149)
    ==14111==    by 0xE0ED32F: krb5_pam_handler_send (krb5_auth.c:1250)
    ==14111==    by 0x418868: file_dp_request (dp_request.c:254)
    ==14111==    by 0x418976: dp_req_send (dp_request.c:300)
    ==14111==    by 0x41C25F: dp_pam_handler (dp_target_auth.c:219)
    ==14111==    by 0x52B3456: sbus_request_invoke_or_finish (sssd_dbus_request.c:71)
    ==14111==    by 0x52B0F37: sbus_message_handler_got_caller_id (sssd_dbus_interface.c:1048)
    ==14111==    by 0x923C923: tevent_common_loop_immediate (tevent_immediate.c:135)

if (kr->pd->cmd == SSS_PAM_AUTHENTICATE ||
kr->pd->cmd == SSS_PAM_PREAUTH ||
kr->pd->cmd == SSS_CMD_RENEW ||
kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM ||
kr->pd->cmd == SSS_PAM_CHAUTHTOK) {
buf->size += 5*sizeof(uint32_t) + strlen(kr->ccname) + strlen(keytab) +
buf->size += 4*sizeof(uint32_t) + strlen(kr->ccname) + strlen(keytab) +
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity ... by your commit message I understand the reason for increasing the buf->size's size in one uint32_t. But what's the reason to decrease it here? Is that because this if does not handle authorization?

Copy link
Contributor

Choose a reason for hiding this comment

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

Explanation is trivial. variable posix_domain is always passed to krb5-child.child and not just in few cases.

@fidencio check changes in the commit 861ab44
src/providers/krb5/krb5_child.c -> unpack_buffer
and src/providers/krb5/krb5_child_handler.c -> create_send_buffer

https://pagure.io/SSSD/sssd/blob/master/f/src/providers/krb5/krb5_child_handler.c#_197

@lslebodn
Copy link
Contributor

lslebodn commented Jun 1, 2017

I tested with:

[domain/sssdad.com]
debug_level = 0xFFF0
id_provider = ad
access_provider = krb5
krb5_realm = SSSDAD

and I could not see any valgrind errors.
But I could see errors in debug log files

[write_pipe_handler] (0x0400): All data has been sent!
[read_pipe_handler] (0x0400): EOF received, client finished
[krb5_access_done] (0x0020): message has the wrong size.
[krb5_pam_handler_access_done] (0x1000): Access denied for user [testuser01-12283@sssdad.com].
[dp_req_done] (0x0400): DP Request [PAM Account #3]: Request handler finished [0]: Success
[_dp_req_recv] (0x0400): DP Request [PAM Account #3]: Receiving request data. 
[dp_req_destructor] (0x0400): DP Request [PAM Account #3]: Request removed.
[dp_req_destructor] (0x0400): Number of active DP request: 0
[dp_method_enabled] (0x0400): Target selinux is not configured
[dp_pam_reply] (0x1000): DP Request [PAM Account #3]: Sending result [6][sssdad.com]

and

[[sssd[krb5_child[7203]]]] [main] (0x0400): krb5_child started.
[[sssd[krb5_child[7203]]]] [unpack_buffer] (0x1000): total buffer size: [79]
[[sssd[krb5_child[7203]]]] [unpack_buffer] (0x0100): cmd [243] uid [406918239] gid [1239000513] validate [false] enterprise principal [false] offline [false] UPN [testuser01-12283@SSSDAD.COM]
[[sssd[krb5_child[7203]]]] [k5c_recv_data] (0x0020): unpack_buffer failed.
[[sssd[krb5_child[7203]]]] [main] (0x0020): krb5_child failed!

errors are gone with patch but I haven't tested with id_provider = ldap and access_provider = krb5

@lslebodn
Copy link
Contributor

lslebodn commented Jun 1, 2017

I am able to see issue in valgrind log. Issue was on my side :-) I tried to find(grep) ERROR summary in valgrind log but it is done only after finishing process. And I didn't stop sssd

@lslebodn
Copy link
Contributor

lslebodn commented Jun 1, 2017

master:

@lslebodn lslebodn closed this Jun 1, 2017
@lslebodn lslebodn added the Pushed label Jun 1, 2017
@jhrozek jhrozek deleted the krb5acc branch January 20, 2019 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants