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

[Omair Majid]

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?