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

Passkey: Add child timeout handler #6893

Closed

Conversation

justin-stephenson
Copy link
Contributor

If passkey auth times out, the SIGCHLD handler needs to be destroyed otherwise the SIGCHLD handler tries to access the tevent_req *req which was already freed from the timeout.

This can be reproduced with local passkey auth - after entering a PIN don't press the passkey device tactile trigger, invoking the passkey child timeout.

@alexey-tikhonov alexey-tikhonov added backport-to-stable passkey Issues and PRs related to 'passkey' feature labels Aug 21, 2023
@alexey-tikhonov
Copy link
Member

Please add "Resolves:" line.

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.

LGTM!

I added this patch to the COPR repo so it's easier to test.

@justin-stephenson
Copy link
Contributor Author

Please add "Resolves:" line.

Done.

struct pam_passkey_auth_send_state *state =
tevent_req_data(req, struct pam_passkey_auth_send_state);

DEBUG(SSSDBG_CRIT_FAILURE, "Timeout reached for passkey child\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

since there is the passkey_child_timeout option, what do you think about mention it here, e.g. similar to p11_child_timeout().

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, updated.

child_handler_destroy(state->child_ctx);
state->child_ctx = NULL;
state->child_status = ETIMEDOUT;
tevent_req_error(req, ERR_PASSKEY_CHILD);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

I think it is worth to add a dedicated ERR_PASSKEY_CHILD_TIMEOUT error code her.

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

If passkey auth times out, the SIGCHLD handler needs to be
destroyed otherwise the SIGCHLD handler tries to access the tevent_req
which was already freed from the timeout.

Resolves: SSSD#6889
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 updates, ACK.

bye,
Sumit

@pbrezina
Copy link
Member

Pushed PR: #6893

  • master
    • b516f1e - Passkey: Add child timeout handler
  • sssd-2-9
    • e71a353 - Passkey: Add child timeout handler

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Aug 31, 2023
@pbrezina pbrezina closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
passkey Issues and PRs related to 'passkey' feature Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants