Bug 42390 - Missing Security Manager checks in classpath apis
Missing Security Manager checks in classpath apis
Status: RESOLVED FIXED
Product: classpath
Classification: Unclassified
Component: classpath
0.98
: P3 critical
: 0.99
Assigned To: Andrew John Hughes
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-16 04:02 UTC by varun srivastava
Modified: 2012-10-26 07:33 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-11-22 01:10:35


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description varun srivastava 2009-12-16 04:02:16 UTC
1)  Constructor missed the sm.checkPermission(SUBCLASS_IMPLEMENTATION_PERMISSION) check in the java.io.ObjectOutputStream: void <init>(java.io.OutputStream) constructor call.
2) 
Method calls :<java.util.logging.LogManager: void removePropertyChangeListener(java.beans.PropertyChangeListener)>

and 
<java.util.logging.LogManager: void addPropertyChangeListener(java.beans.PropertyChangeListener)> 
misses LoggingPermission"control" check
3) Mehtod call :<java.io.File: boolean isHidden()>  missing the check for checkRead()
4) Mehtod call :<java.security.ProtectionDomain: java.lang.String toString()> missing sm.checkPermission(SecurityConstants.GET_POLICY_PERMISSION) for the dynamic policy permission load.
5) 
Mehtod call :<java.net.Socket: void connect(java.net.SocketAddress)>
and  :<java.net.Socket: void connect(java.net.SocketAddress,int)> missing checkConnect.
6)  Method <java.net.DatagramSocket: void connect(java.net.SocketAddress)> should perform checkListen, checkMulticast, checkAccept on top of checkConnect
Comment 1 varun srivastava 2010-01-14 18:07:47 UTC
(In reply to comment #0)
> 1)  Constructor missed the
> sm.checkPermission(SUBCLASS_IMPLEMENTATION_PERMISSION) check in the
> java.io.ObjectOutputStream: void <init>(java.io.OutputStream) constructor call.
> 2) 
> Method calls :<java.util.logging.LogManager: void
> removePropertyChangeListener(java.beans.PropertyChangeListener)>
> 
> and 
> <java.util.logging.LogManager: void
> addPropertyChangeListener(java.beans.PropertyChangeListener)> 
> misses LoggingPermission"control" check
> 3) Mehtod call :<java.io.File: boolean isHidden()>  missing the check for
> checkRead()
> 4) Mehtod call :<java.security.ProtectionDomain: java.lang.String toString()>
> missing sm.checkPermission(SecurityConstants.GET_POLICY_PERMISSION) for the
> dynamic policy permission load.
> 5) 
> Mehtod call :<java.net.Socket: void connect(java.net.SocketAddress)>
> and  :<java.net.Socket: void connect(java.net.SocketAddress,int)> missing
> checkConnect.
> 6)  Method <java.net.DatagramSocket: void connect(java.net.SocketAddress)>
> should perform checkListen, checkMulticast, checkAccept on top of checkConnect
> 
Adding some more inconsistencies in Classpath as compared to other JVMs
7)In "public static final String getDefaultType()" API of java.security.KeyStore 
 the caller requires to have permission to read "keystore.type" property whereas in SUN JVM and Harmony JVM the permission is not required.
8) In "public static SelectorProvider provider()" API of java.nio.channels.spi.SelectorProvider the caller requires permission to load "java.nio.channels.spi.SelectorProvider" property, whereas in SUN and Harmony JVMs this permission is not required.
Comment 2 Andrew John Hughes 2010-11-22 01:10:35 UTC
I can confirm 1, 2 and 3 are missing.

4 is an odd one.  Via a Mauve test, I've confirmed that OpenJDK does perform the
security check but Classpath doesn't.  However, the official
documentation for toString() says nothing about the security exception
in this case.  Examining the Classpath code, it seems an entry point was
specifically adding to the Policy class to allow PermissionDomain to
obtain the policy without the security check.  So the fix is
simple, but documentation upstream also needs to be fixed IMHO.

5 & 6 are similar.  I haven't yet had time to check OpenJDK with these
two, but I can see that the required calls are missing from the
Classpath code.  Again, the security checks aren't documented at all
for Socket and DatagramSocket; it just says
'"SecurityException - if the caller is not allowed to send datagrams
to and receive datagrams from the address and port."
without specifying the actual checks.

The two inconsistencies, 7 & 8, seem to be a case of
System.getProperty being used directly, whereas internal code should
use gnu.classpath.SystemProperties directly.  I'll fix these in GNU Classpath once I've confirmed the last four on OpenJDK.
Comment 3 Andrew John Hughes 2010-12-03 00:51:37 UTC
Working on a fix for these.  I found PR46773 while working on these too.
Comment 4 Andrew John Hughes 2010-12-25 18:39:32 UTC
CVSROOT:        /sources/classpath
Module name:    classpath
Changes by:     Andrew John Hughes <gnu_andrew> 10/12/25 16:53:15

Modified files:
       .              : ChangeLog
       java/io        : ObjectOutputStream.java

Log message:
       PR classpath/42390: Add security check to ObjectOutputStream(OutputStream) constructor.

       2010-12-25  Andrew John Hughes  <ahughes@redhat.com>

               PR classpath/42390
               * java/io/ObjectOutputStream.java:
               (ObjectOutputStream(OutputStream)): Add
               required security check.
               (overridesMethods(Class<?>)): Check whether
               the subclass overrides one of the methods
               which requires a security check.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/classpath/ChangeLog?cvsroot=classpath&r1=1.9807&r2=1.9808
http://cvs.savannah.gnu.org/viewcvs/classpath/java/io/ObjectOutputStream.java?cvsroot=classpath&r1=1.74&r2=1.75
Comment 5 Andrew John Hughes 2010-12-25 18:40:08 UTC
CVSROOT:        /sources/classpath
Module name:    classpath
Changes by:     Andrew John Hughes <gnu_andrew> 10/12/25 18:38:51

Modified files:
       .              : ChangeLog
       java/util/logging: LogManager.java

Log message:
       PR classpath/42390: Throw SecurityException when adding and removing LogManager PropertyChangeListeners.

       2010-12-25  Andrew John Hughes  <ahughes@redhat.com>

               PR classpath/42390
               * java/util/logging/LogManager.java:
               (addPropertyChangeListener(PropertyChangeListener)):
               Document fully.  Throw NPE in a clearer way.  Add
               SecurityException.
               (removePropertyChangeListener(PropertyChangeListener)):
               Document fully. Add SecurityException.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/classpath/ChangeLog?cvsroot=classpath&r1=1.9808&r2=1.9809
http://cvs.savannah.gnu.org/viewcvs/classpath/java/util/logging/LogManager.java?cvsroot=classpath&r1=1.29&r2=1.30
Comment 6 Andrew John Hughes 2010-12-25 19:51:56 UTC
CVSROOT:        /sources/classpath
Module name:    classpath
Changes by:     Andrew John Hughes <gnu_andrew> 10/12/25 19:51:24

Modified files:
       .              : ChangeLog
       java/io        : File.java

Log message:
       PR classpath/42390: Add and document missing security check in java.io.File#isHidden().

       2010-12-25  Andrew John Hughes  <ahughes@redhat.com>

               PR classpath/42390
               * java/io/File.java:
               (isHidden()): Add and document missing
               security check.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/classpath/ChangeLog?cvsroot=classpath&r1=1.9809&r2=1.9810
http://cvs.savannah.gnu.org/viewcvs/classpath/java/io/File.java?cvsroot=classpath&r1=1.75&r2=1.76
Comment 7 Andrew John Hughes 2011-03-06 19:03:43 UTC
CVSROOT:        /sources/classpath
Module name:    classpath
Changes by:     Andrew John Hughes <gnu_andrew> 11/03/06 18:17:37

Modified files:
       .              : ChangeLog
       java/security  : ProtectionDomain.java

Log message:
       PR classpath/42390: Don't include permissions of the Policy in toString() output if reading them is prohibited.

       2011-02-22  Andrew John Hughes  <ahughes@redhat.com>

               PR classpath/42390
               * java/security/ProtectionDomain.java:
               (toString()): Don't include permissions from
               the policy if we don't have permission to read
               it.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/classpath/ChangeLog?cvsroot=classpath&r1=1.9819&r2=1.9820
http://cvs.savannah.gnu.org/viewcvs/classpath/java/security/ProtectionDomain.java?cvsroot=classpath&r1=1.18&r2=1.19
Comment 8 Andrew John Hughes 2011-03-15 23:49:03 UTC
CVSROOT:        /sources/classpath
Module name:    classpath
Changes by:     Andrew John Hughes <gnu_andrew> 11/03/15 23:02:01

Modified files:
       .              : ChangeLog
       java/net       : Socket.java

Log message:
       PR42390: Add missing call to SecurityManager.checkConnect in connect(SocketAddress, int).

       2011-03-14  Andrew John Hughes  <ahughes@redhat.com>

               PR classpath/42390
               * java/net/Socket.java:
               (connect(SocketAddress, int)): Add missing call
               to SecurityManager.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/classpath/ChangeLog?cvsroot=classpath&r1=1.9822&r2=1.9823
http://cvs.savannah.gnu.org/viewcvs/classpath/java/net/Socket.java?cvsroot=classpath&r1=1.65&r2=1.66
Comment 9 Andrew John Hughes 2012-02-08 19:09:54 UTC
CVSROOT:        /sources/classpath
Module name:    classpath
Changes by:     Andrew John Hughes <gnu_andrew> 12/02/08 14:47:57

Modified files:
       .              : ChangeLog
       java/net       : DatagramSocket.java

Log message:
       PR classpath/42390: Add missing security checks in DatagramSocket.connect.

       2012-02-06  Andrew John Hughes  <ahughes@redhat.com>

               PR classpath/42390
               * java/net/DatagramSocket.java:
               (connect(InetAddress,int)): Add missing security
               checks which OpenJDK performs and we don't.  It's
               possible to initialise a DatagramSocket with null
               so we should also ensure we are bound.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/classpath/ChangeLog?cvsroot=classpath&r1=1.9840&r2=1.9841
http://cvs.savannah.gnu.org/viewcvs/classpath/java/net/DatagramSocket.java?cvsroot=classpath&r1=1.52&r2=1.53
Comment 10 Andrew John Hughes 2012-02-08 19:48:09 UTC
CVSROOT:        /sources/classpath
Module name:    classpath
Changes by:     Andrew John Hughes <gnu_andrew> 12/02/08 19:09:38

Modified files:
       .              : ChangeLog
       java/nio/channels/spi: SelectorProvider.java
       java/security  : KeyStore.java

Log message:
       PR classpath/42390: Use AccessController and PrivilegedAction to retrieve system properties.

       2012-02-08  Andrew John Hughes  <ahughes@redhat.com>

               PR classpath/42390
               * java/nio/channels/spi/SelectorProvider.java:
               (provider()): Retrieve property value using
               PrivilegedAction.
               * java/security/KeyStore.java:
               (getDefaultType()): Likewise.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/classpath/ChangeLog?cvsroot=classpath&r1=1.9841&r2=1.9842
http://cvs.savannah.gnu.org/viewcvs/classpath/java/nio/channels/spi/SelectorProvider.java?cvsroot=classpath&r1=1.14&r2=1.15
http://cvs.savannah.gnu.org/viewcvs/classpath/java/security/KeyStore.java?cvsroot=classpath&r1=1.16&r2=1.17
Comment 11 Andrew John Hughes 2012-02-08 19:48:30 UTC
All fixed.
Comment 12 Andrew John Hughes 2012-10-26 07:33:23 UTC
Don't know why this is still open.