[icedtea-web] RFC: reduce permissions on created files

[Deepak Bhole]

* Omair Majid <omajid at redhat.com> [2010-11-24 13:49]:
> On 11/24/2010 01:41 PM, Deepak Bhole wrote:
> >* Omair Majid<omajid at redhat.com>  [2010-11-23 14:54]:
> >>On 11/22/2010 03:31 PM, Deepak Bhole wrote:
> >>>* Omair Majid<omajid at redhat.com>   [2010-11-12 09:46]:
> >>>>On 11/12/2010 06:54 AM, Dr Andrew John Hughes wrote:
> >>>>>On 20:14 Thu 11 Nov     , Omair Majid wrote:
> >>>>>>Hi,
> >>>>>>
> >>>>>>The attached patch tries to make files created by netx/plugin more
> >>>>>>secure by removing unnecessary permissions. IcedTea6 used to carry a
> >>>>>>patch to change the umask used by the javaws process which IcedTea-Web
> >>>>>>does not. This patch tries to make netx/plugin behave like they would in
> >>>>>>the presence of such a patch.
> >>>>>>
> >>>>>>This patch does not change the file permissions on files that are cached
> >>>>>>(mostly because I dont see why they should be protected), but does
> >>>>>>change permissions on KeyStores, native directories created under /tmp/,
> >>>>>>lock files, files created through the JNLP api and log files.
> >>>>>>
> >>>>>
> >>>>>I think this is a better approach, though we should be aware that the permissions
> >>>>>are now changed after the file has been created, whereas before (I presume) they
> >>>>>were created with the correct permissions to begin with.
> >>>>>
> >>>>
> >>>>Hm... Does creating a new (blank) file, setting appropriate
> >>>>permissions on it and then writing content to it ensure that the
> >>>>content can not be seen by others? This was the original intent with
> >>>>this patch (I have since noticed one mistake in the patch). If
> >>>>creating a blank file, setting permissions on it and then writing
> >>>>actual content does not ensure the confidentiality of the data, then
> >>>>I would like to find another way to accomplish this.
> >>>>
> >>>>Thanks for looking over the patch.
> >>>>
> >>>
> >>>Good point Andrew! This patch needs to be modified. If a file is created
> >>>with og+[r|w|x] and then restricted, anything that opened the handle
> >>>before the change will have the permissions it did when the handle was
> >>>opened. We need to create the file under a different (temp) name, change
> >>>permissions, and then move it. AFAIK that shouldn't allow access.
> >>>
> >>>Access bypass can be verified as follows:
> >>>1. Create a file as one user (A), allow a+r
> >>>2. tail -f that file as another user (B)
> >>>3. As A, og-rwx the file
> >>>4. As A, write to the file
> >>>
> >>>The tail -f command from #2 will show the output, thus confirming that
> >>>user B has access when they shouldn't.
> >>>
> >>>tail -f no longer gets the info if the file is renamed to something else
> >>>between step 3 and 4.
> >>>
> >>
> >>Thanks for the name change idea. The attached patch implements it. Thoughts?
> >>
> >
> >Looks good! Once suggestion though -- it might be helpful for
> >debugging/catching errors more easily if the exceptions thrown in
> >createRestrictedFile() printed the file name along with the message.
> >Also, the message should be in the message properties file imo, as users
> >might encounter this more frequently if they set the wrong values.
> >
> 
> Sure. Do we have a policy on what goes in the Messages.properties
> file? I know all user-visible GUI text belongs in there, but should
> we add (some? all?) exception messages in there too?

I think error messages that are only useful to devs are fine in code,
but messages that users can see and may need to understand (i.e. fixable
on their end), should be in Messages.properties..

Cheers,
Deepak