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

passkey: Skip processing non-passkey mapping data #7066

Closed
wants to merge 1 commit into from
Closed

passkey: Skip processing non-passkey mapping data #7066

wants to merge 1 commit into from

Conversation

justin-stephenson
Copy link
Contributor

In the AD case, the user altSecurityIdentities attribute can store passkey, smartcard, of ssh public key mapping data. Check to ensure we are handling passkey data before continuing in PAM passkey processing.

Resolves: #7061

@justin-stephenson justin-stephenson added backport-to-stable passkey Issues and PRs related to 'passkey' feature labels Dec 1, 2023
@alexey-tikhonov
Copy link
Member

of ssh -> or ssh

@justin-stephenson justin-stephenson mentioned this pull request Dec 1, 2023
Closed
@justin-stephenson justin-stephenson marked this pull request as ready for review December 4, 2023 13:36
@justin-stephenson justin-stephenson force-pushed the passkey_mapping_pubkey_fix branch from bc66d7b to d501cd6 Compare December 4, 2023 13:41
ikerexxe
ikerexxe reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

Some minor comments inline

src/responder/pam/pamsrv_passkey.c Outdated Show resolved Hide resolved
src/responder/pam/pamsrv_passkey.c Outdated Show resolved Hide resolved
src/responder/pam/pamsrv_passkey.c Outdated Show resolved Hide resolved
@justin-stephenson justin-stephenson force-pushed the passkey_mapping_pubkey_fix branch 2 times, most recently from 0832cd4 to b9e4643 Compare December 6, 2023 20:07
@justin-stephenson
Copy link
Contributor Author

I added a test which reproduces the crash if run without this PR.

[sssd] [passkey_local] (0x0400): Checking for passkey authentication data                                              
[sssd] [cache_req_set_plugin] (0x2000): CR #0: Setting "User by name" plugin                                           
[sssd] [cache_req_send] (0x0400): CR #0: REQ_TRACE: New request [CID #0] 'User by name'                                                                                                                                                       
[sssd] [cache_req_process_input] (0x0400): CR #0: Parsing input name [pamuser]                                         
[sssd] [cache_req_set_name] (0x0400): CR #0: Setting name [pamuser]                                                    
[sssd] [cache_req_select_domains] (0x0400): CR #0: Performing a multi-domain search                                                                                                                                                           
[sssd] [cache_req_search_domains] (0x0400): CR #0: Search will check the cache and check the data provider             
[sssd] [cache_req_validate_domain_type] (0x2000): Request type POSIX-only for domain pam_test type POSIX is valid                                                                                                                             
[sssd] [cache_req_set_domain] (0x0400): CR #0: Using domain [pam_test]                                                 
[sssd] [cache_req_prepare_domain_data] (0x0400): CR #0: Preparing input data for domain [pam_test] rules               
[sssd] [cache_req_search_send] (0x0400): CR #0: Looking up pamuser@pam_test                                                                                                                                                                   
[sssd] [cache_req_search_ncache] (0x0400): CR #0: Checking negative cache for [pamuser@pam_test]                       
[sssd] [sss_ncache_check_str] (0x2000): Checking negative cache for [NCE/USER/pam_test/pamuser@pam_test]               
[sssd] [cache_req_search_ncache] (0x0400): CR #0: [pamuser@pam_test] is not present in negative cache                  
[sssd] [cache_req_search_cache] (0x0400): CR #0: Looking up [pamuser@pam_test] in cache                                
[sssd] [cache_req_search_send] (0x0400): CR #0: Returning [pamuser@pam_test] from cache                                
[sssd] [cache_req_search_ncache_filter] (0x0400): CR #0: This request type does not support filtering result by negative cache                     
[sssd] [cache_req_create_and_add_result] (0x0400): CR #0: Found 1 entries in domain pam_test                           
[sssd] [cache_req_done] (0x0400): CR #0: Finished: Success                                                             
[sssd] [pam_passkey_get_user_done] (0x4000): Processing passkey data                                                   
[sssd] [process_passkey_data] (0x0040): talloc_strdup public key failed.                                               
[sssd] [passkey_local_verification] (0x0400): Passkey verification is being enforced from LDAP
[sssd] [passkey_local_verification] (0x0400): Passkey verification setting [unset]
[sssd] [pam_passkey_auth_send] (0x0400): Calling child with user-verification unset
[  ERROR   ] --- Test failed with exception: Segmentation fault(11)
[  FAILED  ] test_pam_passkey_bad_mapping
[==========] single_test: 1 test(s) run.
[  PASSED  ] 0 test(s).
[  FAILED  ] single_test: 1 test(s), listed below:
[  FAILED  ] test_pam_passkey_bad_mapping

 1 FAILED TEST(S)

@justin-stephenson justin-stephenson force-pushed the passkey_mapping_pubkey_fix branch 2 times, most recently from e5c649d to bb0f33f Compare December 7, 2023 15:46
@justin-stephenson
passkey: Skip processing non-passkey mapping data
a12f902 
In the AD case, the user altSecurityIdentities attribute can
store passkey, smartcard, or ssh public key mapping data. Check
to ensure we are handling passkey data before continuing in
PAM passkey processing.

:relnote: Fixes a crash when PAM passkey processing incorrectly
handles non-passkey data.

Resolves: SSSD#7061
@justin-stephenson justin-stephenson force-pushed the passkey_mapping_pubkey_fix branch from bb0f33f to a12f902 Compare December 7, 2023 15:51
sumit-bose
sumit-bose approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thanks for the fix and the test, ACK.

bye,
Sumit

@ikerexxe
Copy link
Contributor

I added a test which reproduces the crash if run without this PR.

Perfect! Thank you 😄

LGTM.

@alexey-tikhonov alexey-tikhonov added the Ready to push Ready to push label Dec 11, 2023
@alexey-tikhonov
Copy link
Member

Pushed PR: #7066

  • master
    • 6ed1eff - passkey: Skip processing non-passkey mapping data
  • sssd-2-9
    • 4d01e11 - passkey: Skip processing non-passkey mapping data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexey-tikhonov alexey-tikhonov alexey-tikhonov left review comments

@ikerexxe ikerexxe ikerexxe approved these changes

@sumit-bose sumit-bose sumit-bose approved these changes

Assignees

@ikerexxe ikerexxe

@sumit-bose sumit-bose

Labels
passkey Issues and PRs related to 'passkey' feature Pushed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sssd_pam segfaults during password-based SSH-login
4 participants
@justin-stephenson @alexey-tikhonov @ikerexxe @sumit-bose