Bug 145252

I am also puzzled about the permissions in the context of digikam,
for example why do freshly generated thumbs have
"-rw-------" as rights, even though the umask is on 022.
(i.e. freshly generated files via touch are "-rw-r--r--"),
http://mail.kde.org/pipermail/digikam-devel/2007-April/012183.html
(I am using debian etch, if that matters here at all).

Independent of this, I am not sure whether it is 
a good idea to share digikam albums in the way you intend.
At least you have absolutely make sure that digikam is never run 
by two users at the same time. Otherwise it is very likely
that you damage your database.

I had a look at the rights problem concerning
the thumbnails first:
Thumbnail generation is done in 
  graphics/digikam/kioslave/digikamthumbnail.cpp
where one finds the construct
  KTempFile temp(thumbPath + "-digikam-", ".png");
into which then the thumbnail is written by
  img.save(temp.name(), "PNG", 0);
After this the file is renamed.
According to the documentation of KTempFile the default mode is 0600,
i.e. -rw------- 

So if there is a way to create the temporary
file with the current umask, all should be
fine.

Note that in digikam/kioslave/digikamalbums.cpp 
one finds this approach:
    if ( ::mkdir( _path.data(), 0777 /*umask will be applied*/ ) != 0 )

So to me it seems that 
  // Using 0777 as mode means that the umask will be applied.
  KTempFile temp(thumbPath + "-digikam-", ".png", 0777 );
should solve this problem.

Does this sound reasonable, or are there any security (or other
problems?) with this approach?

I suspect that the other rights problems are similar ...

Best, Arnd

Yes, for images changed in the image editor it should be just the same: in
graphics/digikam/utilities/imageeditor/editor/editorwindow.cpp 
the construct 
  m_savingContext->saveTempFile   = new KTempFile(newURL.directory(false),      
       ".digikamtempfile.tmp");
is used.

So if changing the mode of the temporary files in the first place
makes sense, I will write and test a patch later this evening ...

Arnd,

It sound resonable for me but i can lack few security rules here... I would to have the feedback from system admin for example.

Achim, have you few comments ?

Gilles

Created attachment 21852 [details]
use umask settting for thumbnails and edited images

The attached patch solves the problem for me, 
i.e. modified images and newly generated thumbs have
-rw-r--r-- (when the umask is on 022)

If there should be any security problem, I would guess that the whole approach.

of using temporary files needs to be re-thought.
Therefore, I would suggest to apply the patch (after testing, of course... ;-)

About caches (thumbnail, fulltext indices, htmlpages ...):

The reason they should be readable only by the owner is that
one should only have access to the cached info, if you have
access to the original data.

If e.g. another person has access to an image depends on 
each componemt of the full path to the image.  Nothing one can
map into a single protection mask of a file.   Keeping caches
and access permissions of external images in sync is impossible.
Ditto for date base entries. (fwiw digikam3.db violates this
by default)

One way to circumvent this problem is used in (s)locate:  It
collects everything as root, but readable only by _one_ specially
empowered executable.  And this program makes sure that only meta
data are delivered if the user has access to the original data.


If one lowers the access to meta information, it should never be
done by default.  If user does it, should be informed about the
consequences.

Achim

Hi Achim,

well, I have to admit, that I don't fully understand the consequences
of your description to the original problem (or patch),
i.e. what should we do?

To me the situation seems the following:
- Currently (i.e. without patch)
  thumbnails and modified images result in files with 
  -rw-------
  Any value of the current umask is ignored.
- With the patch above, the umask information is used
   so that thumbnails/modified images may end up as
  -rw-r--r--
  (if umask is set to 022)
  or 
  -rw-------
  (for a different setting of the umask)
 
  So this is in full control of the user and behaves in the same
  way as, for example `touch new_file_to_see_its_rights`.

  This also leads to a consistent behaviour, as for newly created
   directories the mode 0777 is in digikamalbums.cpp.

So for new files, I would think that respecting the umask 
should be ok.

For thumbnails I do understand the problem that this might make
files visible to other users, which originally are not visible
(due to paths right settings etc.).

So the question is: what should we do?
Should this be made a configuration option for the thumbnails,
or better left as is?

>So for new files, I would think that respecting the umask
>should be ok. 

Fine for me too...

>For thumbnails I do understand the problem that this might make
>files visible to other users, which originally are not visible
>(due to paths right settings etc.).

>So the question is: what should we do?
>Should this be made a configuration option for the thumbnails,
<or better left as is? 

No need a setting for thumb. This will bloat the config (:=))), and most users we don't care about...

Why not to replicate the same umask from original image to thumb files ?

Gilles

> Why not to replicate the same umask from original image to thumb files ?


This could expose files which were previously not visible
due to directory rights setting
(And this was one of Achim's main points, right?)

I just checked: konquerer creates thumbs also only as -rw-------
(independent of the mode of the original file)
so presumably we should do the same...

> >So for new files, I would think that respecting the umask
> >should be ok. 
> 
> Fine for me too...


 new files: I agreed.
 modified files: use same protection as the unmodified version had.

> >For thumbnails I do understand the problem that this might make
> >files visible to other users, which originally are not visible
> >(due to paths right settings etc.).
> 
> >So the question is: what should we do?
> >Should this be made a configuration option for the thumbnails,
> <or better left as is? 
> 
> No need a setting for thumb. This will bloat the config (:=))), and most users we don't care about...]
> 
> Why not to replicate the same umask from original image to thumb files ?


Nope.  That's as I tried to explain before IMHO bad design.

IMHO a primitive but clean solution to sharing thumb problem could be
use something like LibraryPathAccess=[default|private|shared] (without
GUI until we properly support it ;)

If set to 'shared' search/create first

	~/Pictures/.thumbnails/...

with thumb having the same protection as the image when created/updated.


Default == as it's right now.  Optional: For 'private'
~/.thumbnail is used and ~/Pictures/ (700) and digikam.db (600), with
as a start checking (changing after asking) protection ~/Picture and
.digikam*.db).


How to realize write access is left as an excercise for the user for
now ;)  To make it possible digikam should use try use umask for
new stuff and keep protection when modifying files.

Achim

>I just checked: konquerer creates thumbs also only as -rw-------
>(independent of the mode of the original file)
>so presumably we should do the same... 

Right, because digiKam and Konqueror share thumbs. No others way can be done...

Gilles

yes.

Using ~/.thumbnails 'mean' I want to share it with all my gnome/kde apps
and as this case is used 'behind' the scene --> play save, use mode 600.

Using ~/Pictures/.thumbnails/ would imply the intent to share the thumbs
with digikam.

AFAIR the freedesktop spec (argl. access denied for http://jens.triq.net/thumbnail-spec/index.html as referenced by f.d.o)
has a search patch  (e.g. for prepared thumbs in readonly CDROM).
_Maybe_ just creating  Pictures/.thumbnails/*/ would make all apps
managing thumb like f.d.o  spec use them  (that would be really cool ;)

Well, for the moment I think it is simplest to:
a) don't touch the thumbnails
b) new files: respect umask
   modified files: use same protection as the unmodified version had. 
   (as Achim suggested above)

For the second case in b): How do I get the mode
from the file?
I.e. the code will (presumably) look like this:
  mode = 0666;
  if (m_savingContext->destinationExisted)
      mode = CALL_TO_GET_THE_MODE_OF_THE_ORIGINAL(m_savingContext->srcURL);
I could not find a method of the KURL class which would provide this.
Is there any KDE/Qt way to do it, or what is the best approach here?

Thanks, Arnd

Arnd,

Are you take a look in this method : EditorWindow::moveFile()

You need to use ::stat() to get file mode. EditorWindow::moveFile() do it already...

But i don't know where is the problem in image editor with temp file generated when a new image is saved, because EditorWindow::moveFile() is called after than temp file is created to create/overwrite target file. If you check code in this method, you will see than mode is already get and restored as well...

If something must be done to solve this file, this must be done in EditorWindow::moveFile(). Right ?

Gilles

Created attachment 21865 [details]
respect umask for newly created files.

> If something must be done to solve this file, this must be done in
EditorWind$

Yes - you are (of course) right!
The relevant part is
    // store old permissions
    mode_t filePermissions = S_IREAD | S_IWRITE;
(If a file previously exists, `filePermissions` is modified, so that
its protections are used.)
So instead of S_IREAD | S_IWRITE, the umask has to
be taken into account.

The attached patch tries to do so and also applies the permissions
in both situations of overwritten or newly created files.

Achim, could you maybe test the patch, if everything is ok?

Best, Arnd

Achim, could you maybe have a look at this patch?
(It would be nice to get this into 0.9.3 and close the bug ;-)

Is it the same problem that occurs with showfoto?
All is right with digikam for me, but permissions changes when I edit photos.

@Arnd, works fine with digikams imageeditor.

@Fabrice: I've forgot to test showfoto before the patch, but after editing
a file with mode 0600 the save-as version has the default protection. (0644 here).  So IMHO it's fixed.

Fabrice : showfoto share a common implementation with digiKam image editor.

Gilles Caulier

SVN commit 786697 by abaecker:

Ensure that the umask setting is respected for both newly created files
and when overwriting files.

CCBUGS: 145252
TODO:KDE4PORT




 M  +2 -1      NEWS  
 M  +12 -7     utilities/imageeditor/editor/editorwindow.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=786697

SVN commit 786733 by cgilles:

backport commit #786697 from KDE3 branch
CCBUGS: 145252


 M  +12 -7     editorwindow.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=786733

Can't this bug be closed now?

yes, for me all is fixed.

Gilles Caulier