<AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Dmitry Markov
Hello,

Could you review a fix for jdk10, please?

 bug: https://bugs.openjdk.java.net/browse/JDK-8154405
 webrev: http://cr.openjdk.java.net/~dmarkov/8154405/webrev.00/

Problem description:
The current implementation of Toolkit.getImage(URL url) and Toolkit.createImage(URL url) uses URLPermission to perform security checks. However the documentation says that security manager’s method checkPermission() is called with url.openConnection().getPermission() permission and url.openConnection().getPermission() returns SocketPermission. So there is an inconsistency between the documentation and the implementation.

Fix:
It is necessary to update the description of the methods to get rid of confusing statements.

Thanks,
Dmitry
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Sergey Bylokhov
Looks fine.

On 17/11/2017 06:38, Dmitry Markov wrote:

> Hello,
>
> Could you review a fix for jdk10, please?
>
>   bug: https://bugs.openjdk.java.net/browse/JDK-8154405
>   webrev: http://cr.openjdk.java.net/~dmarkov/8154405/webrev.00/
>
> Problem description:
> The current implementation of Toolkit.getImage(URL url) and
> Toolkit.createImage(URL url) uses URLPermission to perform security
> checks. However the documentation says that security manager’s method
> checkPermission() is called with url.openConnection().getPermission()
> permission and url.openConnection().getPermission() returns
> SocketPermission. So there is an inconsistency between the documentation
> and the implementation.
>
> Fix:
> It is necessary to update the description of the methods to get rid of
> confusing statements.
>
> Thanks,
> Dmitry


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Dmitry Markov
Thank you, Sergey! Shall I create a CSR for this?

Thanks,
Dmitry

> On 17 Nov 2017, at 19:34, Sergey Bylokhov <[hidden email]> wrote:
>
> Looks fine.
>
> On 17/11/2017 06:38, Dmitry Markov wrote:
>> Hello,
>> Could you review a fix for jdk10, please?
>>  bug: https://bugs.openjdk.java.net/browse/JDK-8154405
>>  webrev: http://cr.openjdk.java.net/~dmarkov/8154405/webrev.00/
>> Problem description:
>> The current implementation of Toolkit.getImage(URL url) and Toolkit.createImage(URL url) uses URLPermission to perform security checks. However the documentation says that security manager’s method checkPermission() is called with url.openConnection().getPermission() permission and url.openConnection().getPermission() returns SocketPermission. So there is an inconsistency between the documentation and the implementation.
>> Fix:
>> It is necessary to update the description of the methods to get rid of confusing statements.
>> Thanks,
>> Dmitry
>
>
> --
> Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

semyon.sadetsky
In reply to this post by Dmitry Markov

+1

--Semyon


On 11/17/17 6:38 AM, Dmitry Markov wrote:
Hello,

Could you review a fix for jdk10, please?

 bug: https://bugs.openjdk.java.net/browse/JDK-8154405
 webrev: http://cr.openjdk.java.net/~dmarkov/8154405/webrev.00/

Problem description:
The current implementation of Toolkit.getImage(URL url) and Toolkit.createImage(URL url) uses URLPermission to perform security checks. However the documentation says that security manager’s method checkPermission() is called with url.openConnection().getPermission() permission and url.openConnection().getPermission() returns SocketPermission. So there is an inconsistency between the documentation and the implementation.

Fix:
It is necessary to update the description of the methods to get rid of confusing statements.

Thanks,
Dmitry

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Sergey Bylokhov
In reply to this post by Dmitry Markov
On 17/11/2017 12:28, Dmitry Markov wrote:
> Thank you, Sergey! Shall I create a CSR for this?

yes we need a CSR.



--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Dmitry Markov
I have created the following one https://bugs.openjdk.java.net/browse/JDK-8191531

Thanks,
Dmitry
On 17 Nov 2017, at 22:10, Sergey Bylokhov <[hidden email]> wrote:

On 17/11/2017 12:28, Dmitry Markov wrote:
Thank you, Sergey! Shall I create a CSR for this?

yes we need a CSR.



--
Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Dmitry Markov
During the CSR review it was decided to update proposed fix. The new version is located at http://cr.openjdk.java.net/~dmarkov/8154405/webrev.01/
Could you review the new version, please?

The list of changes:
- Updated the description of Toolkit.getImage(URL u) and Toolkit.createImage(URL u) (made the wording less specific)
- Added some backward compatibility support to SunToolkit.checkPermission() and to the constructor of URLImageSource. Now if security check of URLPermission is failed we will check the corresponding SocketPermission.
- Added regression test.

Thanks,
Dmitry 

On 18 Nov 2017, at 15:30, Dmitry Markov <[hidden email]> wrote:

I have created the following one https://bugs.openjdk.java.net/browse/JDK-8191531

Thanks,
Dmitry
On 17 Nov 2017, at 22:10, Sergey Bylokhov <[hidden email]> wrote:

On 17/11/2017 12:28, Dmitry Markov wrote:
Thank you, Sergey! Shall I create a CSR for this?

yes we need a CSR.



--
Best regards, Sergey.


Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Dmitry Markov
Reminder. Could you take look, please?

Also I would like to clarify the purpose of the fallback mechanism introduced by the new version. The fallback code addresses the issue that users have not knowing what permission to grant because some connections, (e.g. HTTP) may be established by granting either URLPermission or SocketPermission and it is unclear what permission type is used for check by getImage() or createImage(). In fact this code fixes backward compatibility issue caused by switching from SocketPermission to URLPermission. 

Thanks,
Dmitry

On 1 Dec 2017, at 18:07, Dmitry Markov <[hidden email]> wrote:

During the CSR review it was decided to update proposed fix. The new version is located at http://cr.openjdk.java.net/~dmarkov/8154405/webrev.01/
Could you review the new version, please?

The list of changes:
- Updated the description of Toolkit.getImage(URL u) and Toolkit.createImage(URL u) (made the wording less specific)
- Added some backward compatibility support to SunToolkit.checkPermission() and to the constructor of URLImageSource. Now if security check of URLPermission is failed we will check the corresponding SocketPermission.
- Added regression test.

Thanks,
Dmitry 

On 18 Nov 2017, at 15:30, Dmitry Markov <[hidden email]> wrote:

I have created the following one https://bugs.openjdk.java.net/browse/JDK-8191531

Thanks,
Dmitry
On 17 Nov 2017, at 22:10, Sergey Bylokhov <[hidden email]> wrote:

On 17/11/2017 12:28, Dmitry Markov wrote:
Thank you, Sergey! Shall I create a CSR for this?

yes we need a CSR.



--
Best regards, Sergey.



Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Dmitry Markov
Please ignore the new version of the fix, (i.e. http://cr.openjdk.java.net/~dmarkov/8154405/webrev.01/). It was found out that the usage of fallback code introduces a potential security issue. So I will integrate the previous version of the fix, (i.e. http://cr.openjdk.java.net/~dmarkov/8154405/webrev.00/) which is already approved on this list.

Sean,

Thank you in advance,
Dmitry

On 8 Dec 2017, at 11:19, Dmitry Markov <[hidden email]> wrote:

Reminder. Could you take look, please?

Also I would like to clarify the purpose of the fallback mechanism introduced by the new version. The fallback code addresses the issue that users have not knowing what permission to grant because some connections, (e.g. HTTP) may be established by granting either URLPermission or SocketPermission and it is unclear what permission type is used for check by getImage() or createImage(). In fact this code fixes backward compatibility issue caused by switching from SocketPermission to URLPermission. 

Thanks,
Dmitry

On 1 Dec 2017, at 18:07, Dmitry Markov <[hidden email]> wrote:

During the CSR review it was decided to update proposed fix. The new version is located at http://cr.openjdk.java.net/~dmarkov/8154405/webrev.01/
Could you review the new version, please?

The list of changes:
- Updated the description of Toolkit.getImage(URL u) and Toolkit.createImage(URL u) (made the wording less specific)
- Added some backward compatibility support to SunToolkit.checkPermission() and to the constructor of URLImageSource. Now if security check of URLPermission is failed we will check the corresponding SocketPermission.
- Added regression test.

Thanks,
Dmitry 

On 18 Nov 2017, at 15:30, Dmitry Markov <[hidden email]> wrote:

I have created the following one https://bugs.openjdk.java.net/browse/JDK-8191531

Thanks,
Dmitry
On 17 Nov 2017, at 22:10, Sergey Bylokhov <[hidden email]> wrote:

On 17/11/2017 12:28, Dmitry Markov wrote:
Thank you, Sergey! Shall I create a CSR for this?

yes we need a CSR.



--
Best regards, Sergey.




Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8154405: AccessControlException by URLPermission check

Sean Mullan
On 12/13/17 2:38 AM, Dmitry Markov wrote:
> Sean,
> Could you take a look at
> http://cr.openjdk.java.net/~dmarkov/8154405/webrev.00/ 
> <http://cr.openjdk.java.net/%7Edmarkov/8154405/webrev.00/> , please?

Looks good.

Thanks,
Sean