GNOME Bugzilla – Bug 751038
luks_delete_passphrase_cb(): gvfs-udisks2-volume-monitor killed by SIGSEGV
Last modified: 2015-08-06 06:15:52 UTC
Backtrace: #0 luks_delete_passphrase_cb at gvfsudisks2volume.c:1132 #1 g_simple_async_result_complete at gsimpleasyncresult.c:763 #2 on_delete_searched at libsecret/secret-methods.c:1641 #3 g_simple_async_result_complete at gsimpleasyncresult.c:763 #4 on_search_items_complete at libsecret/secret-paths.c:316 #5 g_simple_async_result_complete at gsimpleasyncresult.c:763 #6 reply_cb at gdbusproxy.c:2623 #7 g_simple_async_result_complete at gsimpleasyncresult.c:763 #8 g_dbus_connection_call_done at gdbusconnection.c:5508 #9 g_simple_async_result_complete at gsimpleasyncresult.c:763 See downstream bugreport for more details: https://bugzilla.redhat.com/show_bug.cgi?id=1206966 See two probably related bugs: - accidental deletion of luks password Bug 730759 - similar crash when locking Bug 678555
Created attachment 305384 [details] [review] Do not clear passphrase if cancelled I can't reproduce this issue, but I have found out that secret_password_clear is called whenever udisks_encrypted_call_unlock fails. This is wrong, the password should be cleared only if obsolete passphrase is in keyring. It would be best to add special error code in udisks if passphrase is invalid. There is comment in the code: "TODO: ideally check against something like UDISKS_ERROR_PASSPHRASE_INVALID when such a thing is available in udisks" We can improve the behavioral and do not clear the passphrase if G_IO_ERROR_CANCELLED is returned at least. The G_IO_ERROR_CANCELLED is returned when you unplug the encrypted device in the meantime.
Unfortunately also I see two other error messages if the device is unplugged: Error unlocking /dev/sdb1: Command-line `cryptsetup luksOpen "/dev/sdb1" "luks-90c34690-5f6f-4adf-9333-7651ccfa0a29" ' exited with non-zero exit status 4: Device /dev/sdb1 doesn't exist or access denied. Error unlocking /dev/sdb1: Command-line `cryptsetup luksOpen "/dev/sdb1" "luks-90c34690-5f6f-4adf-9333-7651ccfa0a29" ' exited with non-zero exit status 1: IO error while decrypting keyslot. We should to clear the password only if "Error unlocking /dev/sdb1: Command-line `cryptsetup luksOpen "/dev/sdb1" "luks-90c34690-5f6f-4adf-9333-7651ccfa0a29" ' exited with non-zero exit status 2: No key available with this passphrase." is returned. But only way how to distinguish the errors is string comparing, which isn't really best idea...
The attached patch will definitely reduce number of crashes, but I'm not really sure if it fixes the root of the problem. Otherwise the code looks good to me, however private data structures are passed a lot there and might be freed too early...
(In reply to Ondrej Holy from comment #1) > Created attachment 305384 [details] [review] [review] > Do not clear passphrase if cancelled > > I can't reproduce this issue, but I have found out that > secret_password_clear is called whenever udisks_encrypted_call_unlock fails. > This is wrong, the password should be cleared only if obsolete passphrase is > in keyring. It would be best to add special error code in udisks if > passphrase is invalid. There is comment in the code: "TODO: ideally check > against something like UDISKS_ERROR_PASSPHRASE_INVALID when such a thing is > available in udisks" > > We can improve the behavioral and do not clear the passphrase if > G_IO_ERROR_CANCELLED is returned at least. The G_IO_ERROR_CANCELLED is > returned when you unplug the encrypted device in the meantime. Actually the passphrase isn't cleared if G_IO_ERROR_CANCELLED is returned, because g_secret_password_clear takes cancellable as a parameter and fails immediately. But still this patch can be useful to safe one additional call.
Created attachment 305806 [details] [review] udisks: Handle libsecret error properly secret_password_clear_finish() returns whether any passwords are removed, so it may return FALSE without setting error. Handle this properly (in this case all we care about is that there wasn't an error).
Review of attachment 305384 [details] [review]: Probably OK, but the commit message should be: "but this definitely isn't a reason for clearing the passphrase." Also, you should also include the GNOME bugzilla link.
Created attachment 305984 [details] [review] udisks2: Do not call secret_password_clear_if_cancelled Thanks for the review! Wrong link replaced. Commit message was changed, but as per Comment 4...
Review of attachment 305806 [details] [review]: That's it probably! I realized this also in the meantime... Just a nitpick, the commit message should begin with "udisks2:".
Pushed to master as 5c2ac3f1ac452755df1c08480a0ee944138b42ec. Thanks!
I also pushed this to gnome-3-14 and gnome-3-16.