You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/01/29 11:30:03 UTC
[GitHub] [commons-io] boris-unckel opened a new pull request #197: [IO-712] SecurityExceptions are hidden instead of breaking the regular …
boris-unckel opened a new pull request #197:
URL: https://github.com/apache/commons-io/pull/197
…flow
Fixes https://issues.apache.org/jira/browse/IO-712
SecurityExceptions are rethrown now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel commented on a change in pull request #197: [IO-712] SecurityExceptions are hidden instead of breaking the regular …
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on a change in pull request #197:
URL: https://github.com/apache/commons-io/pull/197#discussion_r567218271
##########
File path: src/main/java/org/apache/commons/io/FileSystem.java
##########
@@ -146,27 +146,16 @@ private static boolean getOsMatchesName(final String osNamePrefix) {
}
/**
- * <p>
- * Gets a System property, defaulting to {@code null} if the property cannot be read.
- * </p>
- * <p>
- * If a {@code SecurityException} is caught, the return value is {@code null} and a message is written to
- * {@code System.err}.
- * </p>
+ * Gets a System property, defaulting to {@code null} if the property cannot be read,
+ * except for {@code SecurityException}.
*
* @param property
* the system property name
- * @return the system property value or {@code null} if a security problem occurs
+ * @return the system property value or {@code null} if a problem occurs
+ * @throws SecurityException if underlying calls fail due to missing permissions.
*/
private static String getSystemProperty(final String property) {
- try {
- return System.getProperty(property);
- } catch (final SecurityException ex) {
- // we are not allowed to look at this property
- System.err.println("Caught a SecurityException reading the system property '" + property
- + "'; the SystemUtils property value will default to null.");
- return null;
- }
+ return System.getProperty(property);
Review comment:
I have already commented on the use case, but I'll try again with a impact analysis of the change:
a) The absolute majority: The Java Security Manager is not active, before and after change zero impact.
b) The absolute minority: One has the ability to upgrade the library, but not to change the permissions.xml or xyz.policy file. But the security manager is in use. This is very unlikely but in this case you're stuck on the old version. People activating the JSM without ability to grant permissions?
c) The happy case: One has the ability to upgrade the library and to grant permissions. With the clear message what is is missing one can grant the single permission to the updated single library.
d) JavaEE users will have no impact with the container itself, permissions are granted by the container. For their apps, they utilize not single permissions, but permissions.xml for the whole app. Means the grant is for all classes in the jar, not only for a single jar. In example Spring requires a * [PropertyPermission](https://docs.oracle.com/javase/8/docs/technotes/guides/security/permissions.html#PropertyPermission) calling [System.getProperties](https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#getProperties--).
If a user does need to read a property without grant there is a way: [https://docs.oracle.com/javase/8/docs/technotes/guides/security/doprivileged.html](https://docs.oracle.com/javase/8/docs/technotes/guides/security/doprivileged.html)
This way should never be used in a utility method. See chapters "Least Privilege", "More Privilege", "Reflection".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a change in pull request #197: [IO-712] SecurityExceptions are hidden instead of breaking the regular …
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #197:
URL: https://github.com/apache/commons-io/pull/197#discussion_r567153417
##########
File path: src/main/java/org/apache/commons/io/FileSystem.java
##########
@@ -146,27 +146,16 @@ private static boolean getOsMatchesName(final String osNamePrefix) {
}
/**
- * <p>
- * Gets a System property, defaulting to {@code null} if the property cannot be read.
- * </p>
- * <p>
- * If a {@code SecurityException} is caught, the return value is {@code null} and a message is written to
- * {@code System.err}.
- * </p>
+ * Gets a System property, defaulting to {@code null} if the property cannot be read,
+ * except for {@code SecurityException}.
*
* @param property
* the system property name
- * @return the system property value or {@code null} if a security problem occurs
+ * @return the system property value or {@code null} if a problem occurs
+ * @throws SecurityException if underlying calls fail due to missing permissions.
*/
private static String getSystemProperty(final String property) {
- try {
- return System.getProperty(property);
- } catch (final SecurityException ex) {
- // we are not allowed to look at this property
- System.err.println("Caught a SecurityException reading the system property '" + property
- + "'; the SystemUtils property value will default to null.");
- return null;
- }
+ return System.getProperty(property);
Review comment:
This is a special case. The whole purpose of this API is to never blow up. I am not sure how useful this method really is in practice though. If you are running under JAAS and this call returns null because you can't read a sys prop and the rest of your app runs (or not), then you must have a very specific set of JAAS permissions set. But... I can see that one might want to write out or log sys props on start up, and then, writing 'null' instead of blowing up provides... what? I'm not sure. All of this rambling to say that in this case we should not change the behavior IMO.
Writing to the console is a no-no so we should get rid of that for sure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel closed pull request #197: [IO-712] SecurityExceptions are hidden instead of breaking the regular …
Posted by GitBox <gi...@apache.org>.
boris-unckel closed pull request #197:
URL: https://github.com/apache/commons-io/pull/197
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel commented on a change in pull request #197: [IO-712] SecurityExceptions are hidden instead of breaking the regular …
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on a change in pull request #197:
URL: https://github.com/apache/commons-io/pull/197#discussion_r567218271
##########
File path: src/main/java/org/apache/commons/io/FileSystem.java
##########
@@ -146,27 +146,16 @@ private static boolean getOsMatchesName(final String osNamePrefix) {
}
/**
- * <p>
- * Gets a System property, defaulting to {@code null} if the property cannot be read.
- * </p>
- * <p>
- * If a {@code SecurityException} is caught, the return value is {@code null} and a message is written to
- * {@code System.err}.
- * </p>
+ * Gets a System property, defaulting to {@code null} if the property cannot be read,
+ * except for {@code SecurityException}.
*
* @param property
* the system property name
- * @return the system property value or {@code null} if a security problem occurs
+ * @return the system property value or {@code null} if a problem occurs
+ * @throws SecurityException if underlying calls fail due to missing permissions.
*/
private static String getSystemProperty(final String property) {
- try {
- return System.getProperty(property);
- } catch (final SecurityException ex) {
- // we are not allowed to look at this property
- System.err.println("Caught a SecurityException reading the system property '" + property
- + "'; the SystemUtils property value will default to null.");
- return null;
- }
+ return System.getProperty(property);
Review comment:
I have already commented on the use case, but I'll try again with a impact analysis of the change:
a) The absolute majority: The Java Security Manager is not active, before and after change zero impact.
b) The absolute minority: One has the ability to upgrade the library, but not to change the permissions.xml or xyz.policy file. But the security manager is in use. This is very unlikely but in this case you're stuck on the old version. People activating the JSM without ability to grant permissions?
c) The happy case: One has the ability to upgrade the library and to grant permissions. With the clear message what is is missing one can grant the single permission to the updated single library.
d) JavaEE users will have no impact with the container itself, permissions are granted by the container. For their apps, they utilize not single permissions, but permissions.xml for the whole app. Means the grant is for all classes in the ear or war, not only for a single jar. In example Spring requires a * [PropertyPermission](https://docs.oracle.com/javase/8/docs/technotes/guides/security/permissions.html#PropertyPermission) calling [System.getProperties](https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#getProperties--).
If a user does need to read a property without grant there is a way: [https://docs.oracle.com/javase/8/docs/technotes/guides/security/doprivileged.html](https://docs.oracle.com/javase/8/docs/technotes/guides/security/doprivileged.html)
This way should never be used in a utility method. See chapters "Least Privilege", "More Privilege", "Reflection".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls commented on pull request #197: [IO-712] SecurityExceptions are hidden instead of breaking the regular …
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #197:
URL: https://github.com/apache/commons-io/pull/197#issuecomment-770038726
[](https://coveralls.io/builds/36710086)
Coverage decreased (-0.2%) to 88.897% when pulling **87035a502b2ce567ffb03422249f2440b6e91c77 on boris-unckel:security_exception** into **0852cfe3030cc861f0c0988ebc9319c7c787d767 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #197: [IO-712] SecurityExceptions are hidden instead of breaking the regular …
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #197:
URL: https://github.com/apache/commons-io/pull/197#issuecomment-922520425
I still don't buy it, just don't use the API if its POV is so wrong for your use case. If anything, we can better Javadoc this method. What do others think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org