Changeset View
Standalone View
pam_kwallet.c
Show First 20 Lines • Show All 304 Lines • ▼ Show 20 Line(s) | 257 | { | |||
---|---|---|---|---|---|
305 | //even though we just set it, better check to be 100% sure | 305 | //even though we just set it, better check to be 100% sure | ||
306 | result = pam_get_item(pamh, PAM_AUTHTOK, (const void**)&password); | 306 | result = pam_get_item(pamh, PAM_AUTHTOK, (const void**)&password); | ||
307 | if (result != PAM_SUCCESS || !password) { | 307 | if (result != PAM_SUCCESS || !password) { | ||
308 | pam_syslog(pamh, LOG_ERR, "%s: Password is not there even though we set it %s", logPrefix, | 308 | pam_syslog(pamh, LOG_ERR, "%s: Password is not there even though we set it %s", logPrefix, | ||
309 | pam_strerror(pamh, result)); | 309 | pam_strerror(pamh, result)); | ||
310 | return PAM_IGNORE; | 310 | return PAM_IGNORE; | ||
311 | } | 311 | } | ||
312 | 312 | | |||
313 | char *key = malloc(KWALLET_PAM_KEYSIZE); | 313 | char *key = strdup(password); | ||
sitter: This can ENOMEM. Does that maybe need handling? Or will pam_set_data just fail if you give it a… | |||||
Passing nullptr is fine, see comment on https://github.com/linux-pam/linux-pam/blob/master/libpam/pam_data.c#L110 aacid: Passing nullptr is fine, see comment on https://github.com/linux-pam/linux… | |||||
314 | if (!key || kwallet_hash(pamh, password, userInfo, key) != 0) { | | |||
315 | free(key); | | |||
316 | pam_syslog(pamh, LOG_ERR, "%s: Fail into creating the hash", logPrefix); | | |||
317 | return PAM_IGNORE; | | |||
318 | } | | |||
319 | | ||||
320 | result = pam_set_data(pamh, kwalletPamDataKey, key, cleanup_free); | 314 | result = pam_set_data(pamh, kwalletPamDataKey, key, cleanup_free); | ||
321 | 315 | | |||
322 | if (result != PAM_SUCCESS) { | 316 | if (result != PAM_SUCCESS) { | ||
323 | free(key); | 317 | free(key); | ||
324 | pam_syslog(pamh, LOG_ERR, "%s: Impossible to store the hashed password: %s", logPrefix | 318 | pam_syslog(pamh, LOG_ERR, "%s: Impossible to store the password: %s", logPrefix | ||
325 | , pam_strerror(pamh, result)); | 319 | , pam_strerror(pamh, result)); | ||
326 | return PAM_IGNORE; | 320 | return PAM_IGNORE; | ||
327 | } | 321 | } | ||
328 | 322 | | |||
329 | //if sm_open_session has already been called (but we did not have password), call it now | 323 | //if sm_open_session has already been called (but we did not have password), call it now | ||
I wonder about this comment. Can the call sequence here be random? Can open be called before authenticate? sitter: I wonder about this comment. Can the call sequence here be random? Can open be called before… | |||||
That is a good question, the old code was kind of prepared for it. I am going to say "no" open can not be called before authenticate, if you read https://pubs.opengroup.org/onlinepubs/008329799/pam_open_session.htm it says "The pam_open_session() function opens a new session for a user previously authenticated with a call to pam_authenticate()." But my pam knowledge is between none and i googled a little, so I would be happy if someone can google a bit more and agree/disagree with me aacid: That is a good question, the old code was kind of prepared for it.
I am going to say "no" open… | |||||
Alex did add this check in a dedicated commit 634464255a82de55e0288f7e425e50f6c409f51d and even though I couldn't find a subsequent commit that made use of that preparation I'm sure he must have had a reason to add it. Perhaps it was this:
gnome-keyring, as the most topically relevant example, doesn't explicitly have 'open called before' but its written in a very particular way. Both auth and session can unlock the keyring (and attempt a daemon start) it seems. Not really the same, but there's certainly some nuance to how modules may get called. So, the original check for sm_open_session in our code may have been not entirely pointless, it does kinda meet that any-order expectation. It probably also wasn't entirely correct though as it didn't meet the independence expectation ^^ Removing it leaves me a bit uneasy without input from someone who knows more about pam and the two of us. At the same time it's clearly not quite right either. How about leaving it in for 5.18 and drop it for master? sitter: Alex did add this check in a dedicated commit 634464255a82de55e0288f7e425e50f6c409f51d and even… | |||||
Doesn't sound very convincing to me, if there's going to be a regression i'd prefer it to be this month when i still remember this code and i still use kwallet-pam and pam_fscrypt, if you delay it to 5.19.0 or something I will not totally remember this code and may not be using this any of those two either os my motivation to fix it may be pretty small. I can try adding the code that makes pam_sm_open_session from here if you think it makes sense to have it aacid: > How about leaving it in for 5.18 and drop it for master?
> Should there be an unexpected… | |||||
That is a fair point but I for one cannot +1 this diff as-is. So, I would prefer the pam_sm_open_session call making a return. Or someone else gives a +1, I don't profoundly object to the diff, but it is in my mind not suitable for a stable release right now: sitter: That is a fair point but I for one cannot +1 this diff as-is. So, I would prefer the… | |||||
330 | const char *session_bit; | 324 | const char *session_bit; | ||
331 | result = pam_get_data(pamh, "sm_open_session", (const void **)&session_bit); | 325 | result = pam_get_data(pamh, "sm_open_session", (const void **)&session_bit); | ||
332 | if (result == PAM_SUCCESS) { | 326 | if (result == PAM_SUCCESS) { | ||
333 | pam_syslog(pamh, LOG_ERR, "%s: open_session was called before us, calling it now", logPrefix); | 327 | pam_syslog(pamh, LOG_ERR, "%s: open_session was called before us, calling it now", logPrefix); | ||
334 | return pam_sm_open_session(pamh, flags, argc, argv); | 328 | return pam_sm_open_session(pamh, flags, argc, argv); | ||
335 | } | 329 | } | ||
336 | 330 | | |||
337 | //TODO unlock kwallet that is already executed | 331 | //TODO unlock kwallet that is already executed | ||
▲ Show 20 Lines • Show All 231 Lines • ▼ Show 20 Line(s) | 532 | { | |||
569 | 563 | | |||
570 | struct passwd *userInfo; | 564 | struct passwd *userInfo; | ||
571 | userInfo = getpwnam(username); | 565 | userInfo = getpwnam(username); | ||
572 | if (!userInfo) { | 566 | if (!userInfo) { | ||
573 | pam_syslog(pamh, LOG_ERR, "%s: Couldn't get user info (passwd) info", logPrefix); | 567 | pam_syslog(pamh, LOG_ERR, "%s: Couldn't get user info (passwd) info", logPrefix); | ||
574 | return PAM_IGNORE; | 568 | return PAM_IGNORE; | ||
575 | } | 569 | } | ||
576 | 570 | | |||
577 | const char *kwalletKey; | 571 | char *password; | ||
578 | result = pam_get_data(pamh, kwalletPamDataKey, (const void **)&kwalletKey); | 572 | result = pam_get_data(pamh, kwalletPamDataKey, (const void **)&password); | ||
579 | 573 | | |||
580 | if (result != PAM_SUCCESS) { | 574 | if (result != PAM_SUCCESS) { | ||
581 | pam_syslog(pamh, LOG_INFO, "%s: open_session called without %s", logPrefix, kwalletPamDataKey); | 575 | pam_syslog(pamh, LOG_INFO, "%s: open_session called without %s", logPrefix, kwalletPamDataKey); | ||
582 | return PAM_SUCCESS;//We will wait for pam_sm_authenticate | 576 | return PAM_SUCCESS;//We will wait for pam_sm_authenticate | ||
583 | } | 577 | } | ||
584 | 578 | | |||
585 | start_kwallet(pamh, userInfo, kwalletKey); | 579 | char *key = malloc(KWALLET_PAM_KEYSIZE); | ||
580 | if (!key || kwallet_hash(pamh, password, userInfo, key) != 0) { | ||||
581 | free(key); | ||||
582 | pam_syslog(pamh, LOG_ERR, "%s: Fail into creating the hash", logPrefix); | ||||
583 | return PAM_IGNORE; | ||||
584 | } | ||||
585 | | ||||
586 | start_kwallet(pamh, userInfo, key); | ||||
586 | 587 | | |||
587 | return PAM_SUCCESS; | 588 | return PAM_SUCCESS; | ||
588 | } | 589 | } | ||
589 | 590 | | |||
590 | PAM_EXTERN int pam_sm_close_session(pam_handle_t *pamh, int flags, int argc, const char **argv) | 591 | PAM_EXTERN int pam_sm_close_session(pam_handle_t *pamh, int flags, int argc, const char **argv) | ||
591 | { | 592 | { | ||
592 | pam_syslog(pamh, LOG_DEBUG, "%s: pam_sm_close_session", logPrefix); | 593 | pam_syslog(pamh, LOG_DEBUG, "%s: pam_sm_close_session", logPrefix); | ||
593 | return PAM_SUCCESS; | 594 | return PAM_SUCCESS; | ||
▲ Show 20 Lines • Show All 234 Lines • Show Last 20 Lines |
This can ENOMEM. Does that maybe need handling? Or will pam_set_data just fail if you give it a nullptr?