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

nss: inspect returnvalue of token check #4110

Closed

Conversation

@danielgustafsson
Copy link
Member

commented Jul 12, 2019

PK11_IsPresent() checks for the token for the given slot is available, and sets needlogin flags for the PK11_Authenticate() call. Should it return false, we should however treat it as an error and bail out.

@kdudka is there a reason to ignore the returnvalue of PK11_IsPresent() that I'm not seeing?

nss: inspect returnvalue of token check
PK11_IsPresent() checks for the token for the given slot is available,
and sets needlogin flags for the PK11_Authenticate() call.  Should it
return false, we should however treat it as an error and bail out.

@danielgustafsson danielgustafsson requested a review from kdudka Jul 12, 2019

@kdudka

kdudka approved these changes Jul 15, 2019

Copy link
Collaborator

left a comment

The only purpose of the SECMOD_WaitForAnyTokenEvent()/PK11_IsPresent() calls is to force NSS to invalidate its internal cache (originally intended to optimize out unneeded accesses to HW tokens, which could be slow I/O devices). Even NSS itself ignores the return value of PK11_IsPresent() in such cases:
https://github.com/nss-dev/nss/blob/ac64f9e6/lib/pk11wrap/pk11util.c#L1505
https://github.com/nss-dev/nss/blob/ac64f9e6/lib/pk11wrap/pk11util.c#L1635

However, your patch looks fine and it could ease debugging of some unexpected situations.

@jay jay closed this in e5b371d Jul 17, 2019

@jay

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Thanks

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

The only purpose of the SECMOD_WaitForAnyTokenEvent()/PK11_IsPresent() calls is to force NSS to invalidate its internal cache (originally intended to optimize out unneeded accesses to HW tokens, which could be slow I/O devices). Even NSS itself ignores the return value of PK11_IsPresent() in such cases:

Aha, when scanning the NSS code (not carefully enough) I came across cases where the return value was checked and missed these. This makes a lot of sense though, thanks for clarifying.

caraitto added a commit to caraitto/curl that referenced this pull request Jul 23, 2019

nss: inspect returnvalue of token check
PK11_IsPresent() checks for the token for the given slot is available,
and sets needlogin flags for the PK11_Authenticate() call.  Should it
return false, we should however treat it as an error and bail out.

Closes curl#4110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.