[RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello, please review this small change that improve exception messages during manifest parsing of jar archives . Thanks, Matthias Bug : https://bugs.openjdk.java.net/browse/JDK-8205525 Webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525/
On 25/06/2018 14:55, Baesken, Matthias wrote:
Hello, please review this small change that improve exception messages during manifest parsing of jar archives .
Thanks, Matthias
Bug :
https://bugs.openjdk.java.net/browse/JDK-8205525
Webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525/ This potentially leaks sensitive information and will require a security review. There was a similar discussion on net-dev recently related to leaking host names in exceptions. Something similar may be needed here.
-Alan.
Hi, do you consider both the file name and line number as sensitive ?
There was a similar discussion on net-dev recently related to leaking host names in exceptions. Something similar may be needed here
Do you know the outcome of this discussion ? Best regards, Matthias
-----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Montag, 25. Juni 2018 16:17 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 25/06/2018 14:55, Baesken, Matthias wrote:
Hello, please review this small change that improve exception messages during manifest parsing of jar archives .
Thanks, Matthias
Bug :
https://bugs.openjdk.java.net/browse/JDK-8205525
Webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525/ This potentially leaks sensitive information and will require a security review. There was a similar discussion on net-dev recently related to leaking host names in exceptions. Something similar may be needed here.
-Alan.
On 25/06/2018 15:29, Baesken, Matthias wrote:
Hi, do you consider both the file name and line number as sensitive ?
There was a similar discussion on net-dev recently related to leaking host names in exceptions. Something similar may be needed here
Do you know the outcome of this discussion ?
All the details are in JDK-8204233 and the associated CSR. -Alan
Hi Alan ,so it looks like JDK-8204233 added a switch (system property) to enable the enhanced socket IOException messages . That would be an option as well for 8205525 . 8205525 adds the jar file name and the line number info to the exception message . In case that only the jar file name would be considered sensitive , I would prefer to just output the line number (and omit the system property ). What do you think ? Best regards, Matthias
-----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Montag, 25. Juni 2018 16:52 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Cc: Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 25/06/2018 15:29, Baesken, Matthias wrote:
Hi, do you consider both the file name and line number as sensitive ?
There was a similar discussion on net-dev recently related to leaking host names in exceptions. Something similar may be needed here
Do you know the outcome of this discussion ?
All the details are in JDK-8204233 and the associated CSR.
-Alan
Hi Matthias, I am not a reviewer and neither do I have enough knowledge about whether jar/file _names_ are considered security sensitive. However, the patch that's proposed for this change, prints the file _path_ (and not just the name). That I believe is security sensitive. -Jaikiran On 06/07/18 6:14 PM, Baesken, Matthias wrote:
Hi Alan ,so it looks like JDK-8204233 added a switch (system property) to enable the enhanced socket IOException messages . That would be an option as well for 8205525 . 8205525 adds the jar file name and the line number info to the exception message . In case that only the jar file name would be considered sensitive , I would prefer to just output the line number (and omit the system property ). What do you think ? Best regards, Matthias
-----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Montag, 25. Juni 2018 16:52 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Cc: Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives On 25/06/2018 15:29, Baesken, Matthias wrote:
Hi, do you consider both the file name and line number as sensitive ?
There was a similar discussion on net-dev recently related to leaking host names in exceptions. Something similar may be needed here Do you know the outcome of this discussion ? All the details are in JDK-8204233 and the associated CSR. -Alan
On 06/07/2018 13:44, Baesken, Matthias wrote:
Hi Alan ,so it looks like JDK-8204233 added a switch (system property) to enable the enhanced socket IOException messages .
That would be an option as well for 8205525 . Yes, it's documented in conf/security/java.security and something equivalent could be done here. The giveaway in your original patch is that it needed a privileged block to create the exception message.
8205525 adds the jar file name and the line number info to the exception message .
In case that only the jar file name would be considered sensitive , I would prefer to just output the line number (and omit the system property ).
That should be okay (I can't think of any concerns). -Alan
Hi Alan, thanks for commenting on this . Jaikiran mentioned that printing just the jar file name and not file with path might be okay :
I am not a reviewer and neither do I have enough knowledge about whether jar/file _names_ are considered security sensitive. However, the patch that's proposed for this change, prints the file _path_ (and not just the name). That I believe is security sensitive.
What do you think ? Best regards, Matthias
-----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Sonntag, 8. Juli 2018 09:36 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Cc: Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 06/07/2018 13:44, Baesken, Matthias wrote:
Hi Alan ,so it looks like JDK-8204233 added a switch (system property) to enable the enhanced socket IOException messages .
That would be an option as well for 8205525 . Yes, it's documented in conf/security/java.security and something equivalent could be done here. The giveaway in your original patch is that it needed a privileged block to create the exception message.
8205525 adds the jar file name and the line number info to the
exception message .
In case that only the jar file name would be considered sensitive , I would
prefer to just output the line number (and omit the system property ).
That should be okay (I can't think of any concerns).
-Alan
On 10/07/2018 10:53, Baesken, Matthias wrote:
Hi Alan, thanks for commenting on this .
Jaikiran mentioned that printing just the jar file name and not file with path might be okay :
I am not a reviewer and neither do I have enough knowledge about whether jar/file _names_ are considered security sensitive. However, the patch that's proposed for this change, prints the file _path_ (and not just the name). That I believe is security sensitive. What do you think ?
In the ZipFile API, the "name" may include path information but if you strip that and include just the file name then it should be okay. A useful way to think about is the information revealed when a HTTP response serves up a tasty stack trace. -Alan.
Hello, after latest comments from Alan and Jaikiran I created a new webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.2/ The jar file path is now printed in case jdk.includeInExceptions contains jarpath (this approach is "borrowed" from the enhanced socket exceptions ) . The line number is always printed . Best regards, Matthias
-----Original Message----- From: Baesken, Matthias Sent: Dienstag, 10. Juli 2018 11:53 To: 'Alan Bateman' <Alan.Bateman@oracle.com>; core-libs- dev@openjdk.java.net; 'jai.forums2013@gmail.com' <jai.forums2013@gmail.com> Cc: Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Alan, thanks for commenting on this .
Jaikiran mentioned that printing just the jar file name and not file with path might be okay :
I am not a reviewer and neither do I have enough knowledge about whether jar/file _names_ are considered security sensitive. However, the patch that's proposed for this change, prints the file _path_ (and not just the name). That I believe is security sensitive.
What do you think ?
Best regards, Matthias
-----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Sonntag, 8. Juli 2018 09:36 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Cc: Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 06/07/2018 13:44, Baesken, Matthias wrote:
Hi Alan ,so it looks like JDK-8204233 added a switch (system property) to enable the enhanced socket IOException messages .
That would be an option as well for 8205525 . Yes, it's documented in conf/security/java.security and something equivalent could be done here. The giveaway in your original patch is that it needed a privileged block to create the exception message.
8205525 adds the jar file name and the line number info to the
exception message .
In case that only the jar file name would be considered sensitive , I
would prefer to just output the line number (and omit the system property ).
That should be okay (I can't think of any concerns).
-Alan
On 16/07/2018 14:53, Baesken, Matthias wrote:
Hello, after latest comments from Alan and Jaikiran I created a new webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.2/
The jar file path is now printed in case jdk.includeInExceptions contains jarpath (this approach is "borrowed" from the enhanced socket exceptions ) . The line number is always printed .
The general approach seems okay and consistent with the agreement on how to reveal host names in socket exceptions. The name of the security/system property will need discussion as "jdk.includeInExceptions" is very general. If we have something general then we'll need a good name and replace the existing jdk.net.includeInExceptions. It might be better to go with something specific for the area such as "jdk.util.jar.includeInExceptions=jarpath" (to be consistent with other jdk.* properties in this code). A CSR will be needed for this too as the property will be documented in the java.security file. As regards the patch then it mostly looks okay but I think the changes in Attributes will need cleanup to get it consistent (esp. the line lengths) with the existing code. -Alan
The name of the security/system property will need discussion as "jdk.includeInExceptions" is very general. If we have something general then we'll need a good name and replace the existing jdk.net.includeInExceptions. It might be better to go with something specific for the area such as "jdk.util.jar.includeInExceptions=jarpath" (to be consistent with other jdk.* properties in this code). A CSR will be needed for this too as the property will be documented in the java.security file.
Hi Alan, I'll prepare a CSR . I selected a more general name "jdk.includeInExceptions" , because there is the idea to enhance more exceptions with additional output . In such a case " jdk.util.jar.includeInExceptions" would not really help . However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first . Best regards, Matthias
-----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Dienstag, 17. Juli 2018 13:39 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 16/07/2018 14:53, Baesken, Matthias wrote:
Hello, after latest comments from Alan and Jaikiran I created a new webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.2/
The jar file path is now printed in case jdk.includeInExceptions contains jarpath (this approach is "borrowed" from the enhanced socket exceptions ) . The line number is always printed .
The general approach seems okay and consistent with the agreement on how to reveal host names in socket exceptions.
The name of the security/system property will need discussion as "jdk.includeInExceptions" is very general. If we have something general then we'll need a good name and replace the existing jdk.net.includeInExceptions. It might be better to go with something specific for the area such as "jdk.util.jar.includeInExceptions=jarpath" (to be consistent with other jdk.* properties in this code). A CSR will be needed for this too as the property will be documented in the java.security file.
As regards the patch then it mostly looks okay but I think the changes in Attributes will need cleanup to get it consistent (esp. the line lengths) with the existing code.
-Alan
On 18/07/2018 09:21, Baesken, Matthias wrote:
Hi Alan, I'll prepare a CSR . I selected a more general name "jdk.includeInExceptions" , because there is the idea to enhance more exceptions with additional output . In such a case " jdk.util.jar.includeInExceptions" would not really help . However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first .
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. -Alan
Hello, in the meantime I prepared a CSR : https://bugs.openjdk.java.net/browse/JDK-8207768
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base.
Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ? Best regards, Matthias
-----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Mittwoch, 18. Juli 2018 19:44 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 18/07/2018 09:21, Baesken, Matthias wrote:
Hi Alan, I'll prepare a CSR . I selected a more general name "jdk.includeInExceptions" , because there is the idea to enhance more exceptions with additional output . In such a case " jdk.util.jar.includeInExceptions" would not really help . However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first .
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base.
-Alan
On 19 Jul 2018, at 09:07, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hello, in the meantime I prepared a CSR :
This CSR seems a little premature. Matthias said: "However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first “. Has this been checked? I have seen no update on this or any other email thread. JDK-8204233 [1] was a pragmatic and targeted solution to a long standing complaint. It was not intended, as is obvious by the newly added property name, to set precedent for doing similar all over the platform. If, after a broader discussion, this approach is to be applied to several different areas of the platform, then maybe JDK-8204233 should be revisited ( noting that it is JDK 11, which is currently in RDP 1 ). I think we should try to avoid a whole plethora of these properties. -Chris. [1] https://bugs.openjdk.java.net/browse/JDK-8204233
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base.
Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ?
Best regards, Matthias
-----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Mittwoch, 18. Juli 2018 19:44 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 18/07/2018 09:21, Baesken, Matthias wrote:
Hi Alan, I'll prepare a CSR . I selected a more general name "jdk.includeInExceptions" , because there is the idea to enhance more exceptions with additional output . In such a case " jdk.util.jar.includeInExceptions" would not really help . However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first .
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base.
-Alan
I agree with Chris. Unlike the connection failure error messages where the IP/port being part of the error message did add value, I don't think including the "path" to the jar (something like /opt/private/foo/bar/helloworld.jar) in case of MANIFEST parsing errors is really necessary. I think, just the filename (helloworld.jar) and the line number should be good enough in such failures. I have been trying to think in what cases the entire path would be necessary, to understand what's causing the parsing failure and address the issue. So far, I haven't been able to come up with such a use case. I do understand, as Alan already noted in this thread, the ZipFile APIs accept a file "path". So, directly using those paths in the error messages won't work out. Perhaps this changeset can be changed to do the necessary work to just get hold of the file name (while constructing the error message)? That way it wouldn't need any of these security configurations? -Jaikiran On 19/07/18 3:37 PM, Chris Hegarty wrote:
On 19 Jul 2018, at 09:07, Baesken, Matthias <matthias.baesken@sap.com> wrote: Hello, in the meantime I prepared a CSR : https://bugs.openjdk.java.net/browse/JDK-8207768 This CSR seems a little premature. Matthias said: "However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first “. Has this been checked? I have seen no update on this or any other email thread. JDK-8204233 [1] was a pragmatic and targeted solution to a long standing complaint. It was not intended, as is obvious by the newly added property name, to set precedent for doing similar all over the platform. If, after a broader discussion, this approach is to be applied to several different areas of the platform, then maybe JDK-8204233 should be revisited ( noting that it is JDK 11, which is currently in RDP 1 ). I think we should try to avoid a whole plethora of these properties. -Chris. [1] https://bugs.openjdk.java.net/browse/JDK-8204233
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ? Best regards, Matthias -----Original Message----- From: Alan Bateman [mailto:Alan.Bateman@oracle.com] Sent: Mittwoch, 18. Juli 2018 19:44 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives On 18/07/2018 09:21, Baesken, Matthias wrote:
Hi Alan, I'll prepare a CSR . I selected a more general name "jdk.includeInExceptions" , because there is the idea to enhance more exceptions with additional output . In such a case " jdk.util.jar.includeInExceptions" would not really help . However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first . jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. -Alan
On 19/07/2018 09:07, Baesken, Matthias wrote:
Hello, in the meantime I prepared a CSR :
https://bugs.openjdk.java.net/browse/JDK-8207768
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ?
Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that. It probably means we need the code to read this property moved to a jdk.internal package and change the networking, NIO, and JAR code to use that. It introduces a small compatibility issue with JDK 11 jdk.net property so that will need to be handled too. -Alan
On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 19/07/2018 09:07, Baesken, Matthias wrote:
Hello, in the meantime I prepared a CSR :
https://bugs.openjdk.java.net/browse/JDK-8207768
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ?
Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that.
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846 I would like to resolve 8207846 first, then 8205525 can propose to add the new argument value, `jarPath`. -Chris.
On 7/19/18 8:54 AM, Chris Hegarty wrote:
On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 19/07/2018 09:07, Baesken, Matthias wrote:
Hello, in the meantime I prepared a CSR :
https://bugs.openjdk.java.net/browse/JDK-8207768
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ?
Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that.
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I would like to resolve 8207846 first, then 8205525 can propose to add the new argument value, `jarPath`.
Note that making this a security property for all general cases may have performance implications in certain scenarios since the java.security file will need to be loaded and fully parsed before it can be used. If you are already parsing the java.security file for something else (which is probably the case for jdk.net.includeInExceptions), then it should be fine, but that may not always be the case if this is made into a more general property. Just something to think about ... --Sean
On 19 Jul 2018, at 15:42, Sean Mullan <sean.mullan@oracle.com> wrote:
...
Note that making this a security property for all general cases may have performance implications in certain scenarios since the java.security file will need to be loaded and fully parsed before it can be used. If you are already parsing the java.security file for something else (which is probably the case for jdk.net.includeInExceptions), then it should be fine, but that may not always be the case if this is made into a more general property. Just something to think about …
Good point Sean. My concern is more along the lines of a whole plethora of specific properties being added, rather than the implications of such. That should be scrutinised separately. What I am suggesting is to generalise the existing property so that it can, if applicable, be used in other areas. Not that it must be used in all other areas. The current proposal, to add a new property, already has this concern. What I am suggesting does not cause the file to be parsed any more than it would already. -Chris.
On 19 Jul 2018, at 13:54, Chris Hegarty <chris.hegarty@oracle.com> wrote: ....
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I sent out an RFR for 8207846, since I think it is worth proceeding with regardless of the outcome of 8205525. http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054497.html -Chris.
Hi Chris, thanks for starting the new RFR for to generalization of the `includeInExceptions` security property: Best regards, Matthias
-----Original Message----- From: Chris Hegarty [mailto:chris.hegarty@oracle.com] Sent: Freitag, 20. Juli 2018 13:41 To: Alan Bateman <Alan.Bateman@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 19 Jul 2018, at 13:54, Chris Hegarty <chris.hegarty@oracle.com> wrote: ....
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I sent out an RFR for 8207846, since I think it is worth proceeding with regardless of the outcome of 8205525.
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054497.html
-Chris.
Hello , looks like the generalization of the `includeInExceptions` security property is now in jdk/jdk after "8207846: Generalize the jdk.net.includeInExceptions security property" was added, great news and thanks to Chris for driving this ! I use this security property now as well , and updated the change : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.3/ I updated the CSR as well : https://bugs.openjdk.java.net/browse/JDK-8207768 Best regards, Matthias
-----Original Message----- From: Chris Hegarty <chris.hegarty@oracle.com> Sent: Donnerstag, 19. Juli 2018 14:54 To: Alan Bateman <Alan.Bateman@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 19/07/2018 09:07, Baesken, Matthias wrote:
Hello, in the meantime I prepared a CSR :
https://bugs.openjdk.java.net/browse/JDK-8207768
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ?
Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that.
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I would like to resolve 8207846 first, then 8205525 can propose to add the new argument value, `jarPath`.
-Chris.
Ping .... 😊 , any reviews / comments ? Thanks , Matthias
-----Original Message----- From: Baesken, Matthias Sent: Dienstag, 31. Juli 2018 12:28 To: 'Chris Hegarty' <chris.hegarty@oracle.com>; Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Langer, Christoph <christoph.langer@sap.com> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello , looks like the generalization of the `includeInExceptions` security property is now in jdk/jdk after
"8207846: Generalize the jdk.net.includeInExceptions security property"
was added, great news and thanks to Chris for driving this !
I use this security property now as well , and updated the change :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.3/
I updated the CSR as well :
https://bugs.openjdk.java.net/browse/JDK-8207768
Best regards, Matthias
-----Original Message----- From: Chris Hegarty <chris.hegarty@oracle.com> Sent: Donnerstag, 19. Juli 2018 14:54 To: Alan Bateman <Alan.Bateman@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 19/07/2018 09:07, Baesken, Matthias wrote:
Hello, in the meantime I prepared a CSR :
https://bugs.openjdk.java.net/browse/JDK-8207768
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ?
Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that.
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I would like to resolve 8207846 first, then 8205525 can propose to add the new argument value, `jarPath`.
-Chris.
Cross-posting to security-dev since this fix is security related and should also be reviewed there. --Sean On 8/7/18 11:00 AM, Baesken, Matthias wrote:
Ping .... 😊 , any reviews / comments ?
Thanks , Matthias
-----Original Message----- From: Baesken, Matthias Sent: Dienstag, 31. Juli 2018 12:28 To: 'Chris Hegarty' <chris.hegarty@oracle.com>; Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Langer, Christoph <christoph.langer@sap.com> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello , looks like the generalization of the `includeInExceptions` security property is now in jdk/jdk after
"8207846: Generalize the jdk.net.includeInExceptions security property"
was added, great news and thanks to Chris for driving this !
I use this security property now as well , and updated the change :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.3/
I updated the CSR as well :
https://bugs.openjdk.java.net/browse/JDK-8207768
Best regards, Matthias
-----Original Message----- From: Chris Hegarty <chris.hegarty@oracle.com> Sent: Donnerstag, 19. Juli 2018 14:54 To: Alan Bateman <Alan.Bateman@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 19/07/2018 09:07, Baesken, Matthias wrote:
Hello, in the meantime I prepared a CSR :
https://bugs.openjdk.java.net/browse/JDK-8207768
jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ?
Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that.
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I would like to resolve 8207846 first, then 8205525 can propose to add the new argument value, `jarPath`.
-Chris.
I need more time to finish my review but here are some initial comments: - java.security 1079 # jarpath - enables more detailed information in the IOExceptions thrown Use "jarPath" to be consistent with "hostInfo". 1080 # by java.util.jar.Attributes and java.util.jar.Manifest and java.util.jar.JarFile - Manifest.java 57 private String jarFilename = null; Not necessary to set it to null, as that is the default. 82 Manifest(InputStream is, String jarFilename) throws IOException { 83 this.jarFilename = jarFilename; 84 read(is); 85 } Read from the InputStream and check for error conditions *before* setting jarFilename (switch lines 83 & 84). This is a general best practice but can also protect against finalizer attacks. See JSCG 7-3 for more info: https://www.oracle.com/technetwork/java/seccodeguide-139067.html#7 - Attributes.java As Chris and Alan mentioned, you should move the parsing of the property to a more general location so it can be used by other code that uses this property. --Sean On 8/8/18 11:00 AM, Sean Mullan wrote:
Cross-posting to security-dev since this fix is security related and should also be reviewed there.
--Sean
On 8/7/18 11:00 AM, Baesken, Matthias wrote:
Ping .... 😊 , any reviews / comments ?
Thanks , Matthias
-----Original Message----- From: Baesken, Matthias Sent: Dienstag, 31. Juli 2018 12:28 To: 'Chris Hegarty' <chris.hegarty@oracle.com>; Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Langer, Christoph <christoph.langer@sap.com> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello , looks like the generalization of the `includeInExceptions` security property is now in jdk/jdk after
"8207846: Generalize the jdk.net.includeInExceptions security property"
was added, great news and thanks to Chris for driving this !
I use this security property now as well , and updated the change :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.3/
I updated the CSR as well :
https://bugs.openjdk.java.net/browse/JDK-8207768
Best regards, Matthias
-----Original Message----- From: Chris Hegarty <chris.hegarty@oracle.com> Sent: Donnerstag, 19. Juli 2018 14:54 To: Alan Bateman <Alan.Bateman@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 19/07/2018 09:07, Baesken, Matthias wrote:
Hello, in the meantime I prepared a CSR :
https://bugs.openjdk.java.net/browse/JDK-8207768
> jdk.includeInExceptions expands the scope. That might be okay but we > will need to re-visit jdk.net.includeInExceptions and also move the > support to somewhere in jdk.internal so that it can be used by other > code in java.base. Is there some support code for " jdk.net.includeInExceptions " or do you just mean the name of the property ?
Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that.
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I would like to resolve 8207846 first, then 8205525 can propose to add the new argument value, `jarPath`.
-Chris.
As Chris and Alan mentioned, you should move the parsing of the property to a more general location so it can be used by other code that uses this property.
Hi Sean, Thanks for the input and comments . Could we do the moving of the property parsing do in a followup CR, I would prefer this . Best regards, Matthias
-----Original Message----- From: Sean Mullan <sean.mullan@oracle.com> Sent: Freitag, 10. August 2018 17:39 To: Baesken, Matthias <matthias.baesken@sap.com>; Chris Hegarty <chris.hegarty@oracle.com>; Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
I need more time to finish my review but here are some initial comments:
- java.security
1079 # jarpath - enables more detailed information in the IOExceptions thrown
Use "jarPath" to be consistent with "hostInfo".
1080 # by java.util.jar.Attributes and java.util.jar.Manifest
and java.util.jar.JarFile
- Manifest.java
57 private String jarFilename = null;
Not necessary to set it to null, as that is the default.
82 Manifest(InputStream is, String jarFilename) throws IOException { 83 this.jarFilename = jarFilename; 84 read(is); 85 }
Read from the InputStream and check for error conditions *before* setting jarFilename (switch lines 83 & 84). This is a general best practice but can also protect against finalizer attacks. See JSCG 7-3 for more info: https://www.oracle.com/technetwork/java/seccodeguide-139067.html#7
- Attributes.java
As Chris and Alan mentioned, you should move the parsing of the property to a more general location so it can be used by other code that uses this property.
--Sean
On 8/8/18 11:00 AM, Sean Mullan wrote:
Cross-posting to security-dev since this fix is security related and should also be reviewed there.
--Sean
On 8/7/18 11:00 AM, Baesken, Matthias wrote:
Ping .... 😊 , any reviews / comments ?
Thanks , Matthias
-----Original Message----- From: Baesken, Matthias Sent: Dienstag, 31. Juli 2018 12:28 To: 'Chris Hegarty' <chris.hegarty@oracle.com>; Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Langer, Christoph <christoph.langer@sap.com> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello , looks like the generalization of the `includeInExceptions` security property is now in jdk/jdk after
"8207846: Generalize the jdk.net.includeInExceptions security property"
was added, great news and thanks to Chris for driving this !
I use this security property now as well , and updated the change :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.3/
I updated the CSR as well :
https://bugs.openjdk.java.net/browse/JDK-8207768
Best regards, Matthias
-----Original Message----- From: Chris Hegarty <chris.hegarty@oracle.com> Sent: Donnerstag, 19. Juli 2018 14:54 To: Alan Bateman <Alan.Bateman@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 19/07/2018 09:07, Baesken, Matthias wrote: > Hello, in the meantime I prepared a CSR : > > https://bugs.openjdk.java.net/browse/JDK-8207768 > > >> jdk.includeInExceptions expands the scope. That might be okay but we >> will need to re-visit jdk.net.includeInExceptions and also move the >> support to somewhere in jdk.internal so that it can be used by other >> code in java.base. > Is there some support code for " jdk.net.includeInExceptions " > or do you just mean the name of the property ? > Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that.
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I would like to resolve 8207846 first, then 8205525 can propose to add the new argument value, `jarPath`.
-Chris.
On 8/13/18 11:18 AM, Baesken, Matthias wrote:
As Chris and Alan mentioned, you should move the parsing of the property to a more general location so it can be used by other code that uses this property. Hi Sean, Thanks for the input and comments . Could we do the moving of the property parsing do in a followup CR, I would prefer this .
I think it should be done as part of this issue. It is too late to get this into JDK 11, so I think it is better to take the time now to do the code restructuring. Thanks, Sean
Hello, I created another webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/ - use now jarPath instead of jarpath in the java security file - moved the parsing of the property to a more general location (separate class jdk/internal/util/SecuritySystemPropertyHelper.java ) - read now from the InputStream and check for error conditions *before* setting jarFilename in the Manifest constructor Best regards , Matthias
-----Original Message----- From: Sean Mullan <sean.mullan@oracle.com> Sent: Freitag, 10. August 2018 17:39 To: Baesken, Matthias <matthias.baesken@sap.com>; Chris Hegarty <chris.hegarty@oracle.com>; Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
I need more time to finish my review but here are some initial comments:
- java.security
1079 # jarpath - enables more detailed information in the IOExceptions thrown
Use "jarPath" to be consistent with "hostInfo".
1080 # by java.util.jar.Attributes and java.util.jar.Manifest
and java.util.jar.JarFile
- Manifest.java
57 private String jarFilename = null;
Not necessary to set it to null, as that is the default.
82 Manifest(InputStream is, String jarFilename) throws IOException { 83 this.jarFilename = jarFilename; 84 read(is); 85 }
Read from the InputStream and check for error conditions *before* setting jarFilename (switch lines 83 & 84). This is a general best practice but can also protect against finalizer attacks. See JSCG 7-3 for more info: https://www.oracle.com/technetwork/java/seccodeguide-139067.html#7
- Attributes.java
As Chris and Alan mentioned, you should move the parsing of the property to a more general location so it can be used by other code that uses this property.
--Sean
On 8/8/18 11:00 AM, Sean Mullan wrote:
Cross-posting to security-dev since this fix is security related and should also be reviewed there.
--Sean
On 8/7/18 11:00 AM, Baesken, Matthias wrote:
Ping .... 😊 , any reviews / comments ?
Thanks , Matthias
-----Original Message----- From: Baesken, Matthias Sent: Dienstag, 31. Juli 2018 12:28 To: 'Chris Hegarty' <chris.hegarty@oracle.com>; Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Langer, Christoph <christoph.langer@sap.com> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello , looks like the generalization of the `includeInExceptions` security property is now in jdk/jdk after
"8207846: Generalize the jdk.net.includeInExceptions security property"
was added, great news and thanks to Chris for driving this !
I use this security property now as well , and updated the change :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.3/
I updated the CSR as well :
https://bugs.openjdk.java.net/browse/JDK-8207768
Best regards, Matthias
-----Original Message----- From: Chris Hegarty <chris.hegarty@oracle.com> Sent: Donnerstag, 19. Juli 2018 14:54 To: Alan Bateman <Alan.Bateman@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 19 Jul 2018, at 11:46, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 19/07/2018 09:07, Baesken, Matthias wrote: > Hello, in the meantime I prepared a CSR : > > https://bugs.openjdk.java.net/browse/JDK-8207768 > > >> jdk.includeInExceptions expands the scope. That might be okay but we >> will need to re-visit jdk.net.includeInExceptions and also move the >> support to somewhere in jdk.internal so that it can be used by other >> code in java.base. > Is there some support code for " jdk.net.includeInExceptions " > or do you just mean the name of the property ? > Chris is right that it's a bit premature to submit a CSR while the question on whether to rename the existing security property is on the table. I have no objection to doing that.
I filed the following issue to generalize the `includeInExceptions` security property: https://bugs.openjdk.java.net/browse/JDK-8207846
I would like to resolve 8207846 first, then 8205525 can propose to add the new argument value, `jarPath`.
-Chris.
On 24/08/2018 15:52, Baesken, Matthias wrote:
Hello, I created another webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
- use now jarPath instead of jarpath in the java security file - moved the parsing of the property to a more general location (separate class jdk/internal/util/SecuritySystemPropertyHelper.java ) - read now from the InputStream and check for error conditions *before* setting jarFilename in the Manifest constructor
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue? In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes. -Alan
Hi Alan ,
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
I think this is a separate issue .
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
Suggestions are welcome , I was a bit unsure about the name as well 😉 ! Thanks, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 27. August 2018 15:52 To: Baesken, Matthias <matthias.baesken@sap.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 24/08/2018 15:52, Baesken, Matthias wrote:
Hello, I created another webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
- use now jarPath instead of jarpath in the java security file - moved the parsing of the property to a more general location (separate class jdk/internal/util/SecuritySystemPropertyHelper.java ) - read now from the InputStream and check for error conditions *before* setting jarFilename in the Manifest constructor
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
-Alan
On Aug 27, 2018, at 10:25 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Alan ,
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
I think this is a separate issue .
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
Suggestions are welcome , I was a bit unsure about the name as well 😉 !
I am OK with creating a sun.security.util.SecurityProperties class, and a method called privilegeGetOverridable() for the 1st part of SecuritySystemPropertyHelper::initTextProperty (by overridable I mean a system property can shadow the security property). I have no idea where the 2nd part should go. Maybe you can just create an includedInExceptions(String refName) method for this purpose. So it will be something like this public class SecurityProperties { public static String privilegeGetOverridable(String propName) { return AccessController.doPrivileged((PrivilegedAction<String>) () -> { String val = System.getProperty(propName); if (val == null) { return Security.getProperty(propName); } else { return val; } }); } public static boolean includedInExceptions(String refName) { String val = privilegeGetOverridable("jdk.includeInExceptions"); if (val == null){ return false; } String[] tokens = val.split(","); for (String token : tokens) { if (token.equalsIgnoreCase(refName)) return true; } return false; } } Thanks Max
Thanks, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 27. August 2018 15:52 To: Baesken, Matthias <matthias.baesken@sap.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 24/08/2018 15:52, Baesken, Matthias wrote:
Hello, I created another webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
- use now jarPath instead of jarpath in the java security file - moved the parsing of the property to a more general location (separate class jdk/internal/util/SecuritySystemPropertyHelper.java ) - read now from the InputStream and check for error conditions *before* setting jarFilename in the Manifest constructor
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
-Alan
Hi Max, thanks for your input . I created another webrev , this contains now the suggested SecurityProperties class : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/ Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Montag, 27. August 2018 17:35 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 27, 2018, at 10:25 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Alan ,
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
I think this is a separate issue .
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
Suggestions are welcome , I was a bit unsure about the name as well 😉 !
I am OK with creating a sun.security.util.SecurityProperties class, and a method called privilegeGetOverridable() for the 1st part of SecuritySystemPropertyHelper::initTextProperty (by overridable I mean a system property can shadow the security property). I have no idea where the 2nd part should go. Maybe you can just create an includedInExceptions(String refName) method for this purpose.
So it will be something like this
public class SecurityProperties {
public static String privilegeGetOverridable(String propName) { return AccessController.doPrivileged((PrivilegedAction<String>) () -> { String val = System.getProperty(propName); if (val == null) { return Security.getProperty(propName); } else { return val; } }); }
public static boolean includedInExceptions(String refName) { String val = privilegeGetOverridable("jdk.includeInExceptions"); if (val == null){ return false; } String[] tokens = val.split(","); for (String token : tokens) { if (token.equalsIgnoreCase(refName)) return true; } return false; } }
Thanks Max
Thanks, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 27. August 2018 15:52 To: Baesken, Matthias <matthias.baesken@sap.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 24/08/2018 15:52, Baesken, Matthias wrote:
Hello, I created another webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
- use now jarPath instead of jarpath in the java security file - moved the parsing of the property to a more general location
(separate
class jdk/internal/util/SecuritySystemPropertyHelper.java )
- read now from the InputStream and check for error conditions *before* setting jarFilename in the Manifest constructor
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
-Alan
SecurityProperties.java: - Please add the copyright+license header to the new file. - The "jdk.includeInExceptions" definition says there can be "Leading and trailing whitespaces". You might need to call trim() on each token. java.security: - I would suggest saying "classes from the java.util.jar package" or just "when parsing a jar file". Others: - What will the output look like? Is it "/tmp/x.jar:100"? If I read correctly, the line number is of the manifest and not the jar file. Thanks Max
On Aug 29, 2018, at 10:01 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, thanks for your input .
I created another webrev , this contains now the suggested SecurityProperties class :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Montag, 27. August 2018 17:35 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 27, 2018, at 10:25 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Alan ,
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
I think this is a separate issue .
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
Suggestions are welcome , I was a bit unsure about the name as well 😉 !
I am OK with creating a sun.security.util.SecurityProperties class, and a method called privilegeGetOverridable() for the 1st part of SecuritySystemPropertyHelper::initTextProperty (by overridable I mean a system property can shadow the security property). I have no idea where the 2nd part should go. Maybe you can just create an includedInExceptions(String refName) method for this purpose.
So it will be something like this
public class SecurityProperties {
public static String privilegeGetOverridable(String propName) { return AccessController.doPrivileged((PrivilegedAction<String>) () -> { String val = System.getProperty(propName); if (val == null) { return Security.getProperty(propName); } else { return val; } }); }
public static boolean includedInExceptions(String refName) { String val = privilegeGetOverridable("jdk.includeInExceptions"); if (val == null){ return false; } String[] tokens = val.split(","); for (String token : tokens) { if (token.equalsIgnoreCase(refName)) return true; } return false; } }
Thanks Max
Thanks, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 27. August 2018 15:52 To: Baesken, Matthias <matthias.baesken@sap.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 24/08/2018 15:52, Baesken, Matthias wrote:
Hello, I created another webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
- use now jarPath instead of jarpath in the java security file - moved the parsing of the property to a more general location
(separate
class jdk/internal/util/SecuritySystemPropertyHelper.java )
- read now from the InputStream and check for error conditions *before* setting jarFilename in the Manifest constructor
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
-Alan
Hi Max, I changed the following in the new webrev : - added the copyright+license header to the new file - called trim on each token - adjusted the comment java.security like you suggested http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/
- What will the output look like? Is it "/tmp/x.jar:100"?
Yes it look like this : line too long (/testdata/jars/file_with_long_line_1.jar:2) Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Mittwoch, 29. August 2018 16:27 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
SecurityProperties.java:
- Please add the copyright+license header to the new file.
- The "jdk.includeInExceptions" definition says there can be "Leading and trailing whitespaces". You might need to call trim() on each token.
java.security:
- I would suggest saying "classes from the java.util.jar package" or just "when parsing a jar file".
Others:
- What will the output look like? Is it "/tmp/x.jar:100"? If I read correctly, the line number is of the manifest and not the jar file.
Thanks Max
On Aug 29, 2018, at 10:01 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, thanks for your input .
I created another webrev , this contains now the suggested SecurityProperties class :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Montag, 27. August 2018 17:35 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 27, 2018, at 10:25 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Alan ,
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
I think this is a separate issue .
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
Suggestions are welcome , I was a bit unsure about the name as well 😉 !
I am OK with creating a sun.security.util.SecurityProperties class, and a method called privilegeGetOverridable() for the 1st part of SecuritySystemPropertyHelper::initTextProperty (by overridable I mean a system property can shadow the security property). I have no idea where the 2nd part should go. Maybe you can just create an includedInExceptions(String refName) method for this purpose.
So it will be something like this
public class SecurityProperties {
public static String privilegeGetOverridable(String propName) { return AccessController.doPrivileged((PrivilegedAction<String>) () -> { String val = System.getProperty(propName); if (val == null) { return Security.getProperty(propName); } else { return val; } }); }
public static boolean includedInExceptions(String refName) { String val = privilegeGetOverridable("jdk.includeInExceptions"); if (val == null){ return false; } String[] tokens = val.split(","); for (String token : tokens) { if (token.equalsIgnoreCase(refName)) return true; } return false; } }
Thanks Max
Thanks, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 27. August 2018 15:52 To: Baesken, Matthias <matthias.baesken@sap.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty
<chris.hegarty@oracle.com>
Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 24/08/2018 15:52, Baesken, Matthias wrote:
Hello, I created another webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.5/
- use now jarPath instead of jarpath in the java security file - moved the parsing of the property to a more general location (separate class jdk/internal/util/SecuritySystemPropertyHelper.java ) - read now from the InputStream and check for error conditions *before* setting jarFilename in the Manifest constructor
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
In terms of approach then I think what you have is okay although I think we should try to find a better name than "SecuritySystemPropertyHelper" and also get feedback from security-dev on whether they would prefer it in the sun.security tree with the other classes security classes.
-Alan
On Aug 30, 2018, at 8:26 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
- What will the output look like? Is it "/tmp/x.jar:100"?
Yes it look like this :
line too long (/testdata/jars/file_with_long_line_1.jar:2)
Is this a little misleading? I think you mean /testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2 Thanks Max
Hi Max, probably we should add the info about the MANIFEST.MF , for example : change getErrorPosition to http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/share/c... static String getErrorPosition(String filename, final int lineNumber) { if (filename == null || !jarPathInExceptionText) { return "META-INF/MANIFEST.MF line:" + lineNumber; } final File file = new File(filename); return AccessController.doPrivileged(new PrivilegedAction<String>() { public String run() { return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:" + lineNumber; } ..... Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Donnerstag, 30. August 2018 16:04 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 30, 2018, at 8:26 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
- What will the output look like? Is it "/tmp/x.jar:100"?
Yes it look like this :
line too long (/testdata/jars/file_with_long_line_1.jar:2)
Is this a little misleading? I think you mean
/testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2
Thanks Max
Some more comments: Attributes.java - No need to "import java.security.Security". - In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable. - I feel like calling Attributes::getErrorPosition from Manifest a little strange. Maybe it's better to define the method in Manifest and call it from Attributes. First, I think putting the method in a higher level object makes more sense. Second, the position is for the whole Manifest and not an Attributes section (I mean the line number is counted from the first line of the manifest).
On Aug 30, 2018, at 10:50 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, probably we should add the info about the MANIFEST.MF , for example : change getErrorPosition to
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/share/c...
static String getErrorPosition(String filename, final int lineNumber) { if (filename == null || !jarPathInExceptionText) { return "META-INF/MANIFEST.MF line:" + lineNumber; }
final File file = new File(filename); return AccessController.doPrivileged(new PrivilegedAction<String>() { public String run() { return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:" + lineNumber;
I prefer "file:line" which is more compact and more commonly used. In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now. if we want to show the .SF name also, we will need a public API because SignatureFileVerifier.java is inside sun.security.util. Something like Manifest(InputStream,jarName,entryName)? Sorry for making this complicated. Thanks Max
} .....
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Donnerstag, 30. August 2018 16:04 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 30, 2018, at 8:26 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
- What will the output look like? Is it "/tmp/x.jar:100"?
Yes it look like this :
line too long (/testdata/jars/file_with_long_line_1.jar:2)
Is this a little misleading? I think you mean
/testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2
Thanks Max
Or you can smuggle it out through JavaUtilJarAccess with SharedSecrets.
On Aug 31, 2018, at 10:32 AM, Weijun Wang <weijun.wang@oracle.com> wrote:
if we want to show the .SF name also, we will need a public API because SignatureFileVerifier.java is inside sun.security.util. Something like Manifest(InputStream,jarName,entryName)?
Hi Max :
- No need to "import java.security.Security".
Sure I can remove this, it is a leftover.
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
Currently we need it in Manifest.java public void read(InputStream is) throws IOException { ... }
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now.
Then lets just say <jarfile>:<line-number> (e.g. test.jar:10 ) Or ( to mention that it is about a manifest from the jar ) <jarfile>:manifest-line <line-number> (e.g. test.jar:manifest-line 10 ) Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 04:32 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Some more comments:
Attributes.java
- No need to "import java.security.Security".
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
- I feel like calling Attributes::getErrorPosition from Manifest a little strange. Maybe it's better to define the method in Manifest and call it from Attributes. First, I think putting the method in a higher level object makes more sense. Second, the position is for the whole Manifest and not an Attributes section (I mean the line number is counted from the first line of the manifest).
On Aug 30, 2018, at 10:50 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, probably we should add the info about the MANIFEST.MF , for example : change getErrorPosition to
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s hare/classes/java/util/jar/Attributes.java.udiff.html
static String getErrorPosition(String filename, final int lineNumber) { if (filename == null || !jarPathInExceptionText) { return "META-INF/MANIFEST.MF line:" + lineNumber; }
final File file = new File(filename); return AccessController.doPrivileged(new PrivilegedAction<String>() { public String run() { return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:" +
lineNumber;
I prefer "file:line" which is more compact and more commonly used.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now. if we want to show the .SF name also, we will need a public API because SignatureFileVerifier.java is inside sun.security.util. Something like Manifest(InputStream,jarName,entryName)?
Sorry for making this complicated.
Thanks Max
} .....
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Donnerstag, 30. August 2018 16:04 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 30, 2018, at 8:26 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
- What will the output look like? Is it "/tmp/x.jar:100"?
Yes it look like this :
line too long (/testdata/jars/file_with_long_line_1.jar:2)
Is this a little misleading? I think you mean
/testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2
Thanks Max
On Aug 31, 2018, at 8:52 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max :
- No need to "import java.security.Security".
Sure I can remove this, it is a leftover.
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
Currently we need it in Manifest.java public void read(InputStream is) throws IOException { ... }
I was talking about the name of the parameter. You can simply use int read(Manifest.FastInputStream is, byte[] lbuf, String filename, int lineNumber) and there is no need to reassign it with "int lineNumber = offset" anymore.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now.
Then lets just say <jarfile>:<line-number> (e.g. test.jar:10 )
Or ( to mention that it is about a manifest from the jar )
<jarfile>:manifest-line <line-number> (e.g. test.jar:manifest-line 10 )
How about you pass in the full name ("/path/to/file.jar!META-INF/MANIFEST.MF") to "new Manifest(stream,name)" directly? So the name will be constructed in JarFile.java (after checking for jarPathInExceptionText). The getErrorPosition method simply concat the name (if not null) and the line number. Thus the exception thrown from parsing X.SF simply will not include any file info. If we want it we can enhance later. Thanks Max
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 04:32 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Some more comments:
Attributes.java
- No need to "import java.security.Security".
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
- I feel like calling Attributes::getErrorPosition from Manifest a little strange. Maybe it's better to define the method in Manifest and call it from Attributes. First, I think putting the method in a higher level object makes more sense. Second, the position is for the whole Manifest and not an Attributes section (I mean the line number is counted from the first line of the manifest).
On Aug 30, 2018, at 10:50 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, probably we should add the info about the MANIFEST.MF , for example : change getErrorPosition to
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s hare/classes/java/util/jar/Attributes.java.udiff.html
static String getErrorPosition(String filename, final int lineNumber) { if (filename == null || !jarPathInExceptionText) { return "META-INF/MANIFEST.MF line:" + lineNumber; }
final File file = new File(filename); return AccessController.doPrivileged(new PrivilegedAction<String>() { public String run() { return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:" +
lineNumber;
I prefer "file:line" which is more compact and more commonly used.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now. if we want to show the .SF name also, we will need a public API because SignatureFileVerifier.java is inside sun.security.util. Something like Manifest(InputStream,jarName,entryName)?
Sorry for making this complicated.
Thanks Max
} .....
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Donnerstag, 30. August 2018 16:04 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 30, 2018, at 8:26 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
- What will the output look like? Is it "/tmp/x.jar:100"?
Yes it look like this :
line too long (/testdata/jars/file_with_long_line_1.jar:2)
Is this a little misleading? I think you mean
/testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2
Thanks Max
Hi Max, I - moved getErrorPosition method to Manifest.java - in read() method, removed "int offset" - in the exception message, I write now " manifest of " ... (without mentioning a manifest name) http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/ Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 15:53 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 31, 2018, at 8:52 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max :
- No need to "import java.security.Security".
Sure I can remove this, it is a leftover.
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
Currently we need it in Manifest.java public void read(InputStream is) throws IOException { ... }
I was talking about the name of the parameter. You can simply use
int read(Manifest.FastInputStream is, byte[] lbuf, String filename, int lineNumber)
and there is no need to reassign it with "int lineNumber = offset" anymore.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now.
Then lets just say <jarfile>:<line-number> (e.g. test.jar:10 )
Or ( to mention that it is about a manifest from the jar )
<jarfile>:manifest-line <line-number> (e.g. test.jar:manifest-line 10 )
How about you pass in the full name ("/path/to/file.jar!META- INF/MANIFEST.MF") to "new Manifest(stream,name)" directly?
So the name will be constructed in JarFile.java (after checking for jarPathInExceptionText). The getErrorPosition method simply concat the name (if not null) and the line number. Thus the exception thrown from parsing X.SF simply will not include any file info. If we want it we can enhance later.
Thanks Max
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 04:32 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Some more comments:
Attributes.java
- No need to "import java.security.Security".
- In the updated read() method, I think there is no need to use an "int
parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
- I feel like calling Attributes::getErrorPosition from Manifest a little strange. Maybe it's better to define the method in Manifest and call it from Attributes. First, I think putting the method in a higher level object makes more sense. Second, the position is for the whole Manifest and not an Attributes section (I mean the line number is counted from the first line of the manifest).
On Aug 30, 2018, at 10:50 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, probably we should add the info about the MANIFEST.MF , for example : change getErrorPosition to
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s
hare/classes/java/util/jar/Attributes.java.udiff.html
static String getErrorPosition(String filename, final int lineNumber) { if (filename == null || !jarPathInExceptionText) { return "META-INF/MANIFEST.MF line:" + lineNumber; }
final File file = new File(filename); return AccessController.doPrivileged(new PrivilegedAction<String>()
{
public String run() { return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:"
offset" +
lineNumber;
I prefer "file:line" which is more compact and more commonly used.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now. if we want to show the .SF name also, we will need a public API because SignatureFileVerifier.java is inside sun.security.util. Something like Manifest(InputStream,jarName,entryName)?
Sorry for making this complicated.
Thanks Max
} .....
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Donnerstag, 30. August 2018 16:04 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty
<chris.hegarty@oracle.com>;
security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 30, 2018, at 8:26 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
> - What will the output look like? Is it "/tmp/x.jar:100"? >
Yes it look like this :
line too long (/testdata/jars/file_with_long_line_1.jar:2)
Is this a little misleading? I think you mean
/testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2
Thanks Max
Hi Matthias The change looks fine. We can enhance the name if we want to support .SF parsing later. Please revise your CSR and get it approved first. Thanks Max
On Sep 3, 2018, at 10:19 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, I
- moved getErrorPosition method to Manifest.java - in read() method, removed "int offset" - in the exception message, I write now " manifest of " ... (without mentioning a manifest name)
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 15:53 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 31, 2018, at 8:52 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max :
- No need to "import java.security.Security".
Sure I can remove this, it is a leftover.
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
Currently we need it in Manifest.java public void read(InputStream is) throws IOException { ... }
I was talking about the name of the parameter. You can simply use
int read(Manifest.FastInputStream is, byte[] lbuf, String filename, int lineNumber)
and there is no need to reassign it with "int lineNumber = offset" anymore.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now.
Then lets just say <jarfile>:<line-number> (e.g. test.jar:10 )
Or ( to mention that it is about a manifest from the jar )
<jarfile>:manifest-line <line-number> (e.g. test.jar:manifest-line 10 )
How about you pass in the full name ("/path/to/file.jar!META- INF/MANIFEST.MF") to "new Manifest(stream,name)" directly?
So the name will be constructed in JarFile.java (after checking for jarPathInExceptionText). The getErrorPosition method simply concat the name (if not null) and the line number. Thus the exception thrown from parsing X.SF simply will not include any file info. If we want it we can enhance later.
Thanks Max
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 04:32 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Some more comments:
Attributes.java
- No need to "import java.security.Security".
- In the updated read() method, I think there is no need to use an "int
parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
- I feel like calling Attributes::getErrorPosition from Manifest a little strange. Maybe it's better to define the method in Manifest and call it from Attributes. First, I think putting the method in a higher level object makes more sense. Second, the position is for the whole Manifest and not an Attributes section (I mean the line number is counted from the first line of the manifest).
On Aug 30, 2018, at 10:50 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, probably we should add the info about the MANIFEST.MF , for example : change getErrorPosition to
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s
hare/classes/java/util/jar/Attributes.java.udiff.html
static String getErrorPosition(String filename, final int lineNumber) { if (filename == null || !jarPathInExceptionText) { return "META-INF/MANIFEST.MF line:" + lineNumber; }
final File file = new File(filename); return AccessController.doPrivileged(new PrivilegedAction<String>()
{
public String run() { return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:"
offset" +
lineNumber;
I prefer "file:line" which is more compact and more commonly used.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now. if we want to show the .SF name also, we will need a public API because SignatureFileVerifier.java is inside sun.security.util. Something like Manifest(InputStream,jarName,entryName)?
Sorry for making this complicated.
Thanks Max
} .....
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Donnerstag, 30. August 2018 16:04 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote: > >> - What will the output look like? Is it "/tmp/x.jar:100"? >> > > Yes it look like this : > > line too long (/testdata/jars/file_with_long_line_1.jar:2)
Is this a little misleading? I think you mean
/testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2
Thanks Max
The change looks fine. We can enhance the name if we want to support .SF parsing later.
Please revise your CSR and get it approved first.
Hi Max, Thanks ! I adjusted the CSR , please approve : https://bugs.openjdk.java.net/browse/JDK-8207768 Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Montag, 3. September 2018 17:14 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Matthias
The change looks fine. We can enhance the name if we want to support .SF parsing later.
Please revise your CSR and get it approved first.
Thanks Max
On Sep 3, 2018, at 10:19 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, I
- moved getErrorPosition method to Manifest.java - in read() method, removed "int offset" - in the exception message, I write now " manifest of " ... (without mentioning a manifest name)
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 15:53 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 31, 2018, at 8:52 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max :
- No need to "import java.security.Security".
Sure I can remove this, it is a leftover.
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
Currently we need it in Manifest.java public void read(InputStream is) throws IOException { ... }
I was talking about the name of the parameter. You can simply use
int read(Manifest.FastInputStream is, byte[] lbuf, String filename, int lineNumber)
and there is no need to reassign it with "int lineNumber = offset" anymore.
In fact, I have a small concern on the hardcoded file name here,
because
when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now.
Then lets just say <jarfile>:<line-number> (e.g. test.jar:10 )
Or ( to mention that it is about a manifest from the jar )
<jarfile>:manifest-line <line-number> (e.g. test.jar:manifest-line 10 )
How about you pass in the full name ("/path/to/file.jar!META- INF/MANIFEST.MF") to "new Manifest(stream,name)" directly?
So the name will be constructed in JarFile.java (after checking for jarPathInExceptionText). The getErrorPosition method simply concat the name (if not null) and the line number. Thus the exception thrown from parsing X.SF simply will not include any file info. If we want it we can enhance later.
Thanks Max
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 04:32 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty
<chris.hegarty@oracle.com>;
security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Some more comments:
Attributes.java
- No need to "import java.security.Security".
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
- I feel like calling Attributes::getErrorPosition from Manifest a little strange. Maybe it's better to define the method in Manifest and call it from Attributes. First, I think putting the method in a higher level object makes more sense. Second, the position is for the whole Manifest and not an Attributes section (I mean the line number is counted from the first line of the manifest).
On Aug 30, 2018, at 10:50 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, probably we should add the info about the MANIFEST.MF , for example : change getErrorPosition to
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s
hare/classes/java/util/jar/Attributes.java.udiff.html
static String getErrorPosition(String filename, final int lineNumber) { if (filename == null || !jarPathInExceptionText) { return "META-INF/MANIFEST.MF line:" + lineNumber; }
final File file = new File(filename); return AccessController.doPrivileged(new PrivilegedAction<String>()
{
public String run() { return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:"
lineNumber;
I prefer "file:line" which is more compact and more commonly used.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now. if we want to show the .SF name also, we will need a public API because SignatureFileVerifier.java is inside sun.security.util. Something like Manifest(InputStream,jarName,entryName)?
Sorry for making this complicated.
Thanks Max
} .....
Best regards, Matthias
> -----Original Message----- > From: Weijun Wang <weijun.wang@oracle.com> > Sent: Donnerstag, 30. August 2018 16:04 > To: Baesken, Matthias <matthias.baesken@sap.com> > Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan > <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; > security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > parsing of jar archives > > > >> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias > <matthias.baesken@sap.com> wrote: >> >>> - What will the output look like? Is it "/tmp/x.jar:100"? >>> >> >> Yes it look like this : >> >> line too long (/testdata/jars/file_with_long_line_1.jar:2) > > Is this a little misleading? I think you mean > > /testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2 > > Thanks > Max
I've added myself as a reviewer. You might want to set scope to "JDK". A CSR is approved by the CSR group after you finalize it. First you should propose it. If you think it's urgent or trivial, you can also fast-track it. Read https://wiki.openjdk.java.net/display/csr/Main for more details. Thanks Max
On Sep 4, 2018, at 7:33 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
The change looks fine. We can enhance the name if we want to support .SF parsing later.
Please revise your CSR and get it approved first.
Hi Max, Thanks !
I adjusted the CSR , please approve :
https://bugs.openjdk.java.net/browse/JDK-8207768
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Montag, 3. September 2018 17:14 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Matthias
The change looks fine. We can enhance the name if we want to support .SF parsing later.
Please revise your CSR and get it approved first.
Thanks Max
On Sep 3, 2018, at 10:19 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max, I
- moved getErrorPosition method to Manifest.java - in read() method, removed "int offset" - in the exception message, I write now " manifest of " ... (without mentioning a manifest name)
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 15:53 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Aug 31, 2018, at 8:52 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hi Max :
- No need to "import java.security.Security".
Sure I can remove this, it is a leftover.
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
Currently we need it in Manifest.java public void read(InputStream is) throws IOException { ... }
I was talking about the name of the parameter. You can simply use
int read(Manifest.FastInputStream is, byte[] lbuf, String filename, int lineNumber)
and there is no need to reassign it with "int lineNumber = offset" anymore.
In fact, I have a small concern on the hardcoded file name here,
because
when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now.
Then lets just say <jarfile>:<line-number> (e.g. test.jar:10 )
Or ( to mention that it is about a manifest from the jar )
<jarfile>:manifest-line <line-number> (e.g. test.jar:manifest-line 10 )
How about you pass in the full name ("/path/to/file.jar!META- INF/MANIFEST.MF") to "new Manifest(stream,name)" directly?
So the name will be constructed in JarFile.java (after checking for jarPathInExceptionText). The getErrorPosition method simply concat the name (if not null) and the line number. Thus the exception thrown from parsing X.SF simply will not include any file info. If we want it we can enhance later.
Thanks Max
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Freitag, 31. August 2018 04:32 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan <sean.mullan@oracle.com>; Chris Hegarty
<chris.hegarty@oracle.com>;
security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Some more comments:
Attributes.java
- No need to "import java.security.Security".
- In the updated read() method, I think there is no need to use an "int offset" parameter. "int lineNumber" is enough and you can modify it and return it without a new local variable.
- I feel like calling Attributes::getErrorPosition from Manifest a little strange. Maybe it's better to define the method in Manifest and call it from Attributes. First, I think putting the method in a higher level object makes more sense. Second, the position is for the whole Manifest and not an Attributes section (I mean the line number is counted from the first line of the manifest).
> On Aug 30, 2018, at 10:50 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote: > > > > Hi Max, probably we should add the info about the MANIFEST.MF , for example : > change getErrorPosition to > >
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s
hare/classes/java/util/jar/Attributes.java.udiff.html > > > static String getErrorPosition(String filename, final int lineNumber) { > if (filename == null || !jarPathInExceptionText) { > return "META-INF/MANIFEST.MF line:" + lineNumber; > } > > final File file = new File(filename); > return AccessController.doPrivileged(new PrivilegedAction<String>() { > public String run() { > return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:"
lineNumber;
I prefer "file:line" which is more compact and more commonly used.
In fact, I have a small concern on the hardcoded file name here, because when verifying a signed jar, the META-INF/X.SF file is also parsed into a Manifest object and if it's invalid similar exceptions will be thrown. I don't have a nice solution now. if we want to show the .SF name also, we will need a public API because SignatureFileVerifier.java is inside sun.security.util. Something like Manifest(InputStream,jarName,entryName)?
Sorry for making this complicated.
Thanks Max
> } > ..... > > > Best regards, Matthias > > > >> -----Original Message----- >> From: Weijun Wang <weijun.wang@oracle.com> >> Sent: Donnerstag, 30. August 2018 16:04 >> To: Baesken, Matthias <matthias.baesken@sap.com> >> Cc: Alan Bateman <Alan.Bateman@oracle.com>; Sean Mullan >> <sean.mullan@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; >> security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net >> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest >> parsing of jar archives >> >> >> >>> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias >> <matthias.baesken@sap.com> wrote: >>> >>>> - What will the output look like? Is it "/tmp/x.jar:100"? >>>> >>> >>> Yes it look like this : >>> >>> line too long (/testdata/jars/file_with_long_line_1.jar:2) >> >> Is this a little misleading? I think you mean >> >> /testdata/jars/file_with_long_line_1.jar!META-INF/MANIFEST.MF:2 >> >> Thanks >> Max
On 8/29/18 10:01 AM, Baesken, Matthias wrote:
Hi Max, thanks for your input .
I created another webrev , this contains now the suggested SecurityProperties class :
java/util/jar/Attributes.java 469 return AccessController.doPrivileged(new PrivilegedAction<String>() { 470 public String run() { 471 return file.getAbsolutePath() + ":" + lineNumber; 472 } 473 }); I have a serious concern with the code above. With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible. In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path. Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property. The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path). --Sean
In my understanding, the author deliberately wants to show the absolute paths when there are multiple jar files with the same name (Ex: a jar hell). Maybe we can add some more detail in the java.security so an admin knows what exact impact it has. --Max
On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
On 8/29/18 10:01 AM, Baesken, Matthias wrote:
Hi Max, thanks for your input . I created another webrev , this contains now the suggested SecurityProperties class : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
java/util/jar/Attributes.java
469 return AccessController.doPrivileged(new PrivilegedAction<String>() { 470 public String run() { 471 return file.getAbsolutePath() + ":" + lineNumber; 472 } 473 });
I have a serious concern with the code above.
With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible.
In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property.
The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path).
--Sean
On 9/7/18 7:58 PM, Weijun Wang wrote:
In my understanding, the author deliberately wants to show the absolute paths when there are multiple jar files with the same name (Ex: a jar hell).
If you are very familiar with a particular application and understand the risks associated with running it, then I agree that is ok. But security properties apply to all applications using that JDK, and so I don't always think it is practical for an admin to understand every type of application that may be using that JDK and whether or not enabling this property presents a risk.
Maybe we can add some more detail in the java.security so an admin knows what exact impact it has.
It would be a slippery slope in my opinion. You would have to say something like: "enabling this property may allow untrusted code running under a SecurityManager to access sensitive information such as the user.home system property even if it has not been granted permission to do so." I think we should consider not allowing this property to take effect if a SecurityManager is running. --Sean
--Max
On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
On 8/29/18 10:01 AM, Baesken, Matthias wrote:
Hi Max, thanks for your input . I created another webrev , this contains now the suggested SecurityProperties class : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
java/util/jar/Attributes.java
469 return AccessController.doPrivileged(new PrivilegedAction<String>() { 470 public String run() { 471 return file.getAbsolutePath() + ":" + lineNumber; 472 } 473 });
I have a serious concern with the code above.
With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible.
In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property.
The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path).
--Sean
Thinking about this again. Looks like the absolute path is not necessary. Even if there are multiple files using the same name, they will be in different directories, no matter absolute or relative. Suppose the jarPath info is used for debugging purpose mostly like the developer can find out what the current working directory is and can find the files. *Matthias*: Is the absolute path really necessary? Are you working on actual case? As for the possible global effect of a security property, maybe we can emphasis that this is both a security property and system property, and if it’s just for one time use, it’s better to use a system property. BTW, does the existing value “hostInfo” of the property have a similar problem? Thanks Max
在 2018年9月8日，21:42，Sean Mullan <sean.mullan@oracle.com> 写道：
On 9/7/18 7:58 PM, Weijun Wang wrote: In my understanding, the author deliberately wants to show the absolute paths when there are multiple jar files with the same name (Ex: a jar hell).
If you are very familiar with a particular application and understand the risks associated with running it, then I agree that is ok. But security properties apply to all applications using that JDK, and so I don't always think it is practical for an admin to understand every type of application that may be using that JDK and whether or not enabling this property presents a risk.
Maybe we can add some more detail in the java.security so an admin knows what exact impact it has.
It would be a slippery slope in my opinion. You would have to say something like: "enabling this property may allow untrusted code running under a SecurityManager to access sensitive information such as the user.home system property even if it has not been granted permission to do so."
I think we should consider not allowing this property to take effect if a SecurityManager is running.
--Sean
--Max
On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
On 8/29/18 10:01 AM, Baesken, Matthias wrote:
Hi Max, thanks for your input . I created another webrev , this contains now the suggested SecurityProperties class : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
java/util/jar/Attributes.java
469 return AccessController.doPrivileged(new PrivilegedAction<String>() { 470 public String run() { 471 return file.getAbsolutePath() + ":" + lineNumber; 472 } 473 });
I have a serious concern with the code above.
With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible.
In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property.
The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path).
--Sean
Hello are you fine with changing from file.getAbsolutePath() to file.getName() ? http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/src/java.base/share/c... 206 static String getErrorPosition(String filename, final int lineNumber) { 207 if (filename == null || !jarPathInExceptionText) { 208 return "line " + lineNumber; 209 } 210 211 final File file = new File(filename); 212 return AccessController.doPrivileged(new PrivilegedAction<String>() { 213 public String run() { 214 return "manifest of " + file.getName() + ":" + lineNumber; 215 } Best regards, Matthias
-----Original Message----- From: Wang Weijun <weijun.wang@oracle.com> Sent: Samstag, 8. September 2018 17:43 To: Sean Mullan <sean.mullan@oracle.com> Cc: Baesken, Matthias <matthias.baesken@sap.com>; Alan Bateman <Alan.Bateman@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Thinking about this again. Looks like the absolute path is not necessary. Even if there are multiple files using the same name, they will be in different directories, no matter absolute or relative. Suppose the jarPath info is used for debugging purpose mostly like the developer can find out what the current working directory is and can find the files. *Matthias*: Is the absolute path really necessary? Are you working on actual case?
As for the possible global effect of a security property, maybe we can emphasis that this is both a security property and system property, and if it’s just for one time use, it’s better to use a system property.
BTW, does the existing value “hostInfo” of the property have a similar problem?
Thanks Max
在 2018年9月8日，21:42，Sean Mullan <sean.mullan@oracle.com> 写 道：
On 9/7/18 7:58 PM, Weijun Wang wrote: In my understanding, the author deliberately wants to show the absolute paths when there are multiple jar files with the same name (Ex: a jar hell).
If you are very familiar with a particular application and understand the risks associated with running it, then I agree that is ok. But security properties apply to all applications using that JDK, and so I don't always think it is practical for an admin to understand every type of application that may be using that JDK and whether or not enabling this property presents a risk.
Maybe we can add some more detail in the java.security so an admin knows what exact impact it has.
It would be a slippery slope in my opinion. You would have to say something like: "enabling this property may allow untrusted code running under a SecurityManager to access sensitive information such as the user.home system property even if it has not been granted permission to do so."
I think we should consider not allowing this property to take effect if a SecurityManager is running.
--Sean
--Max
On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
On 8/29/18 10:01 AM, Baesken, Matthias wrote:
Hi Max, thanks for your input . I created another webrev , this contains now the suggested SecurityProperties class : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
java/util/jar/Attributes.java
469 return AccessController.doPrivileged(new PrivilegedAction<String>() { 470 public String run() { 471 return file.getAbsolutePath() + ":" + lineNumber; 472 } 473 });
I have a serious concern with the code above.
With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible.
In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property.
The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path).
--Sean
Hi Matthias, I think it would be enough to drop the privileged section and just return "filename" as is. (without conveting to a file object). I also agree with Sean that the common parts of the new src/java.base/share/classes/sun/security/util/SecurityProperties.java should be aligned with src/java.base/share/classes/sun/net/util/SocketExceptions.java (That is, SocketExceptions calling SecurityProperties) Best regards Christoph
-----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Baesken, Matthias Sent: Montag, 10. September 2018 09:53 To: Wang Weijun <weijun.wang@oracle.com>; Sean Mullan <sean.mullan@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: [CAUTION] RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello are you fine with changing from file.getAbsolutePath() to file.getName() ?
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/src/java.base/s hare/classes/java/util/jar/Manifest.java.frames.html
206 static String getErrorPosition(String filename, final int lineNumber) { 207 if (filename == null || !jarPathInExceptionText) { 208 return "line " + lineNumber; 209 } 210 211 final File file = new File(filename); 212 return AccessController.doPrivileged(new PrivilegedAction<String>() { 213 public String run() { 214 return "manifest of " + file.getName() + ":" + lineNumber; 215 }
Best regards, Matthias
-----Original Message----- From: Wang Weijun <weijun.wang@oracle.com> Sent: Samstag, 8. September 2018 17:43 To: Sean Mullan <sean.mullan@oracle.com> Cc: Baesken, Matthias <matthias.baesken@sap.com>; Alan Bateman <Alan.Bateman@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Thinking about this again. Looks like the absolute path is not necessary. Even if there are multiple files using the same name, they will be in different directories, no matter absolute or relative. Suppose the jarPath info is used for debugging purpose mostly like the developer can find out what the current working directory is and can find the files. *Matthias*: Is the absolute path really necessary? Are you working on actual case?
As for the possible global effect of a security property, maybe we can emphasis that this is both a security property and system property, and if it’s just for one time use, it’s better to use a system property.
BTW, does the existing value “hostInfo” of the property have a similar problem?
Thanks Max
在 2018年9月8日，21:42，Sean Mullan <sean.mullan@oracle.com> 写 道：
On 9/7/18 7:58 PM, Weijun Wang wrote: In my understanding, the author deliberately wants to show the absolute paths when there are multiple jar files with the same name (Ex: a jar hell).
If you are very familiar with a particular application and understand the risks associated with running it, then I agree that is ok. But security properties apply to all applications using that JDK, and so I don't always think it is practical for an admin to understand every type of application that may be using that JDK and whether or not enabling this property presents a risk.
Maybe we can add some more detail in the java.security so an admin knows what exact impact it has.
It would be a slippery slope in my opinion. You would have to say something like: "enabling this property may allow untrusted code running under a SecurityManager to access sensitive information such as the user.home system property even if it has not been granted permission to do so."
I think we should consider not allowing this property to take effect if a SecurityManager is running.
--Sean
--Max
On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
On 8/29/18 10:01 AM, Baesken, Matthias wrote:
Hi Max, thanks for your input . I created another webrev , this contains now the suggested SecurityProperties class : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
java/util/jar/Attributes.java
469 return AccessController.doPrivileged(new PrivilegedAction<String>() { 470 public String run() { 471 return file.getAbsolutePath() + ":" + lineNumber; 472 } 473 });
I have a serious concern with the code above.
With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible.
In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property.
The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path).
--Sean
I think it would be enough to drop the privileged section and just return "filename" as is. (without conveting to a file object).
OK, any objections on this ?
I also agree with Sean that the common parts of the new src/java.base/share/classes/sun/security/util/SecurityProperties.java should be aligned with src/java.base/share/classes/sun/net/util/SocketExceptions.java (That is, SocketExceptions calling SecurityProperties)
OK I will include this in the next webrev . Thanks, Matthias
-----Original Message----- From: Langer, Christoph Sent: Montag, 10. September 2018 10:04 To: Baesken, Matthias <matthias.baesken@sap.com>; Wang Weijun <weijun.wang@oracle.com>; Sean Mullan <sean.mullan@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Matthias,
I think it would be enough to drop the privileged section and just return "filename" as is. (without conveting to a file object).
I also agree with Sean that the common parts of the new src/java.base/share/classes/sun/security/util/SecurityProperties.java should be aligned with src/java.base/share/classes/sun/net/util/SocketExceptions.java (That is, SocketExceptions calling SecurityProperties)
Best regards Christoph
-----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Baesken, Matthias Sent: Montag, 10. September 2018 09:53 To: Wang Weijun <weijun.wang@oracle.com>; Sean Mullan <sean.mullan@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: [CAUTION] RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello are you fine with changing from file.getAbsolutePath() to file.getName() ?
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/src/java.base/s
hare/classes/java/util/jar/Manifest.java.frames.html
206 static String getErrorPosition(String filename, final int lineNumber) { 207 if (filename == null || !jarPathInExceptionText) { 208 return "line " + lineNumber; 209 } 210 211 final File file = new File(filename); 212 return AccessController.doPrivileged(new PrivilegedAction<String>() { 213 public String run() { 214 return "manifest of " + file.getName() + ":" + lineNumber; 215 }
Best regards, Matthias
-----Original Message----- From: Wang Weijun <weijun.wang@oracle.com> Sent: Samstag, 8. September 2018 17:43 To: Sean Mullan <sean.mullan@oracle.com> Cc: Baesken, Matthias <matthias.baesken@sap.com>; Alan Bateman <Alan.Bateman@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Thinking about this again. Looks like the absolute path is not necessary. Even if there are multiple files using the same name, they will be in different directories, no matter absolute or relative. Suppose the jarPath info is used for debugging purpose mostly like the developer can find out what the current working directory is and can find the files. *Matthias*: Is the absolute path really necessary? Are you working on actual case?
As for the possible global effect of a security property, maybe we can emphasis that this is both a security property and system property, and if it’s just for one time use, it’s better to use a system property.
BTW, does the existing value “hostInfo” of the property have a similar problem?
Thanks Max
在 2018年9月8日，21:42，Sean Mullan <sean.mullan@oracle.com> 写 道：
On 9/7/18 7:58 PM, Weijun Wang wrote: In my understanding, the author deliberately wants to show the absolute paths when there are multiple jar files with the same name (Ex: a jar hell).
If you are very familiar with a particular application and understand the risks associated with running it, then I agree that is ok. But security properties apply to all applications using that JDK, and so I don't always think it is practical for an admin to understand every type of application that may be using that JDK and whether or not enabling this property presents a risk.
Maybe we can add some more detail in the java.security so an admin knows what exact impact it has.
It would be a slippery slope in my opinion. You would have to say something like: "enabling this property may allow untrusted code running under a SecurityManager to access sensitive information such as the user.home system property even if it has not been granted permission to do so."
I think we should consider not allowing this property to take effect if a SecurityManager is running.
--Sean
--Max
On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
On 8/29/18 10:01 AM, Baesken, Matthias wrote: > Hi Max, thanks for your input . > I created another webrev , this contains now the suggested SecurityProperties class : > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
java/util/jar/Attributes.java
469 return AccessController.doPrivileged(new PrivilegedAction<String>() { 470 public String run() { 471 return file.getAbsolutePath() + ":" + lineNumber; 472 } 473 });
I have a serious concern with the code above.
With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible.
In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property.
The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path).
--Sean
On 9/10/18 4:21 AM, Baesken, Matthias wrote:
I think it would be enough to drop the privileged section and just return "filename" as is. (without conveting to a file object).
OK, any objections on this ?
No, this is fine with me. --Sean
New webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.9/ - SocketExceptions class has been adjusted to new sun.security.util.SecurityProperties - Attributes getErrorPosition adjusted (see proposal of Christoph " I think it would be enough to drop the privileged section and just return "filename" as is.) After the changes I wonder - should the jarPath category be renamed to jarFile (or something else) ? Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Montag, 10. September 2018 10:04 To: Baesken, Matthias <matthias.baesken@sap.com>; Wang Weijun <weijun.wang@oracle.com>; Sean Mullan <sean.mullan@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Matthias,
I think it would be enough to drop the privileged section and just return "filename" as is. (without conveting to a file object).
I also agree with Sean that the common parts of the new src/java.base/share/classes/sun/security/util/SecurityProperties.java should be aligned with src/java.base/share/classes/sun/net/util/SocketExceptions.java (That is, SocketExceptions calling SecurityProperties)
Best regards Christoph
-----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Baesken, Matthias Sent: Montag, 10. September 2018 09:53 To: Wang Weijun <weijun.wang@oracle.com>; Sean Mullan <sean.mullan@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: [CAUTION] RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello are you fine with changing from file.getAbsolutePath() to file.getName() ?
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/src/java.base/s
hare/classes/java/util/jar/Manifest.java.frames.html
206 static String getErrorPosition(String filename, final int lineNumber) { 207 if (filename == null || !jarPathInExceptionText) { 208 return "line " + lineNumber; 209 } 210 211 final File file = new File(filename); 212 return AccessController.doPrivileged(new PrivilegedAction<String>() { 213 public String run() { 214 return "manifest of " + file.getName() + ":" + lineNumber; 215 }
Best regards, Matthias
-----Original Message----- From: Wang Weijun <weijun.wang@oracle.com> Sent: Samstag, 8. September 2018 17:43 To: Sean Mullan <sean.mullan@oracle.com> Cc: Baesken, Matthias <matthias.baesken@sap.com>; Alan Bateman <Alan.Bateman@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Thinking about this again. Looks like the absolute path is not necessary. Even if there are multiple files using the same name, they will be in different directories, no matter absolute or relative. Suppose the jarPath info is used for debugging purpose mostly like the developer can find out what the current working directory is and can find the files. *Matthias*: Is the absolute path really necessary? Are you working on actual case?
As for the possible global effect of a security property, maybe we can emphasis that this is both a security property and system property, and if it’s just for one time use, it’s better to use a system property.
BTW, does the existing value “hostInfo” of the property have a similar problem?
Thanks Max
在 2018年9月8日，21:42，Sean Mullan <sean.mullan@oracle.com> 写 道：
On 9/7/18 7:58 PM, Weijun Wang wrote: In my understanding, the author deliberately wants to show the absolute paths when there are multiple jar files with the same name (Ex: a jar hell).
If you are very familiar with a particular application and understand the risks associated with running it, then I agree that is ok. But security properties apply to all applications using that JDK, and so I don't always think it is practical for an admin to understand every type of application that may be using that JDK and whether or not enabling this property presents a risk.
Maybe we can add some more detail in the java.security so an admin knows what exact impact it has.
It would be a slippery slope in my opinion. You would have to say something like: "enabling this property may allow untrusted code running under a SecurityManager to access sensitive information such as the user.home system property even if it has not been granted permission to do so."
I think we should consider not allowing this property to take effect if a SecurityManager is running.
--Sean
--Max
On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
On 8/29/18 10:01 AM, Baesken, Matthias wrote: > Hi Max, thanks for your input . > I created another webrev , this contains now the suggested SecurityProperties class : > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
java/util/jar/Attributes.java
469 return AccessController.doPrivileged(new PrivilegedAction<String>() { 470 public String run() { 471 return file.getAbsolutePath() + ":" + lineNumber; 472 } 473 });
I have a serious concern with the code above.
With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible.
In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property.
The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path).
--Sean
On Sep 10, 2018, at 9:59 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
New webrev :
Looks fine to me.
- SocketExceptions class has been adjusted to new sun.security.util.SecurityProperties - Attributes getErrorPosition adjusted (see proposal of Christoph " I think it would be enough to drop the privileged section and just return "filename" as is.)
After the changes I wonder - should the jarPath category be renamed to jarFile (or something else) ?
jarFile seems to be an instance of JarFile. I don't think jarPath has any problem. Thanks Max
Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Montag, 10. September 2018 10:04 To: Baesken, Matthias <matthias.baesken@sap.com>; Wang Weijun <weijun.wang@oracle.com>; Sean Mullan <sean.mullan@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Matthias,
I think it would be enough to drop the privileged section and just return "filename" as is. (without conveting to a file object).
I also agree with Sean that the common parts of the new src/java.base/share/classes/sun/security/util/SecurityProperties.java should be aligned with src/java.base/share/classes/sun/net/util/SocketExceptions.java (That is, SocketExceptions calling SecurityProperties)
Best regards Christoph
-----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Baesken, Matthias Sent: Montag, 10. September 2018 09:53 To: Wang Weijun <weijun.wang@oracle.com>; Sean Mullan <sean.mullan@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: [CAUTION] RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello are you fine with changing from file.getAbsolutePath() to file.getName() ?
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/src/java.base/s
hare/classes/java/util/jar/Manifest.java.frames.html
206 static String getErrorPosition(String filename, final int lineNumber) { 207 if (filename == null || !jarPathInExceptionText) { 208 return "line " + lineNumber; 209 } 210 211 final File file = new File(filename); 212 return AccessController.doPrivileged(new PrivilegedAction<String>() { 213 public String run() { 214 return "manifest of " + file.getName() + ":" + lineNumber; 215 }
Best regards, Matthias
-----Original Message----- From: Wang Weijun <weijun.wang@oracle.com> Sent: Samstag, 8. September 2018 17:43 To: Sean Mullan <sean.mullan@oracle.com> Cc: Baesken, Matthias <matthias.baesken@sap.com>; Alan Bateman <Alan.Bateman@oracle.com>; Chris Hegarty <chris.hegarty@oracle.com>; security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Thinking about this again. Looks like the absolute path is not necessary. Even if there are multiple files using the same name, they will be in different directories, no matter absolute or relative. Suppose the jarPath info is used for debugging purpose mostly like the developer can find out what the current working directory is and can find the files. *Matthias*: Is the absolute path really necessary? Are you working on actual case?
As for the possible global effect of a security property, maybe we can emphasis that this is both a security property and system property, and if it’s just for one time use, it’s better to use a system property.
BTW, does the existing value “hostInfo” of the property have a similar problem?
Thanks Max
在 2018年9月8日，21:42，Sean Mullan <sean.mullan@oracle.com> 写 道：
On 9/7/18 7:58 PM, Weijun Wang wrote: In my understanding, the author deliberately wants to show the absolute paths when there are multiple jar files with the same name (Ex: a jar hell).
If you are very familiar with a particular application and understand the risks associated with running it, then I agree that is ok. But security properties apply to all applications using that JDK, and so I don't always think it is practical for an admin to understand every type of application that may be using that JDK and whether or not enabling this property presents a risk.
Maybe we can add some more detail in the java.security so an admin knows what exact impact it has.
It would be a slippery slope in my opinion. You would have to say something like: "enabling this property may allow untrusted code running under a SecurityManager to access sensitive information such as the user.home system property even if it has not been granted permission to do so."
I think we should consider not allowing this property to take effect if a SecurityManager is running.
--Sean
--Max > On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan@oracle.com> wrote: > > On 8/29/18 10:01 AM, Baesken, Matthias wrote: >> Hi Max, thanks for your input . >> I created another webrev , this contains now the suggested SecurityProperties class : >> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/ > > java/util/jar/Attributes.java > > 469 return AccessController.doPrivileged(new PrivilegedAction<String>() { > 470 public String run() { > 471 return file.getAbsolutePath() + ":" + lineNumber; > 472 } > 473 }); > > I have a serious concern with the code above. > > With this change, untrusted code running under a SecurityManager could potentially create a JarFile on a non-absolute path ex: "foo.jar", and (somehow) cause an IOException to be thrown which would then reveal the absolute path of where the jar was located, and thus could reveal sensitive details about the system (ex: the user's home directory). It could still be hard to exploit, since it would have to be a known jar that exists, and you would then need to cause an IOException to be thrown, but it's still theoretically possible. > > In short, this is a more general issue in that it may allow untrusted code to access something it couldn't have previously. new JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path. > > Granted this can only be done if the security property is enabled, but I believe this is not something administrators should have to know about their environment and account for when enabling this property. > > The above code should be changed to return only what the caller provides to JarFile, which is the name of the file (without the full path). > > --Sean
On 9/10/18 9:59 AM, Baesken, Matthias wrote:
New webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.9/
- SocketExceptions class has been adjusted to new sun.security.util.SecurityProperties - Attributes getErrorPosition adjusted (see proposal of Christoph " I think it would be enough to drop the privileged section and just return "filename" as is.)
After the changes I wonder - should the jarPath category be renamed to jarFile (or something else) ?
Yes, renaming it to "jarFile" makes more sense. You will need to update the CSR with this change too. A few other comments: - java/util/jar/Manifest.java 62 // name of the corresponding jar archive if available. 63 private String jarFilename; You can mark this final. - sun/net/util/SocketExceptions.java 33 import java.security.Security; This import is no longer needed. --Sean
On Sep 10, 2018, at 10:35 PM, Sean Mullan <sean.mullan@oracle.com> wrote:
After the changes I wonder - should the jarPath category be renamed to jarFile (or something else) ?
Yes, renaming it to "jarFile" makes more sense. You will need to update the CSR with this change too.
Well, maybe either is OK. I am still thinking that one day we could make this more precise, for example: /path/to/lib.jar!META-INF/MANIFEST.MF:23 /path/to/lib.jar!META-INF/SIGNER.SF:34 The name should (at least vaguely) cover these info. --Max
Hi Sean, Max, do you think we need property jdk.includeInExceptions=jar<File/Path> at all, if we don't resolve the absolute path? @Matthias: small remark to the code: src/java.base/share/classes/sun/security/util/SecurityProperties.java 36 public static String privilegeGetOverridable(String propName) { Should that method really be public? At the moment it doesn't seem to be used outside of SecurityProperties. Best regards Christoph
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Montag, 10. September 2018 16:43 To: Sean Mullan <sean.mullan@oracle.com> Cc: Baesken, Matthias <matthias.baesken@sap.com>; Langer, Christoph <christoph.langer@sap.com>; security-dev@openjdk.java.net; core-libs- dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On Sep 10, 2018, at 10:35 PM, Sean Mullan <sean.mullan@oracle.com> wrote:
After the changes I wonder - should the jarPath category be renamed to jarFile (or something else) ?
Yes, renaming it to "jarFile" makes more sense. You will need to update the CSR with this change too.
Well, maybe either is OK.
I am still thinking that one day we could make this more precise, for example:
/path/to/lib.jar!META-INF/MANIFEST.MF:23 /path/to/lib.jar!META-INF/SIGNER.SF:34
The name should (at least vaguely) cover these info.
--Max
On Sep 10, 2018, at 10:46 PM, Langer, Christoph <christoph.langer@sap.com> wrote:
Hi Sean, Max,
do you think we need property jdk.includeInExceptions=jar<File/Path> at all, if we don't resolve the absolute path?
I think so. File path is still sensitive. In fact, I tend to believe people usually use absolute paths for JAR files (or maybe made absolute by using a file:// URL somewhere inside JDK). Do you really see relative jar paths while testing this code change?
@Matthias: small remark to the code: src/java.base/share/classes/sun/security/util/SecurityProperties.java 36 public static String privilegeGetOverridable(String propName) {
Should that method really be public? At the moment it doesn't seem to be used outside of SecurityProperties.
I like it to be public. There are quite some other such system/security properties (Ex: jdk.serialFilter) that can make use of this method. Thanks Max
Best regards Christoph
Hi,
do you think we need property jdk.includeInExceptions=jar<File/Path> at all, if we don't resolve the absolute path?
I think so. File path is still sensitive.
In fact, I tend to believe people usually use absolute paths for JAR files (or maybe made absolute by using a file:// URL somewhere inside JDK). Do you really see relative jar paths while testing this code change?
I agree.
small remark to the code: src/java.base/share/classes/sun/security/util/SecurityProperties.java 36 public static String privilegeGetOverridable(String propName) {
Should that method really be public? At the moment it doesn't seem to be used outside of SecurityProperties.
I like it to be public. There are quite some other such system/security properties (Ex: jdk.serialFilter) that can make use of this method.
Ok, maybe it should be named "priviledgedGetOverridable" then. @Matthias: Further small cleanups, as you touch the files: src/java.base/share/classes/java/util/jar/Manifest.java: line 35: you can remove "import java.util.Iterator;" src/java.base/share/classes/sun/net/util/SocketExceptions.java: line 41: indentation is ridiculously long, I think it should be 8 chars src/java.base/share/classes/sun/security/util/SecurityProperties.java: Indentation of lines 38-45 is too deep, should be 12. The 2 methods could use a brief Javadoc. Best regards Christoph
On Sep 11, 2018, at 3:29 AM, Langer, Christoph <christoph.langer@sap.com> wrote:
Ok, maybe it should be named "priviledgedGetOverridable" then.
Ah yes. My mistake.
On 9/10/18 5:13 PM, Weijun Wang wrote:
On Sep 11, 2018, at 3:29 AM, Langer, Christoph <christoph.langer@sap.com> wrote:
Ok, maybe it should be named "priviledgedGetOverridable" then.
Ah yes. My mistake.
Small spelling nit: there's no "d" before "g", so this should likely be privilegedGetOverridable s'marks
Hello, please check the new webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.10/ I kept the jarPath category name . Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Montag, 10. September 2018 21:30 To: Weijun Wang <weijun.wang@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: Sean Mullan <sean.mullan@oracle.com>; security- dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi,
do you think we need property jdk.includeInExceptions=jar<File/Path> at all, if we don't resolve the absolute path?
I think so. File path is still sensitive.
In fact, I tend to believe people usually use absolute paths for JAR files (or maybe made absolute by using a file:// URL somewhere inside JDK). Do you really see relative jar paths while testing this code change?
I agree.
small remark to the code: src/java.base/share/classes/sun/security/util/SecurityProperties.java 36 public static String privilegeGetOverridable(String propName) {
Should that method really be public? At the moment it doesn't seem to be used outside of SecurityProperties.
I like it to be public. There are quite some other such system/security properties (Ex: jdk.serialFilter) that can make use of this method.
Ok, maybe it should be named "priviledgedGetOverridable" then.
@Matthias: Further small cleanups, as you touch the files: src/java.base/share/classes/java/util/jar/Manifest.java: line 35: you can remove "import java.util.Iterator;"
src/java.base/share/classes/sun/net/util/SocketExceptions.java: line 41: indentation is ridiculously long, I think it should be 8 chars
src/java.base/share/classes/sun/security/util/SecurityProperties.java: Indentation of lines 38-45 is too deep, should be 12. The 2 methods could use a brief Javadoc.
Best regards Christoph
Attributes.java: - Line 377: Too long, add a break. Otherwise fine. I don't have a strong opinion on making Manifest.jarFilename final or a different property name. Thanks Max
On Sep 11, 2018, at 5:07 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hello, please check the new webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.10/
I kept the jarPath category name .
Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Montag, 10. September 2018 21:30 To: Weijun Wang <weijun.wang@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: Sean Mullan <sean.mullan@oracle.com>; security- dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi,
do you think we need property jdk.includeInExceptions=jar<File/Path> at all, if we don't resolve the absolute path?
I think so. File path is still sensitive.
In fact, I tend to believe people usually use absolute paths for JAR files (or maybe made absolute by using a file:// URL somewhere inside JDK). Do you really see relative jar paths while testing this code change?
I agree.
small remark to the code: src/java.base/share/classes/sun/security/util/SecurityProperties.java 36 public static String privilegeGetOverridable(String propName) {
Should that method really be public? At the moment it doesn't seem to be used outside of SecurityProperties.
I like it to be public. There are quite some other such system/security properties (Ex: jdk.serialFilter) that can make use of this method.
Ok, maybe it should be named "priviledgedGetOverridable" then.
@Matthias: Further small cleanups, as you touch the files: src/java.base/share/classes/java/util/jar/Manifest.java: line 35: you can remove "import java.util.Iterator;"
src/java.base/share/classes/sun/net/util/SocketExceptions.java: line 41: indentation is ridiculously long, I think it should be 8 chars
src/java.base/share/classes/sun/security/util/SecurityProperties.java: Indentation of lines 38-45 is too deep, should be 12. The 2 methods could use a brief Javadoc.
Best regards Christoph
I don't have a strong opinion on making Manifest.jarFilename final
Hi, just making it final leads to compile errors anyway. Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Dienstag, 11. September 2018 13:04 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Langer, Christoph <christoph.langer@sap.com>; Sean Mullan <sean.mullan@oracle.com>; security-dev@openjdk.java.net; core-libs- dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Attributes.java:
- Line 377: Too long, add a break.
Otherwise fine.
I don't have a strong opinion on making Manifest.jarFilename final or a different property name.
Thanks Max
On Sep 11, 2018, at 5:07 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hello, please check the new webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.10/
I kept the jarPath category name .
Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Montag, 10. September 2018 21:30 To: Weijun Wang <weijun.wang@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: Sean Mullan <sean.mullan@oracle.com>; security- dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi,
do you think we need property jdk.includeInExceptions=jar<File/Path> at all, if we don't resolve the absolute path?
I think so. File path is still sensitive.
In fact, I tend to believe people usually use absolute paths for JAR files (or maybe made absolute by using a file:// URL somewhere inside JDK). Do you really see relative jar paths while testing this code change?
I agree.
small remark to the code: src/java.base/share/classes/sun/security/util/SecurityProperties.java 36 public static String privilegeGetOverridable(String propName) {
Should that method really be public? At the moment it doesn't seem to be used outside of SecurityProperties.
I like it to be public. There are quite some other such system/security properties (Ex: jdk.serialFilter) that can make use of this method.
Ok, maybe it should be named "priviledgedGetOverridable" then.
@Matthias: Further small cleanups, as you touch the files: src/java.base/share/classes/java/util/jar/Manifest.java: line 35: you can remove "import java.util.Iterator;"
src/java.base/share/classes/sun/net/util/SocketExceptions.java: line 41: indentation is ridiculously long, I think it should be 8 chars
src/java.base/share/classes/sun/security/util/SecurityProperties.java: Indentation of lines 38-45 is too deep, should be 12. The 2 methods could use a brief Javadoc.
Best regards Christoph
Hi, first of all, I suggest to use "jarDetails" instead of "jarPath" as category name. Because with this contribution we add the notion of jar file plus line of manifest to Exceptions occurring when parsing jar manifests. And if there were further Exception details to be added in the area of jar files, they could go under a category "jarDetails" as well. Would you agree? Then, in Manifest.java, the field "jarPathInExceptionText" could be renamed to "detailedExceptions". As for the code, I have the following remarks: src/java.base/share/classes/java/util/jar/Manifest.java: You could further clean up the ordering of includes by sorting them alphabetically and add a blank line between lines 34/35. Line 52: I suggest an indentation of 8 chars Please use jarFilename as final. You'll have to initialize jarFilename in each constructor then or initialize it to null in the default constructor and call this constructor from all the other ctors except for the one taking the jarFilename as param. src/java.base/share/classes/sun/net/util/SocketExceptions.java please add an empty line between 32 and 33 Line 39: I suggest an indentation of 8 chars src/java.base/share/classes/sun/security/util/SecurityProperties.java Line 35: change comment opener to /** (from /*), then it's a real Javadoc Furthermore I suggest to change the wording to "Returns the value of the security property propName, which can be overridden by a system property of the same name" Best regards Christoph
-----Original Message----- From: Baesken, Matthias Sent: Dienstag, 11. September 2018 13:29 To: Weijun Wang <weijun.wang@oracle.com> Cc: Langer, Christoph <christoph.langer@sap.com>; Sean Mullan <sean.mullan@oracle.com>; security-dev@openjdk.java.net; core-libs- dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
I don't have a strong opinion on making Manifest.jarFilename final
Hi, just making it final leads to compile errors anyway.
Best regards, Matthias
-----Original Message----- From: Weijun Wang <weijun.wang@oracle.com> Sent: Dienstag, 11. September 2018 13:04 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Langer, Christoph <christoph.langer@sap.com>; Sean Mullan <sean.mullan@oracle.com>; security-dev@openjdk.java.net; core-libs- dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Attributes.java:
- Line 377: Too long, add a break.
Otherwise fine.
I don't have a strong opinion on making Manifest.jarFilename final or a different property name.
Thanks Max
On Sep 11, 2018, at 5:07 PM, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Hello, please check the new webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.10/
I kept the jarPath category name .
Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Montag, 10. September 2018 21:30 To: Weijun Wang <weijun.wang@oracle.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: Sean Mullan <sean.mullan@oracle.com>; security- dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi,
do you think we need property jdk.includeInExceptions=jar<File/Path> at all, if we don't resolve the absolute path?
I think so. File path is still sensitive.
In fact, I tend to believe people usually use absolute paths for JAR files (or maybe made absolute by using a file:// URL somewhere inside JDK). Do you really see relative jar paths while testing this code change?
I agree.
small remark to the code: src/java.base/share/classes/sun/security/util/SecurityProperties.java 36 public static String privilegeGetOverridable(String propName) {
Should that method really be public? At the moment it doesn't seem to be used outside of SecurityProperties.
I like it to be public. There are quite some other such system/security properties (Ex: jdk.serialFilter) that can make use of this method.
Ok, maybe it should be named "priviledgedGetOverridable" then.
@Matthias: Further small cleanups, as you touch the files: src/java.base/share/classes/java/util/jar/Manifest.java: line 35: you can remove "import java.util.Iterator;"
src/java.base/share/classes/sun/net/util/SocketExceptions.java: line 41: indentation is ridiculously long, I think it should be 8 chars
src/java.base/share/classes/sun/security/util/SecurityProperties.java: Indentation of lines 38-45 is too deep, should be 12. The 2 methods could use a brief Javadoc.
Best regards Christoph
On 9/11/18 8:14 AM, Langer, Christoph wrote:
Hi,
first of all, I suggest to use "jarDetails" instead of "jarPath" as category name. Because with this contribution we add the notion of jar file plus line of manifest to Exceptions occurring when parsing jar manifests. And if there were further Exception details to be added in the area of jar files, they could go under a category "jarDetails" as well. Would you agree? Then, in Manifest.java, the field "jarPathInExceptionText" could be renamed to "detailedExceptions".
Or just "jar" would be my preference. I don't like "jarPath" as that sounds too much like a file pathname to me, which we have now removed. --Sean
Hello I changed it to jar , also added some minor adjustments suggested by Christoph . http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.12/ Regards, Matthias
-----Original Message----- From: Sean Mullan <sean.mullan@oracle.com> Sent: Dienstag, 11. September 2018 20:44 To: Langer, Christoph <christoph.langer@sap.com>; Baesken, Matthias <matthias.baesken@sap.com>; Weijun Wang <weijun.wang@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 9/11/18 8:14 AM, Langer, Christoph wrote:
Hi,
first of all, I suggest to use "jarDetails" instead of "jarPath" as category name. Because with this contribution we add the notion of jar file plus line of manifest to Exceptions occurring when parsing jar manifests. And if there were further Exception details to be added in the area of jar files, they could go under a category "jarDetails" as well. Would you agree? Then, in Manifest.java, the field "jarPathInExceptionText" could be renamed to "detailedExceptions".
Or just "jar" would be my preference. I don't like "jarPath" as that sounds too much like a file pathname to me, which we have now removed.
--Sean
Looks good to me.
-----Original Message----- From: Baesken, Matthias Sent: Mittwoch, 12. September 2018 11:27 To: Sean Mullan <sean.mullan@oracle.com>; Langer, Christoph <christoph.langer@sap.com>; Weijun Wang <weijun.wang@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello I changed it to jar , also added some minor adjustments suggested by Christoph .
http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.12/
Regards, Matthias
-----Original Message----- From: Sean Mullan <sean.mullan@oracle.com> Sent: Dienstag, 11. September 2018 20:44 To: Langer, Christoph <christoph.langer@sap.com>; Baesken, Matthias <matthias.baesken@sap.com>; Weijun Wang <weijun.wang@oracle.com> Cc: security-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 9/11/18 8:14 AM, Langer, Christoph wrote:
Hi,
first of all, I suggest to use "jarDetails" instead of "jarPath" as category name. Because with this contribution we add the notion of jar file plus line of manifest to Exceptions occurring when parsing jar manifests. And if there were further Exception details to be added in the area of jar files, they could go under a category "jarDetails" as well. Would you agree? Then, in Manifest.java, the field "jarPathInExceptionText" could be renamed to "detailedExceptions".
Or just "jar" would be my preference. I don't like "jarPath" as that sounds too much like a file pathname to me, which we have now removed.
--Sean
On 9/8/18 11:42 AM, Wang Weijun wrote:
Thinking about this again. Looks like the absolute path is not necessary. Even if there are multiple files using the same name, they will be in different directories, no matter absolute or relative. Suppose the jarPath info is used for debugging purpose mostly like the developer can find out what the current working directory is and can find the files. *Matthias*: Is the absolute path really necessary? Are you working on actual case?
As for the possible global effect of a security property, maybe we can emphasis that this is both a security property and system property, and if it’s just for one time use, it’s better to use a system property.
BTW, does the existing value “hostInfo” of the property have a similar problem?
No. In that case, the sensitive data (IP address) is provided by the caller, so there is no leakage of sensitive data from trusted code that it is calling. --Sean
On 8/27/18 10:25 AM, Baesken, Matthias wrote:
Will sun.net.util.SocketExceptions be changed to use the supporting class or is that a separate issue?
I think this is a separate issue .
I think we should fix it as part of this issue. It shouldn't be hard and then we don't have to file another issue to fix it. --Sean
On 07/08/2018 16:00, Baesken, Matthias wrote:
Ping .... 😊 , any reviews / comments ? Did we get to a conclusion on whether to have central infrastructure to read/parse the security property? As I recall, this one was originally proposed before the generalization of the networking solution. There are details related to trimming that would be better not to duplicate if possible.
Also, I think one of my comments on the original patch is that we should get the style and line lengths a bit more consistent with the existing code (the patch added excessively long lines for example). -Alan.
Did we get to a conclusion on whether to have central infrastructure to read/parse the security property? As I recall, this one was originally proposed before the generalization of the networking solution. There are details related to trimming that would be better not to duplicate if possible.
Hi Alan / Chris , I am aware of https://bugs.openjdk.java.net/browse/JDK-8207846 where jdk.net.includeInExceptions has been generalized to be used for other use cases than exceptions in the networking area . This has been pushed in the meantine. Could you point me to the other discussion, is there already a webrev posted for this ?
Also, I think one of my comments on the original patch is that we should get the style and line lengths a bit more consistent with the existing code (the patch added excessively long lines for example).
Sure I'll look into it ! Best regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Mittwoch, 8. August 2018 20:30 To: Baesken, Matthias <matthias.baesken@sap.com>; Chris Hegarty <chris.hegarty@oracle.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Langer, Christoph <christoph.langer@sap.com>; OpenJDK Dev list <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 07/08/2018 16:00, Baesken, Matthias wrote:
Ping .... 😊 , any reviews / comments ? Did we get to a conclusion on whether to have central infrastructure to read/parse the security property? As I recall, this one was originally proposed before the generalization of the networking solution. There are details related to trimming that would be better not to duplicate if possible.
Also, I think one of my comments on the original patch is that we should get the style and line lengths a bit more consistent with the existing code (the patch added excessively long lines for example).
-Alan.
Matthias,
On 9 Aug 2018, at 08:07, Baesken, Matthias <matthias.baesken@sap.com> wrote:
Did we get to a conclusion on whether to have central infrastructure to read/parse the security property? As I recall, this one was originally proposed before the generalization of the networking solution. There are details related to trimming that would be better not to duplicate if possible.
Hi Alan / Chris ,
I am aware of
https://bugs.openjdk.java.net/browse/JDK-8207846
where jdk.net.includeInExceptions has been generalized to be used for other use cases than exceptions in the networking area . This has been pushed in the meantine.
Could you point me to the other discussion, is there already a webrev posted for this ?
Given that 8207846 was being targeted to JDK 11, during RDP 1, it was decided to keep the changes as minimal as possible. The supporting implementation, that reads and parses the property, remains in src/java.base/share/classes/sun/net/util/SocketExceptions.java. Alan referred to this in one of the review comments for 8207846 [1]. If this mechanism is to be used outside of the networking area, and I think it can, then the implementation in SocketExceptions should probably be moved to some other more appropriate internal package.
Also, I think one of my comments on the original patch is that we should get the style and line lengths a bit more consistent with the existing code (the patch added excessively long lines for example).
Sure I'll look into it !
-Chris. [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054501.html
Best regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Mittwoch, 8. August 2018 20:30 To: Baesken, Matthias <matthias.baesken@sap.com>; Chris Hegarty <chris.hegarty@oracle.com> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Langer, Christoph <christoph.langer@sap.com>; OpenJDK Dev list <security- dev@openjdk.java.net> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 07/08/2018 16:00, Baesken, Matthias wrote:
Ping .... 😊 , any reviews / comments ? Did we get to a conclusion on whether to have central infrastructure to read/parse the security property? As I recall, this one was originally proposed before the generalization of the networking solution. There are details related to trimming that would be better not to duplicate if possible.
Also, I think one of my comments on the original patch is that we should get the style and line lengths a bit more consistent with the existing code (the patch added excessively long lines for example).
-Alan.
participants (9)
-
Alan Bateman
-
Baesken, Matthias
-
Chris Hegarty
-
Jaikiran Pai
-
Langer, Christoph
-
Sean Mullan
-
Stuart Marks
-
Wang Weijun
-
Weijun Wang