RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Looks good to me. > -Original Message- > From: Baesken, Matthias > Sent: Mittwoch, 12. September 2018 11:27 > To: Sean Mullan ; Langer, Christoph > ; Weijun Wang > Cc: security-...@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 > > Sent: Dienstag, 11. September 2018 20:44 > > To: Langer, Christoph ; Baesken, Matthias > > ; Weijun Wang > > Cc: security-...@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
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 > Sent: Dienstag, 11. September 2018 20:44 > To: Langer, Christoph ; Baesken, Matthias > ; Weijun Wang > Cc: security-...@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
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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
> Cc: Langer, Christoph ; Sean Mullan
> ; security-...@openjdk.java.net; core-libs-
> d...@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
> > Sent: Dienstag, 11. September 2018 13:04
> > To: Baesken, Matthias
> > Cc: Langer, Christoph ; Sean Mullan
> > ; security-...@openjdk.java.net; core-libs-
> > d...@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
> > 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 ; Baesken, Matthias
> > >>
> > >> Cc: Sean Mullan ; security-
> > >> d...@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
> > 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
> > >
>
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
> Sent: Dienstag, 11. September 2018 13:04
> To: Baesken, Matthias
> Cc: Langer, Christoph ; Sean Mullan
> ; security-...@openjdk.java.net; core-libs-
> d...@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
> 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 ; Baesken, Matthias
> >>
> >> Cc: Sean Mullan ; security-
> >> d...@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
> 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
> >
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
> 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 ; Baesken, Matthias
>>
>> Cc: Sean Mullan ; security-
>> d...@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 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
>
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 ; Baesken, Matthias
>
> Cc: Sean Mullan ; security-
> d...@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 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 9/10/18 5:13 PM, Weijun Wang wrote: On Sep 11, 2018, at 3:29 AM, Langer, Christoph 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On Sep 11, 2018, at 3:29 AM, Langer, Christoph > wrote: > > Ok, maybe it should be named "priviledgedGetOverridable" then. Ah yes. My mistake.
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi,
> > do you think we need property jdk.includeInExceptions=jar 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On Sep 10, 2018, at 10:46 PM, Langer, Christoph
> wrote:
>
> Hi Sean, Max,
>
> do you think we need property jdk.includeInExceptions=jar 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Sean, Max,
do you think we need property jdk.includeInExceptions=jar 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
> Sent: Montag, 10. September 2018 16:43
> To: Sean Mullan
> Cc: Baesken, Matthias ; Langer, Christoph
> ; security-...@openjdk.java.net; core-libs-
> d...@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
> 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On Sep 10, 2018, at 10:35 PM, Sean Mullan 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On Sep 10, 2018, at 9:59 PM, Baesken, Matthias
> wrote:
>
> New webrev :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.9/
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 ; Wang Weijun
>> ; Sean Mullan
>> Cc: security-...@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 On Behalf
>>> Of Baesken, Matthias
>>> Sent: Montag, 10. September 2018 09:53
>>> To: Wang Weijun ; Sean Mullan
>>>
>>> Cc: security-...@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()
>>> {
>>> 213 public String run() {
>>> 214 return "manifest of " + file.getName() + ":" +
>>> lineNumber;
>>> 215 }
>>>
>>>
>>> Best regards, Matthias
>>>
>>>
>>>> -Original Message-
>>>> From: Wang Weijun
>>>> Sent: Samstag, 8. September 2018 17:43
>>>> To: Sean Mullan
>>>> Cc: Baesken, Matthias ; Alan Bateman
>>>> ; Chris Hegarty
>> ;
>>>> security-...@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
>> 写
>>>> 道：
>>>>>>
>>>>&
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 ; Wang Weijun
> ; Sean Mullan
> Cc: security-...@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 On Behalf
> > Of Baesken, Matthias
> > Sent: Montag, 10. September 2018 09:53
> > To: Wang Weijun ; Sean Mullan
> >
> > Cc: security-...@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()
> > {
> > 213 public String run() {
> > 214 return "manifest of " + file.getName() + ":" +
> > lineNumber;
> > 215 }
> >
> >
> > Best regards, Matthias
> >
> >
> > > -Original Message-
> > > From: Wang Weijun
> > > Sent: Samstag, 8. September 2018 17:43
> > > To: Sean Mullan
> > > Cc: Baesken, Matthias ; Alan Bateman
> > > ; Chris Hegarty
> ;
> > > security-...@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
> 写
> > > 道：
> > > >>
> > > >> 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> 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 ; Wang Weijun
> ; Sean Mullan
> Cc: security-...@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 On Behalf
> > Of Baesken, Matthias
> > Sent: Montag, 10. September 2018 09:53
> > To: Wang Weijun ; Sean Mullan
> >
> > Cc: security-...@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()
> > {
> > 213 public String run() {
> > 214 return "manifest of " + file.getName() + ":" +
> > lineNumber;
> > 215 }
> >
> >
> > Best regards, Matthias
> >
> >
> > > -Original Message-
> > > From: Wang Weijun
> > > Sent: Samstag, 8. September 2018 17:43
> > > To: Sean Mullan
> > > Cc: Baesken, Matthias ; Alan Bateman
> > > ; Chris Hegarty
> ;
> > > security-...@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
> 写
> > > 道：
> > > >>
> > > >> 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
> > >
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 On Behalf
> Of Baesken, Matthias
> Sent: Montag, 10. September 2018 09:53
> To: Wang Weijun ; Sean Mullan
>
> Cc: security-...@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()
> {
> 213 public String run() {
> 214 return "manifest of " + file.getName() + ":" +
> lineNumber;
> 215 }
>
>
> Best regards, Matthias
>
>
> > -Original Message-
> > From: Wang Weijun
> > Sent: Samstag, 8. September 2018 17:43
> > To: Sean Mullan
> > Cc: Baesken, Matthias ; Alan Bateman
> > ; Chris Hegarty ;
> > security-...@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 写
> > 道：
> > >>
> > >> 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
> > 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/~mba
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/share/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() {
213 public String run() {
214 return "manifest of " + file.getName() + ":" + lineNumber;
215 }
Best regards, Matthias
> -Original Message-
> From: Wang Weijun
> Sent: Samstag, 8. September 2018 17:43
> To: Sean Mullan
> Cc: Baesken, Matthias ; Alan Bateman
> ; Chris Hegarty ;
> security-...@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 写
> 道：
> >>
> >> 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
> 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() {
> >>> 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
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 写道：
>>
>> 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 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() {
>>> 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 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()
{
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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 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() {
> 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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() {
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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Matthias, I have reviewed your change, +1 I also reviewed the CSR. Best regards Christoph > -Original Message- > From: Baesken, Matthias > Sent: Mittwoch, 5. September 2018 10:07 > To: security-...@openjdk.java.net; Weijun Wang > ; core-libs-dev@openjdk.java.net > Cc: Langer, Christoph > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > parsing of jar archives > > Hi Max, thanks for adding yourself as a reviewer. > > I set the CSR ( https://bugs.openjdk.java.net/browse/JDK-8207768 ) to > proposed. > > Best regards, Matthias > > > > > Message: 2 > > Date: Tue, 4 Sep 2018 21:31:58 +0800 > > From: Weijun Wang > > To: "Baesken, Matthias" > > Cc: "security-...@openjdk.java.net" , > > "core-libs-dev@openjdk.java.net" > d...@openjdk.java.net> > > Subject: Re: [RFR] 8205525 : Improve exception messages during > > manifest parsing of jar archives > > Message-ID: <058bd7b5-4d3a-4b56-acb0-0dedddea2...@oracle.com> > > Content-Type: text/plain; charset=us-ascii > > > > 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 > > 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Max, thanks for adding yourself as a reviewer. I set the CSR ( https://bugs.openjdk.java.net/browse/JDK-8207768 ) to proposed. Best regards, Matthias > > Message: 2 > Date: Tue, 4 Sep 2018 21:31:58 +0800 > From: Weijun Wang > To: "Baesken, Matthias" > Cc: "security-...@openjdk.java.net" , > "core-libs-dev@openjdk.java.net" d...@openjdk.java.net> > Subject: Re: [RFR] 8205525 : Improve exception messages during > manifest parsing of jar archives > Message-ID: <058bd7b5-4d3a-4b56-acb0-0dedddea2...@oracle.com> > Content-Type: text/plain; charset=us-ascii > > 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 > 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
> 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
>> Sent: Montag, 3. September 2018 17:14
>> To: Baesken, Matthias
>> Cc: Alan Bateman ; Sean Mullan
>> ; Chris Hegarty ;
>> security-...@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
>> 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
>>>> Sent: Freitag, 31. August 2018 15:53
>>>> To: Baesken, Matthias
>>>> Cc: Alan Bateman ; Sean Mullan
>>>> ; Chris Hegarty ;
>>>> security-...@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
>>>> 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 : (e.g. test.jar:10 )
>>>>>
>>>>> Or ( to mention that it is about a manifest from the jar )
>>>>>
>>>>> :manifest-line(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 fi
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> 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
> Sent: Montag, 3. September 2018 17:14
> To: Baesken, Matthias
> Cc: Alan Bateman ; Sean Mullan
> ; Chris Hegarty ;
> security-...@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
> 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
> >> Sent: Freitag, 31. August 2018 15:53
> >> To: Baesken, Matthias
> >> Cc: Alan Bateman ; Sean Mullan
> >> ; Chris Hegarty ;
> >> security-...@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
> >> 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 : (e.g. test.jar:10 )
> >>>
> >>> Or ( to mention that it is about a manifest from the jar )
> >>>
> >>> :manifest-line(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
> >>>> Sent: Freitag, 31. August 2018 04:32
> >>>> To: Baesken, Matthias
> >>>> Cc: Alan Bateman ; Sean Mullan
> >>>> ; Chris Hegarty
> ;
> >>>> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> >>>> Subject: Re: [RFR] 8205525 : Improve exception messages during
> manifest
> >>>> parsing of jar archives
> >>>>
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
> 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
>> Sent: Freitag, 31. August 2018 15:53
>> To: Baesken, Matthias
>> Cc: Alan Bateman ; Sean Mullan
>> ; Chris Hegarty ;
>> security-...@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
>> 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 : (e.g. test.jar:10 )
>>>
>>> Or ( to mention that it is about a manifest from the jar )
>>>
>>> :manifest-line(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
>>>> Sent: Freitag, 31. August 2018 04:32
>>>> To: Baesken, Matthias
>>>> Cc: Alan Bateman ; Sean Mullan
>>>> ; Chris Hegarty ;
>>>> security-...@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
>>>> wrote:
>>>>>
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
> Sent: Freitag, 31. August 2018 15:53
> To: Baesken, Matthias
> Cc: Alan Bateman ; Sean Mullan
> ; Chris Hegarty ;
> security-...@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
> 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 : (e.g. test.jar:10 )
> >
> > Or ( to mention that it is about a manifest from the jar )
> >
> > :manifest-line(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
> >> Sent: Freitag, 31. August 2018 04:32
> >> To: Baesken, Matthias
> >> Cc: Alan Bateman ; Sean Mullan
> >> ; Chris Hegarty ;
> >> security-...@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
> >> 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On Aug 31, 2018, at 8:52 PM, Baesken, Matthias
> 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 : (e.g. test.jar:10 )
>
> Or ( to mention that it is about a manifest from the jar )
>
> :manifest-line(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
>> Sent: Freitag, 31. August 2018 04:32
>> To: Baesken, Matthias
>> Cc: Alan Bateman ; Sean Mullan
>> ; Chris Hegarty ;
>> security-...@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
>> 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() {
>>> 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
>>
>>>}
>>> .
>>>
>>>
>
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 : (e.g. test.jar:10 )
Or ( to mention that it is about a manifest from the jar )
:manifest-line(e.g. test.jar:manifest-line 10 )
Best regards, Matthias
> -Original Message-
> From: Weijun Wang
> Sent: Freitag, 31. August 2018 04:32
> To: Baesken, Matthias
> Cc: Alan Bateman ; Sean Mullan
> ; Chris Hegarty ;
> security-...@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
> 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() {
> >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
> >> Sent: Donnerstag, 30. August 2018 16:04
> >> To: Baesken, Matthias
> >> Cc: Alan Bateman ; Sean Mullan
> >> ; Chris Hegarty ;
> >> security-...@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
> >> 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Or you can smuggle it out through JavaUtilJarAccess with SharedSecrets. > On Aug 31, 2018, at 10:32 AM, Weijun Wang 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)?
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
> 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/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() {
>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
>> Sent: Donnerstag, 30. August 2018 16:04
>> To: Baesken, Matthias
>> Cc: Alan Bateman ; Sean Mullan
>> ; Chris Hegarty ;
>> security-...@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
>> 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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/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() {
public String run() {
return file.getAbsolutePath() + "!META-INF/MANIFEST.MF line:"
+ lineNumber;
}
.
Best regards, Matthias
> -Original Message-
> From: Weijun Wang
> Sent: Donnerstag, 30. August 2018 16:04
> To: Baesken, Matthias
> Cc: Alan Bateman ; Sean Mullan
> ; Chris Hegarty ;
> security-...@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
> 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On Aug 30, 2018, at 8:26 PM, Baesken, Matthias > 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
> Sent: Mittwoch, 29. August 2018 16:27
> To: Baesken, Matthias
> Cc: Alan Bateman ; Sean Mullan
> ; Chris Hegarty ;
> security-...@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
> 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
> >> Sent: Montag, 27. August 2018 17:35
> >> To: Baesken, Matthias
> >> Cc: Alan Bateman ; Sean Mullan
> >> ; Chris Hegarty ;
> >> security-...@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
> >> 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 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;
> >> }
>
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
> 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
>> Sent: Montag, 27. August 2018 17:35
>> To: Baesken, Matthias
>> Cc: Alan Bateman ; Sean Mullan
>> ; Chris Hegarty ;
>> security-...@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
>> 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 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
>>>> Sent: Montag, 27. August 2018 15:52
>>>> To: Baesken, Matthias ; Sean Mullan
>>>> ; Chris Hegarty
>>>> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK >>> d...@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 jarFilenamein 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
>
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
> Sent: Montag, 27. August 2018 17:35
> To: Baesken, Matthias
> Cc: Alan Bateman ; Sean Mullan
> ; Chris Hegarty ;
> security-...@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
> 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 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
> >> Sent: Montag, 27. August 2018 15:52
> >> To: Baesken, Matthias ; Sean Mullan
> >> ; Chris Hegarty
> >> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK >> d...@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 jarFilenamein 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On Aug 27, 2018, at 10:25 PM, Baesken, Matthias
> 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 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
>> Sent: Montag, 27. August 2018 15:52
>> To: Baesken, Matthias ; Sean Mullan
>> ; Chris Hegarty
>> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK > d...@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 jarFilenamein 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 > Sent: Montag, 27. August 2018 15:52 > To: Baesken, Matthias ; Sean Mullan > ; Chris Hegarty > Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK d...@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 jarFilenamein 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
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 jarFilenamein 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 jarFilenamein the Manifest constructor
Best regards , Matthias
> -Original Message-
> From: Sean Mullan
> Sent: Freitag, 10. August 2018 17:39
> To: Baesken, Matthias ; Chris Hegarty
> ; Alan Bateman
> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK d...@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' ; Alan Bateman
> >>>
> >>> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz
> >>> ; Langer, Christoph
> >>>
> >>> 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
> >>>> Sent: Donnerstag, 19. Juli 2018 14:54
> >>>> To: Alan Bateman ; Baesken, Matthias
> >>>>
> >>>> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz
> >>>>
> >>>> Subject: Re: [RFR] 8205525 : Improve exception messages during
> manifest
> >>>> parsing of jar archives
> >>>>
> >>>>
> >>>>> On 19 Jul 2018, at 11:46, Alan Bateman
> >>>> 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.
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 parsingdo 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
>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 parsingdo in a followup CR, I
would prefer this .
Best regards, Matthias
> -Original Message-
> From: Sean Mullan
> Sent: Freitag, 10. August 2018 17:39
> To: Baesken, Matthias ; Chris Hegarty
> ; Alan Bateman
> Cc: core-libs-dev@openjdk.java.net; security Dev OpenJDK d...@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' ; Alan Bateman
> >>>
> >>> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz
> >>> ; Langer, Christoph
> >>>
> >>> 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
> >>>> Sent: Donnerstag, 19. Juli 2018 14:54
> >>>> To: Alan Bateman ; Baesken, Matthias
> >>>>
> >>>> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz
> >>>>
> >>>> Subject: Re: [RFR] 8205525 : Improve exception messages during
> manifest
> >>>> parsing of jar archives
> >>>>
> >>>>
> >>>>> On 19 Jul 2018, at 11:46, Alan Bateman
> >>>> 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.
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' ; Alan Bateman
Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz
; Langer, Christoph
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
Sent: Donnerstag, 19. Juli 2018 14:54
To: Alan Bateman ; Baesken, Matthias
Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz
Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
parsing of jar archives
On 19 Jul 2018, at 11:46, Alan Bateman
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.
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Matthias, > On 9 Aug 2018, at 08:07, Baesken, Matthias 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 >> Sent: Mittwoch, 8. August 2018 20:30 >> To: Baesken, Matthias ; Chris Hegarty >> >> Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz >> ; Langer, Christoph >> ; OpenJDK Dev list > d...@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.
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> 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 > Sent: Mittwoch, 8. August 2018 20:30 > To: Baesken, Matthias ; Chris Hegarty > > Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz > ; Langer, Christoph > ; OpenJDK Dev list d...@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.
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.
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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' ; Alan Bateman Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz ; Langer, Christoph 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 Sent: Donnerstag, 19. Juli 2018 14:54 To: Alan Bateman ; Baesken, Matthias Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives On 19 Jul 2018, at 11:46, Alan Bateman 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.
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Ping 😊 , any reviews / comments ? Thanks , Matthias > -Original Message- > From: Baesken, Matthias > Sent: Dienstag, 31. Juli 2018 12:28 > To: 'Chris Hegarty' ; Alan Bateman > > Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz > ; Langer, Christoph > > 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 > > Sent: Donnerstag, 19. Juli 2018 14:54 > > To: Alan Bateman ; Baesken, Matthias > > > > Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz > > > > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > > parsing of jar archives > > > > > > > On 19 Jul 2018, at 11:46, Alan Bateman > > 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.
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 > Sent: Donnerstag, 19. Juli 2018 14:54 > To: Alan Bateman ; Baesken, Matthias > > Cc: core-libs-dev@openjdk.java.net; Lindenmaier, Goetz > > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > parsing of jar archives > > > > On 19 Jul 2018, at 11:46, Alan Bateman > 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.
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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.hega...@oracle.com] > Sent: Freitag, 20. Juli 2018 13:41 > To: Alan Bateman ; Baesken, Matthias > > 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 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.
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On 19 Jul 2018, at 13:54, Chris Hegarty 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.
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On 19 Jul 2018, at 15:42, Sean Mullan 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.
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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 >> 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.bate...@oracle.com] Sent: Mittwoch, 18. Juli 2018 19:44 >>> To: Baesken, Matthias ; core-libs- >>> d...@openjdk.java.net; Lindenmaier, Goetz >>> 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 >
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 7/19/18 8:54 AM, Chris Hegarty wrote: On 19 Jul 2018, at 11:46, Alan Bateman 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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On 19 Jul 2018, at 11:46, Alan Bateman 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.
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> On 19 Jul 2018, at 09:07, Baesken, Matthias 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.bate...@oracle.com] >> Sent: Mittwoch, 18. Juli 2018 19:44 >> To: Baesken, Matthias ; core-libs- >> d...@openjdk.java.net; Lindenmaier, Goetz >> 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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.bate...@oracle.com] > Sent: Mittwoch, 18. Juli 2018 19:44 > To: Baesken, Matthias ; core-libs- > d...@openjdk.java.net; Lindenmaier, Goetz > 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
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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> 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.bate...@oracle.com] > Sent: Dienstag, 17. Juli 2018 13:39 > To: Baesken, Matthias ; core-libs- > d...@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 JaikiranI 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
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 JaikiranI 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hello, after latest comments from Alan and JaikiranI 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' ; core-libs- > d...@openjdk.java.net; 'jai.forums2...@gmail.com' > > Cc: Lindenmaier, Goetz > 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.bate...@oracle.com] > > Sent: Sonntag, 8. Juli 2018 09:36 > > To: Baesken, Matthias ; core-libs- > > d...@openjdk.java.net > > Cc: Lindenmaier, Goetz > > 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 likeJDK-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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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.
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.bate...@oracle.com] > Sent: Sonntag, 8. Juli 2018 09:36 > To: Baesken, Matthias ; core-libs- > d...@openjdk.java.net > Cc: Lindenmaier, Goetz > 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 likeJDK-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
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 likeJDK-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
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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.bate...@oracle.com] Sent: Montag, 25. Juni 2018 16:52 To: Baesken, Matthias ; core-libs- d...@openjdk.java.net Cc: Lindenmaier, Goetz 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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Alan ,so it looks likeJDK-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.bate...@oracle.com] > Sent: Montag, 25. Juni 2018 16:52 > To: Baesken, Matthias ; core-libs- > d...@openjdk.java.net > Cc: Lindenmaier, Goetz > 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
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
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
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.bate...@oracle.com] > Sent: Montag, 25. Juni 2018 16:17 > To: Baesken, Matthias ; core-libs- > d...@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.
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.
