RFR 8210395: Add doc to SecurityTools.java

[Weijun Wang]

I ran javadoc on test/lib so I can learn the useful helper classes, and find 
out SecurityTools.java has no javadoc at all.

Please review this change

   http://cr.openjdk.java.net/~weijun/8210395/webrev.00/

Thanks
Max

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

These sun.security.pkcs11.wrapper classes are internal and subject to 
changes without notice.


For the time being, maybe we can leave these method intact and add a 
comment about calling the new methods which use P11Key argument instead 
of the key handle argument.


Regards,
Valerie

On 9/1/2018 11:18 AM, Michael StJohns wrote:

Hi Valerie -

I may be speaking only for myself, but there are probably a number of 
folk using sun.security.pkcs11.wrapper.* in lieu of the PKCS11 
provider for a number of reasons including an ability to manage the 
key storage and wrapping efficiently.   Looking at this I'm thinking 
there will be a large number of things breaking because of the way you 
refactored things.


While I understand that none of the sun.security.* classes are 
"public" - these were mostly taken directly from the IAIK contribution 
way back when and the folks I worked with used them in lieu of 
external libraries to have a consistent PKCS11 interface java-to-java.


Perhaps instead you'd consider doing something like leaving the Java 
to Native public methods intact?


Mike




On 8/31/2018 7:53 PM, Valerie Peng wrote:


Hi Martin,

With the new model of "creating the key handle as needed", I think we 
should not allow the direct access of keyID field outside of P11Key 
class. This field should be made private and accessed through 
methods. In addition, existing PKCS11.C_XXX() methods with P11 keyID 
arguments can be made private and we can add wrapper methods with 
P11Key object argument to ensure proper usage of the P11Key object 
and its keyID field.


I have also refactored the keyID management code into a separate 
holder class. The prototype code can be found at: 
http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.00/


The main changes that I made are in P11Key.java and PKCS11.java. The 
other java classes are updated to call the wrapper methods in 
PKCS11.java.


Thanks & have a great Labor day weekend,

Valerie


On 8/16/2018 5:28 PM, Valerie Peng wrote:


Hi Martin,


Since there is no regression test for this test, you will need to 
add a "noreg-xxx" label to the bug record. For this issue, it'll 
probably be "noreg-hard".


To understand the changes better, please find my questions and some 
review comments below:



- line 397: It's incorrect to change initialize() to 
ensureInitialized(). If the cipher object were to be initialized 
twice with two different keys, you need to re-initialize again.

- line 471: Why do you change it to Throwable from PKCS11Exception?


- line 99: comment is somewhat unclear. typo: "temporal" should be 
"temporary".
- line 148-149: JDK-8165996 has been fixed. This does not apply now, 
does it?
- You changed the constructors of all the P11Key-related classes to 
take an additional boolean argument "tmpNativeKey". However, this 
boolean argument is only used when the underlying token is "NSS". 
Why limiting to NSS only? It seems that all callers except for 
PKCS11 KeyStore pass true for "tmpNativeKey". When "tmpNativeKey" is 
true, the keyID handle first destroyed in constructor and later 
created again (ref count == 1) and destroyed (when ref count drops 
to 0). This replaced the PhantomReference approach in SessionKeyRef 
class, right? Now both approaches seem to be used and key 
destruction is taking places at different methods. I think we should 
clarify the cleanup model and perhaps refactor the code so it's 
easier which approach is in use. Or grouping all these 
cleanup-related fields/variables into a separate class for 
readability/clarity.
- line 157-175: nativeKeyWrapperKeyID is a static variable, 
shouldn't it be synchronized on a class level object?
- line 1343: the impl doesn't look right. Why do you call both 
removeObject() and addObject() here with the same check?
- line 1363: the change seems incorrect, you should still call 
session.removeObject(). the call setKeyIDAndSession(0, null) does 
not lower the session object count.


Thanks,
Valerie

On 8/7/2018 5:41 PM, Valerie Peng wrote:


Hi Martin,

Thanks for the update, I will resume the review on this one with 
your latest webrev.


BTW, there is no webrev.07 for your other fix, i.e. JDK-8029661, 
right? Just checking.


Regards,
Valerie

On 8/3/2018 2:13 PM, Martin Balao wrote:

Hi Valerie,

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10/ 

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10.zip 



New in webrev 10:

 * Bug fix when private DSA/EC keys are sensitive
  * I found this bug removing "attributes = compatibility" entry 
in NSS configuration file so keys were CKA_SENSITIVE.
  * This is really an NSS bug when unwrapping ephemeral keys 
(NSC_UnwrapKey function), because CKA_NETSCAPE_DB attribute is 
required but not used/needed. There was a similar bug when 
creating objects 

Re: Conceptual feedback on new ECC JEP

[Michael StJohns]

On 9/4/2018 3:19 PM, Adam Petcher wrote:
I think what you are suggesting is that the implementation should 
convert between BigInteger and the internal representation when 
necessary. The problem with this approach is that it is too easy to 
inadvertently supply a BigInteger to the implementation, and this 
would result in a branch. I understand that this branch may be 
acceptable in some circumstances, but we would need something in the 
API to tell the implementation whether it is okay to branch or not. I 
think the simplest way to do that is to have a provider that never 
branches. If branching is okay, then SunEC can be used.


Basically yes.

But I don't understand what you mean by "inadvertently supply a 
BigInteger"?  AFAICT, you "supply" a BigInteger in an ECPrivateKeySpec 
and you retrieve one when you call getEncoded() or getS().    Your 
implementation would convert between the BigInteger and internal 
representation during the use of the engineGeneratePrivate() method of 
the KeyFactorySpi and would convert from your internal representation 
when exporting S, or encoding something as PKCS8.


Again - you're wrongly conflating interface requirements with 
implementation requirements.


And how do you expect to move key material between SunEC and this 
implementation?  See below for my commentary on that.



That's essentially the plan. Calling PrivateKey::getEncoded will 
return null, which is a convention for non-extractable keys. Trying to 
convert from/to an ECPrivateKeySpec using the KeyFactory in the new 
provider will result in an exception---so you won't have an object to 
call getS() on.
That's not what PKCS11 does - it just gives you a "PrivateKey" object 
with an internal type of sun.security.pkcs11.P11Key.  While that object 
is not type safe exactly, it is provider safe.


You're still wanting to use the various EC classes of java.security, 
java.security.spec, and java.security.interfaces, but you're unwilling 
to actually meet their contracts for some really suspect reasons.





To create the key from stored information, the best way is to 
construct a PKCS8EncodedKeySpec using the encoded key. If you are 
starting with a BigInteger, and if branching is acceptable, you can 
use the KeyFactory from SunEC to convert an ECPrivateKeySpec to 
PrivateKey to get the encoded value. 


Umm... what?

If you were doing NewEC -> SunEC manually (getEncoded() -> KeySpec) - 
you'll need to end up emitting a PKCS8 blob using RFC5915, which - 
unsurprisingly has  BigEndian INTEGERs  (yes, its an octet string, but 
the encoding is specified by RFC3447 as pretty much the big endian 
encoding of an integer).  E.g. it may look opaque from Java's point of 
view, but it's not really opaque. (See below)


Or have you got a different way of encoding the PKCS8 blob for the new 
provider?  E.g. point me at a specification please.


My head hurt when I tried to work through the various cases of 
translating a private key from your provider to SunEC or to BouncyCastle 
and vice versa.  Basically, if you don't support the getS() call, then 
KeyFactory.translateKey() will fail.  (Below from 
sun.security.ec.ECKeyFactory.java - the SunEC provider's implementation).



 private PrivateKey implTranslatePrivateKey(PrivateKey key)
    throws InvalidKeyException {
    if (key instanceof ECPrivateKey) {
    if (key instanceof ECPrivateKeyImpl) {
    return key;
    }
    ECPrivateKey ecKey = (ECPrivateKey)key;
    return new ECPrivateKeyImpl(
    ecKey.getS(),
    ecKey.getParams()
    );
    } else if ("PKCS#8".equals(key.getFormat())) {
    return new ECPrivateKeyImpl(key.getEncoded());
    } else {
    throw new InvalidKeyException("Private keys must be instance "
    + "of ECPrivateKey or have PKCS#8 encoding");
    }





  4.1  I2OSP



I2OSP converts a nonnegative integer to an octet string of a
specified length.

I2OSP (x, xLen)

Input:
xnonnegative integer to be converted
xLen intended length of the resulting octet string

Output:
Xcorresponding octet string of length xLen

Error: "integer too large"

Steps:

1. If x >= 256^xLen, output "integer too large" and stop.

2. Write the integer x in its unique xLen-digit representation in
   base 256:

  x = x_(xLen-1) 256^(xLen-1) + x_(xLen-2) 256^(xLen-2) + ...
  + x_1 256 + x_0,

   where 0 <= x_i < 256 (note that one or more leading digits will be
   zero if x is less than 256^(xLen-1)).

3. Let the octet X_i have the integer value x_(xLen-i) for 1 <= i <=
   xLen.  Output the octet string

  X = X_1 X_2 ... X_xLen.

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

[Xuelei Fan]

I have no finished the full code review.  So far, I have a few question 
about the struct of the code.

1. XECParameters
I can see the reason to dynamic parameters for something other than 
X25519/X448.  But for JSSE, currently, only named curves are used.  It 
would be nice to use the name for the static parameters.


2. "TlsPremasterSecret" only XDH key agreement
It would be nice the JCE implementation can support key agreement other 
than TLS protocols and providers other than SunJSSE provider.  It would 
be nice if the JCE implementation does not bind to the SunJSSE provider 
private algorithm name.


We used to use SunJSSE private name in the JCE crypto implementation. 
But there are some known problems with this dependence.


Is there a problem to support generic key agreement?

3. NamedGroupFunctions
It might be more straightforward to define these functions in 
NamedGroup.  If comparing nameGroup.getFunctions().getParameterSpec() 
and namedGroup.getParameterSpec(), I'd prefer the latter.


4. SSLKeyAgreementCredentials
I did not see too much benefit of this new interface.  It is not always 
true that a key agreement could have a public key.  Although we can 
simplify the key agreement for public key based, but it also add 
additional layers.


I know where this improvement comes from.  Maybe, you can consolidate 
the named group functions, and encode/decode the credentials there.


Xuelei


On 8/30/2018 8:58 AM, Adam Petcher wrote:

Webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8171279

Please review the following change to add support for X25519 and X448 
(XDH) to TLS 1.3. This change includes some refactoring to remove code 
that was duplicated for DH and ECDH, and to avoid adding more for XDH. 
In addition to running the included regression test, I tested by 
connecting to an openssl server and confirmed that the connection was 
established using TLS 1.3 and X25519/X448.


Here are some detailed notes:

*) The NamedGroupFunctions class was added to hold the functions that 
are needed for key agreement with some named group. Most of the 
group-specific code was moved into subclasses of NamedGroupFunctions. 
This allowed me to remove a bunch of code like "if (type == ECDHE) {...} 
else if (type == FFDHE) {...}".
*) There are a couple of files in the webrev with no changes due to a 
webrev issue. Please ignore them.
*) I moved some code related to XDH parameters and encoding into 
java.base. ECUtil now has code to encode/decode XDH public keys, and the 
XECParameters file was moved into java.base/sun.security.util. This 
organization is similar to how CurveDB and NamedCurve are in java.base, 
and it allows the TLS implementation to encode/decode keys without using 
the jdk.crypto.ec module.

Re: Conceptual feedback on new ECC JEP

[Adam Petcher]

On 9/4/2018 2:01 PM, Michael StJohns wrote:


Below

*buzz* wrong answer.   Sorry.   The internal storage of the key can be 
anything you want it to be if you want to prevent a non-constant-time 
issue for normal calculation.  But the import/export of the key really 
isn't subject to the cargo cult "must not branch" dogma - hint - I'm 
moving across a security boundary line anyway.    So if I do a 
"getEncoded()" or a "getS()" on an ECPrivateKey object or provide a 
BigInteger on an ECPrivateKeySpec there is no additional threat over 
the presence of the private key bits in public space.


I think what you are suggesting is that the implementation should 
convert between BigInteger and the internal representation when 
necessary. The problem with this approach is that it is too easy to 
inadvertently supply a BigInteger to the implementation, and this would 
result in a branch. I understand that this branch may be acceptable in 
some circumstances, but we would need something in the API to tell the 
implementation whether it is okay to branch or not. I think the simplest 
way to do that is to have a provider that never branches. If branching 
is okay, then SunEC can be used.




If you believe this to be such a problem, then I'd suggest instead 
updating BigInteger to allow for BigEndian or LittleEndian encoding 
input/output and fix the constant time issue there inside the 
implementation rather than breaking public APIs.


BigInteger wasn't designed for this sort of thing, and changing it so 
that supports constant-time encoding/decoding is a massive undertaking, 
if it is even possible. It would be more reasonable to add a new public 
type for integers that supports constant-time operations, but I don't 
think that helps us here.




Alternately, I guess you could throw some sort of exception if someone 
tries to call getS() or getEncoded().  Or you could do what PKCS11 
does for non-extractable private keys and only class the private key 
as a PrivateKey - make it opaque.  But then how do you create the key 
from stored information?


That's essentially the plan. Calling PrivateKey::getEncoded will return 
null, which is a convention for non-extractable keys. Trying to convert 
from/to an ECPrivateKeySpec using the KeyFactory in the new provider 
will result in an exception---so you won't have an object to call getS() 
on.


To create the key from stored information, the best way is to construct 
a PKCS8EncodedKeySpec using the encoded key. If you are starting with a 
BigInteger, and if branching is acceptable, you can use the KeyFactory 
from SunEC to convert an ECPrivateKeySpec to PrivateKey to get the 
encoded value.

Re: Conceptual feedback on new ECC JEP

[Michael StJohns]

Below

On 9/4/2018 8:57 AM, Adam Petcher wrote:

On 9/1/2018 2:03 PM, Michael StJohns wrote:


On 8/23/2018 1:50 PM, Adam Petcher wrote:
It will only support a subset of the API that is supported by the 
implementation in SunEC. In particular, it will reject any private 
keys with scalar values specified using BigInteger (as in 
ECPrivateKeySpec), and its private keys will not return scalar 
values as BigInteger (as in ECPrivateKey.getS()). 


Um... why?   EC Private keys are integers I've said this multiple 
times and - with the single exception of EDDSA keys because of a very 
idiosyncratic (and IMHO short-sighted) RFC specification - all of the 
EC private keys of whatever curve can be expressed as integers.




The explanation is in the JEP:

"The existing API for ECC private keys has some classes that specify 
private scalar values using BigInteger. There is no way to get a value 
out of a BigInteger (into, for example, a fixed-length array) without 
branching."


There is no problem with making private keys integers in the API. The 
problem is specifically with BigInteger and its implementation. 
BigInteger stores the value in the shortest int array possible. To 
access the value, you need to branch on the length of the array, which 
leaks whether the high-order bits of the private key are 0.





*buzz* wrong answer.   Sorry.   The internal storage of the key can be 
anything you want it to be if you want to prevent a non-constant-time 
issue for normal calculation.  But the import/export of the key really 
isn't subject to the cargo cult "must not branch" dogma - hint - I'm 
moving across a security boundary line anyway.    So if I do a 
"getEncoded()" or a "getS()" on an ECPrivateKey object or provide a 
BigInteger on an ECPrivateKeySpec there is no additional threat over the 
presence of the private key bits in public space.


If you believe this to be such a problem, then I'd suggest instead 
updating BigInteger to allow for BigEndian or LittleEndian encoding 
input/output and fix the constant time issue there inside the 
implementation rather than breaking public APIs.


Alternately, I guess you could throw some sort of exception if someone 
tries to call getS() or getEncoded().  Or you could do what PKCS11 does 
for non-extractable private keys and only class the private key as a 
PrivateKey - make it opaque.  But then how do you create the key from 
stored information?


Later, Mike

Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Weijun Wang]

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-dev@openjdk.java.net; core-libs-...@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-dev@openjdk.java.net; core-libs-...@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-dev@openjdk.java.net; core-libs-...@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 

Re: Conceptual feedback on new ECC JEP

[Adam Petcher]

On 9/1/2018 2:03 PM, Michael StJohns wrote:


On 8/23/2018 1:50 PM, Adam Petcher wrote:
It will only support a subset of the API that is supported by the 
implementation in SunEC. In particular, it will reject any private 
keys with scalar values specified using BigInteger (as in 
ECPrivateKeySpec), and its private keys will not return scalar values 
as BigInteger (as in ECPrivateKey.getS()). 


Um... why?   EC Private keys are integers I've said this multiple 
times and - with the single exception of EDDSA keys because of a very 
idiosyncratic (and IMHO short-sighted) RFC specification - all of the 
EC private keys of whatever curve can be expressed as integers.




The explanation is in the JEP:

"The existing API for ECC private keys has some classes that specify 
private scalar values using BigInteger. There is no way to get a value 
out of a BigInteger (into, for example, a fixed-length array) without 
branching."


There is no problem with making private keys integers in the API. The 
problem is specifically with BigInteger and its implementation. 
BigInteger stores the value in the shortest int array possible. To 
access the value, you need to branch on the length of the array, which 
leaks whether the high-order bits of the private key are 0.

RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Baesken, 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.

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-dev@openjdk.java.net; core-libs-...@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-dev@openjdk.java.net; core-libs-...@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-dev@openjdk.java.net; core-libs-...@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
>