GNOME Bugzilla – Bug 326168
User with no permision to write on the cd not get wrong information.
Last modified: 2009-08-12 19:18:23 UTC
Please describe the problem: When the user don't have write permission on the device (not being on the cdrom group for example) get an "there is no writable disc on the unit, insert Please put a blank disc, with at least XXX MiB free, into the drive." information dialog, even if they have a blank disc on the drive. So the user could think that the cd is already written, or something. Steps to reproduce: 1. Select an iso image to write (or build an burn:// session) 2. Be sure that you don't have write permissions on the writing device 3. Insert a blank CD on the drive 4. Right button->Write this image in order to start burning. Actual results: I get wrong information about my blank cd Expected results: A message with what's really happening, I don't have permission to write the cd. Does this happen every time? Yes Other information:
Thanks for the bug report! Confirming.
Created attachment 79935 [details] [review] Permission check on drive and suid/sgid on cdrecord or growisofs I've been unable to test it yet, however, I would like to have some review until tests are made.
Comment on attachment 79935 [details] [review] Permission check on drive and suid/sgid on cdrecord or growisofs Thanks for giving this a try! Some comments on the patch. >+gboolean command_can_write_drive (const char * command, NautilusBurnDrive * drive) >+{ >+ gchar * program; >+ const char * device; >+ struct stat * program_stat = NULL; >+ struct stat * device_stat = NULL; Please try to follow the coding style of the rest of the file. This means the above should: - have function return type on a separate line as function name - have function arguments on separate lines - have function argument variables aligned with each other - use same indentation as rest of file - align variable definitions - (minor point) don't add a space between (*) and variable Other things. Don't mix the usage of gchar and char - in fact just use char. This function should probably be marked static. >+ >+ g_return_val_if_fail (command != NULL, FALSE); >+ g_return_val_if_fail (drive != NULL, FALSE); >+ >+ device = nautilus_burn_drive_get_device (drive); >+ >+ program = g_find_program_in_path ((const gchar *)command); You shouldn't need a cast here. >+ /* If some error occured on stat, assumes it cannot write */ >+ g_return_val_if_fail (!g_stat ((const gchar *)program, program_stat) && >+ !g_stat ((const gchar *)device, device_stat), FALSE); g_return_* functions are only used to test for conditions that are not allowed. For example, if a NULL object is passed to a function that requires a non-NULL object then g_return_* is used to fail and either issue a warning or assert and fail. >+ g_return_val_if_fail (nautilus_burn_drive_device_write_ok (drive) == FALSE, TRUE); Same here. >+ /* Checks root owns the executable and have suid */ >+ g_return_val_if_fail ((program_stat->st_uid == 0) && (device_stat->st_mode | (S_ISUID)), >+ FALSE); >+ >+ /* Checks owner of executable and device and suid */ >+ g_return_val_if_fail ((program_stat->st_uid == device_stat->st_uid) && >+ (device_stat->st_mode | (S_IRUSR | S_IWUSR | S_ISUID)), >+ FALSE); >+ >+ g_return_val_if_fail ((program_stat->st_gid == device_stat->st_gid) && >+ (device_stat->st_mode | (S_IRGRP | S_IWGRP | S_ISGID)), >+ FALSE); Same for all these uses. >+ return TRUE; >+} >+ > static gboolean > cdrecord_stdout_line (NautilusBurnProcess *process, > const char *line, >@@ -866,6 +902,7 @@ > gboolean can_rewrite; > gboolean is_blank; > gboolean is_locked; >+ gchar *command; > int ret; Just use char. > g_return_val_if_fail (recorder != NULL, NAUTILUS_BURN_RECORDER_RESULT_ERROR); >@@ -912,6 +949,8 @@ > } > > if (cd_write_needs_growisofs (drive, type, tracks)) { >+ command = "growisofs"; >+ You've defined "command" as a "char *" but you are using it here as a "const char *" > ret = nautilus_burn_recorder_write_growisofs (recorder, > drive, > tracks, >@@ -919,6 +958,8 @@ > flags, > error); > } else { >+ command = "cdrecord"; >+ Same here. > ret = nautilus_burn_recorder_write_cdrecord (recorder, > drive, > tracks, >@@ -927,6 +968,16 @@ > error); > } > >+ if (!command_can_write_drive ((const char *)command, drive)) >+ { >+ g_set_error (error, >+ NAUTILUS_BURN_RECORDER_ERROR, >+ NAUTILUS_BURN_RECORDER_ERROR_GENERAL, >+ _("You have not permissions to burn drive")); >+ >+ return NAUTILUS_BURN_RECORDER_RESULT_ERROR; >+ } Please try to use the brace and indentation style of the rest of the file. The string should probably say: "You do not have permission to write to this drive". > if (is_locked) { > nautilus_burn_drive_unlock (drive); > } >Index: src/nautilus-burn-drive.c >=================================================================== >--- src/nautilus-burn-drive.c (revisión: 1974) >+++ src/nautilus-burn-drive.c (copia de trabajo) >@@ -284,6 +284,31 @@ > } > > /** >+ * nautilus_burn_drive_device_write_ok: >+ * @drive: #NautilusBurnDrive >+ * >+ * Report the whether the user has permission to drive >+ * >+ * Returns: %TRUE if the user can rewrite, otherwise return %FALSE. >+ * >+ * Since: 2.18 >+ **/ I don't really like this name. Perhaps we can have: gboolean nautilus_burn_drive_access (NautilusBurnDrive *drive int mode, GError **error) Where the mode is the same as g_access(). And we can return an error message. >+gboolean >+nautilus_burn_drive_device_write_ok (NautilusBurnDrive *drive) >+{ >+ const char * device; >+ >+ g_return_val_if_fail (drive != NULL, FALSE); >+ >+ device = nautilus_burn_drive_get_device (drive); >+ >+ g_return_val_if_fail (g_access ((const gchar *)device, W_OK) == 0, >+ FALSE); >+ >+ return TRUE; >+} >+ >+/** > * nautilus_burn_drive_can_rewrite: > * @drive: #NautilusBurnDrive > * >Index: src/nautilus-burn-drive.h >=================================================================== >--- src/nautilus-burn-drive.h (revisión: 1974) >+++ src/nautilus-burn-drive.h (copia de trabajo) >@@ -128,6 +128,9 @@ > char * nautilus_burn_drive_get_name_for_display (NautilusBurnDrive *drive); > const char * nautilus_burn_drive_get_device (NautilusBurnDrive *drive); > >+gboolean nautilus_burn_drive_device_write_ok (NautilusBurnDrive *drive); >+ >+ > /* Capabilities */ > gboolean nautilus_burn_drive_can_write (NautilusBurnDrive *drive); > gboolean nautilus_burn_drive_can_rewrite (NautilusBurnDrive *drive);
Created attachment 81216 [details] [review] Permission check on drive and suid/sgid on cdrecord or growisofs It seems to work ok for device permissions, but I haven't been able to get a decent message for lack of RW perms on cdrecord/growisofs.
Ubuntu bug https://bugs.launchpad.net/nautilus-cd-burner/+bug/97597 "Binary package hint: nautilus-cd-burner To be able to burn cds in ubuntu you need to give the user the permission to do so. There are several ways of doing this, the most user friendly way is to go to the User and Groups option in the Administration menu. The CD burning functionality in Nautilus does not respect this option, it allows the user to burn a CD which will later fail because the user cannot write to the cdrom device. Ideally nautilus-cd-burner should as well check if the user haven't logged out yet, because it will still fail if you find out that you lack permission to burn and later add it without logging out and in again. ..."
Created attachment 92866 [details] [review] Permission check on drive and suid/sgid on cdrecord or growisofs Style fixes and better api addition name and doc comment header.
So, one problem with the patch is that there is no need to require the tools to have setuid root on many systems. And it is probably too late to add API in this cycle. Also, wouldn't it be better to just improve the error message when the write fails instead of trying to anticipate the error?
Anyway, I don't quite understand your suggestion with the setuid check, can you elaborate? If the problem is the public API I just followed the instructions on how to address the patch that Fabio suggested me once on IRC. We could just make a private function for this release? Regarding the permission lookahead, I don't think that looking ahead is such a bad idea, I wonder if we should let the user click the write button or select the drive if he is not going to be able to write anyway. From an usability point of view, I find it more convenient, but I'm open to suggestions. I haven't tried to improve the patch because I would like to have some input on where to go first. I would like to have a short term solution at least, what would be the changes needed to solve the problem for 2.20 in your opinion?
On many/most linux systems cdrecord etc are not required to have setuid root and so don't have it. I think I would prefer to handle the error from cdrecord better and not try to anticipate it. Even in your use-case this patch wouldn't work if cdrecord was setgid cdrom, right?
I know that cdrecord is not required, but if cdrecord has rights to write, I think that this situation should be exposed on the tool. I would find quite confusing if a sysadmin chooses this policy to grant writing rights in multiuser environments (subliminal ad here ;-) and the recorder just says that it has no rights, when actually, it has them. Regarding the setgid, I didn't though in that case. I don't know when I might find time to change the logic of the patch, actually, I wrote it some time ago and I've forgot most about it (the last review was a style fix and just an attempt to catch the reviewer's attention). I followed the tips Fabio gave me back then, and I find kind of discouraging to change it now.
This patch is broken. It requires cdrecord to be SUID, or have some combination of permissions. There's a number of cases where we might be blocking the device access, such as SELinux contexts, AppArmour, ACLs (if the drive is external, and we use fast-user switching). I don't think you want to implement that. Jon's suggestion doesn't require new API. You need to catch the error from cdrecord/growisofs and make it have a better error message when permission on the device is denied. I'm not sure whether nautilus-cd-burner already has API for it, but it might be useful to have a "retry" button in such cases (so that an ISO image doesn't need to be deleted/recreated when the write fails). But that's more a slight speedup than a requirement to fix this problem.
I think you're right, I'll try to focus on making a less intrusive patch and delay the lookahead permission for a better UI experience for another bug until we find a good solution for it. I might find some time this weekend but dunno if I'll be able to came up with the full fix.
A debug log of what the errors look like would be a good start.
So, any news on this one?
Closing this bug report as no further information has been provided. Please feel free to reopen this bug if you can provide the information asked for. Thanks!