[rfc][icedtea-web] Temporary Permissions generated via reflection (re-proposal)

[Andrew Azores]

On 06/13/2014 04:26 PM, Deepak Bhole wrote:
> * Andrew Azores <aazores at redhat.com> [2014-06-13 11:19]:
>> On 06/13/2014 11:09 AM, Andrew Azores wrote:
>>> Hi,
>>>
>>> When Temporary Permissions on the security dialogs were first introduced,
>>> I proposed to have them generated from the PolicyEditorPermissions
>>> dynamically using reflection, so that "grantable" permissions were only
>>> defined in one place and so maintenance would be easy, and the
>>> TemporaryPermissions class could be kept small. This was rejected due to
>>> the required use of reflection, but I am re-proposing this patch because I
>>> do believe it is a much cleaner solution, especially now that the number
>>> of grantable permissions has grown and the temporary permissions menu has
>>> become a multi-select UI rather than preset groups.
>>>
>>> ChangeLog:
>>>     TemporaryPermissions generated via reflection rather than hard coded
>>>     * netx/net/sourceforge/jnlp/resources/Messages.properties
>>>     (STempPermNoFile, STempPermNoNetwork, STempPermNoExec,
>>>     STempNoFileOrNetwork, STempNoExecOrNetwork, STempNoFileOrExec,
>>>     STempNoFileOrNetworkOrExec, STempAllMedia, STempSoundOnly,
>>>     STempClipboardOnly, STempPrintOnly, STempAllFileAndPropertyAccess,
>>>     STempReadLocalFilesAndProperties, STempReflectionOnly): removed
>>>     (STempNetwork, STempReadFilesAndProperties,
>>> STempWriteFilesAndProperties,
>>>     STempReflectionAndExternal, STempAllMedia): new messages
>>>     * netx/net/sourceforge/jnlp/resources/Messages_cs.properties
>>>     (STempPermNoFile, STempPermNoNetwork, STempPermNoExec,
>>>     STempNoFileOrNetwork, STempNoExecOrNetwork, STempNoFileOrExec,
>>>     STempNoFileOrNetworkOrExec, STempAllMedia, STempSoundOnly,
>>>     STempClipboardOnly, STempPrintOnly, STempAllFileAndPropertyAccess,
>>>     STempReadLocalFilesAndProperties, STempReflectionOnly): removed
>>>     *
>>> netx/net/sourceforge/jnlp/security/dialogs/TemporaryPermissions.java:
>>>     rewrite to use reflection to generate permissions from
>>>     PolicyEditorPermissions rather than exposing statically defined values
>>>     *
>>> netx/net/sourceforge/jnlp/security/dialogs/TemporaryPermissionsButton.java:
>>>     refactor to use new TemporaryPermissions change
>>>     * tests/netx/unit/net/sourceforge/jnlp/security/dialogs/TemporaryPermissionsTest.java:
>>>     new tests for TemporaryPermissions
>>>
>>>
>>> Thanks,
>>>
>> Lightning-quick update thanks to Jie Kang's keen eye: now using multi-catch
>> in TemporaryPermissions.getPermission. Semantically the same and no other
>> changes.
>>
> Hi Andrew,
>
> I agree, reflection is preferable over an ever growing list of static
> permission objects.
>
> Right now the code seems to be boxed such that the reflected object
> creation is always done dynamically (via the new getPermission method).
> Is there is a chance that this could later be utilized with user input?
>
> In its current static state it is fine, but my concern is that there is
> no arg sanitization when we try to create permission objects with the
> given string args in getPermission. In addition to potential bugs, there
> could also be security implications caused by incorrect permission
> creation/granting.
>
> Rest looks fine to me.
>
> Cheers,
> Deepak

I don't anticipate this particular code being used to accept user input, 
although I suppose it would be possible. For me at least the purpose of 
these temporary permissions is just to give users a faster way to 
quickly test applets with various restricted permission configurations, 
rather than having to use PolicyEditor or policytool, which is a bit 
more involved. If the user needs a custom-defined permission, then 
PolicyEditor allows this through CustomPolicyViewer.

As it is, the PolicyEditorPermissions argument is pretty restrictive 
already. It's an enum with two String fields providing simple 
human-readable descriptions, and three more enum fields that model the 
permission type (eg java.io.FilePermission), target (eg "${user.home}"), 
and actions (eg "read"). So there isn't any actual sanitization on the 
input, but it's just because the input is restricted to come from these 
pre-defined enum values anyway.

Thanks,

-- 
Andrew A