Re: RFR 8241960: The SHA3 message digests are not thread safe when cloned

[Valerie Peng]

Right, with a closer look, it does require multiple threads to trigger 
this problem.


Valerie


On 3/31/2020 6:10 PM, Weijun Wang wrote:



On Apr 1, 2020, at 4:01 AM, Valerie Peng  wrote:

Hi Alexey,

Good catch, thanks for the report, I will review it.

On a first look, it seems that this is more about the clone() method of the 
SHA-3 impl missed copying/cloning an internal field.

Given that this is about SUN provider, I've modified the synopsis accordingly 
and move the priority up to P3.

It may not take multi-thread to reproduce this? If so, we can simplify the 
regression test.

Looks like a must:

 private void keccak() {
 // convert the 200-byte state into 25 lanes
 bytes2Lanes(state, lanes);
 // process the lanes through step mappings
 for (int ir = 0; ir < NR; ir++) {
 smIota(smChi(smPiRho(smTheta(lanes))), ir);
 }
 // convert the resulting 25 lanes back into 200-byte state
 lanes2Bytes(lanes, state);
 }

One object's lanes can only be used by another if this method is called by 
multiple threads at the same time.

--Max


Regards,
Valerie
On 3/31/2020 11:27 AM, Alexey Bakhtin wrote:

Hi All,

Please review fix for SHA3 message digests thread safety.
Issue reproduced on the JDK11, JDK13 and JDK14
JTREG test is provided in the patch

JBS: https://bugs.openjdk.java.net/browse/JDK-8241960
Webrev: https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v0/

Regards
Alexey

Re: RFR 8241960: The SHA3 message digests are not thread safe when cloned

[Weijun Wang]

> On Apr 1, 2020, at 4:01 AM, Valerie Peng  wrote:
> 
> Hi Alexey,
> 
> Good catch, thanks for the report, I will review it.
> 
> On a first look, it seems that this is more about the clone() method of the 
> SHA-3 impl missed copying/cloning an internal field.
> 
> Given that this is about SUN provider, I've modified the synopsis accordingly 
> and move the priority up to P3.
> 
> It may not take multi-thread to reproduce this? If so, we can simplify the 
> regression test.

Looks like a must:

private void keccak() {
// convert the 200-byte state into 25 lanes
bytes2Lanes(state, lanes);
// process the lanes through step mappings
for (int ir = 0; ir < NR; ir++) {
smIota(smChi(smPiRho(smTheta(lanes))), ir);
}
// convert the resulting 25 lanes back into 200-byte state
lanes2Bytes(lanes, state);
}

One object's lanes can only be used by another if this method is called by 
multiple threads at the same time.

--Max

> 
> Regards,
> Valerie
> On 3/31/2020 11:27 AM, Alexey Bakhtin wrote:
>> Hi All,
>> 
>> Please review fix for SHA3 message digests thread safety.
>> Issue reproduced on the JDK11, JDK13 and JDK14
>> JTREG test is provided in the patch
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8241960
>> Webrev: https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v0/
>> 
>> Regards
>> Alexey
>> 

Re: RFR 8239385: KerberosTicket client name refers wrongly to sAMAccountName in AD

[Weijun Wang]

> On Apr 1, 2020, at 8:22 AM, Martin Balao  wrote:
> 
> Hi Max,
> 
> Thanks for having a look at the CSR.
> 
> On 3/30/20 11:39 PM, Weijun Wang wrote:
>> 1. I don't think there is a need to talk about the java.security.krb5.conf 
>> system property, the krb5.conf file name is more popular.
>> 
> 
> Added a reference to the krb5.conf file in the first place. I wish we
> could keep a reference to the System property for completeness.

OK. I just want to emphasize that we are following MIT krb5 the reference 
implementation of krb5, but not inventing something new.

--Max

> 
>> 2. I'd rather always say "TGS requests" instead of "AP requests".
>> 
> 
> Changed. This also helped me to fix an AS/AP typo.
> 
> I set the target release to JDK-15.
> 
> Look forward to the CSR approval.
> 
> Thanks,
> Martin.-
> 

Re: RFR 8239385: KerberosTicket client name refers wrongly to sAMAccountName in AD

[Martin Balao]

Hi Max,

Thanks for having a look at the CSR.

On 3/30/20 11:39 PM, Weijun Wang wrote:
> 1. I don't think there is a need to talk about the java.security.krb5.conf 
> system property, the krb5.conf file name is more popular.
> 

Added a reference to the krb5.conf file in the first place. I wish we
could keep a reference to the System property for completeness.

> 2. I'd rather always say "TGS requests" instead of "AP requests".
> 

Changed. This also helped me to fix an AS/AP typo.

I set the target release to JDK-15.

Look forward to the CSR approval.

Thanks,
Martin.-

Re: RFR 8241960: The SHA3 message digests are not thread safe when cloned

[Valerie Peng]

Hi Alexey,

Good catch, thanks for the report, I will review it.

On a first look, it seems that this is more about the clone() method of 
the SHA-3 impl missed copying/cloning an internal field.


Given that this is about SUN provider, I've modified the synopsis 
accordingly and move the priority up to P3.


It may not take multi-thread to reproduce this? If so, we can simplify 
the regression test.


Regards,
Valerie
On 3/31/2020 11:27 AM, Alexey Bakhtin wrote:

Hi All,

Please review fix for SHA3 message digests thread safety.
Issue reproduced on the JDK11, JDK13 and JDK14
JTREG test is provided in the patch

JBS: https://bugs.openjdk.java.net/browse/JDK-8241960
Webrev: https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v0/

Regards
Alexey

Re: RFR: 8076999: SunJCE support of password-based encryption scheme 2 params (PBES2) not working

[Valerie Peng]

Hi Jamil,

I looked at java.security.AlgorithmParameters and need to update my 
earlier comment below


- By convention, each init() is a fresh start and wipes out the effect 
previous init() calls. But in the current webrev, they seems to apply 
changes on top of each other. This may not be the right model of how 
things should be handled. In addition, with the existing code handles 
both PBES2 and PBEwithAnd, we may need extra logic to 
restore the fields back to when they are first constructed at the 
beginning of every engineInit(...).


It turns out that AlgorithmParameters have an "initialized" flag which 
would only accept one successful init() call.


Subsequent call would be rejected by throwing either IOException or 
InvalidParameterSpecException. So, I guess we don't need to worry about 
multiple init() calls. Just that the supplied data is legit and is not 
conflicting with the assumed algorithm values if the object is requested 
under the "friendly" name.


Thanks,
Valerie
On 3/30/2020 8:25 PM, Valerie Peng wrote:


Hi Jamil,

Thanks for being so patient. It take me sometime to play around with 
the changes and think about various scenarios...


Here are some comments:



- Line 38 has RFC 2268 which is for RC2, RC5 is in RFC 2040.

- Line 48-51 comments can be simplified further, essentially, IV 
(provided or not) shall have the same length as the block size. If not 
provided explicitly, then its values are all 0s. Just some nit.




- line 187: variable name 'pbeSpec' should probably be named as 
'rc5Spec' as it has nothing to do with PBE.




- The test is very extensive... May I ask how are the test vectors 
generated? Are they from some RFCs or external website like NIST or 
internally generated using these new impl? In particular, I wonder 
about the test "HmacSHA256 and AES-256-CBC [No Enc parameters]", this 
is expected to pass, but AES CBC requires IV and if no parameters are 
explicitly specified, how do things work for decryption where IV is 
required and cannot generate random values as default IV? I am also 
not sure about the InitByDERAndEncodeTest, I think we should not alter 
the given encoding based on our own assumptions/defaults. If given a 
DER encoding bytes, the same (minor difference such as an absent null 
params is ok) encoding should be returned when getEncoded() is called.


- Line 143 expVals[4] should be expVals[3]?



- Current changes are a bit too complicated and I am not sure if it's 
worthwhile to support RC2, RC5, DESede and DES and do their 
algorithm-specific checking. PBES2 AlgorithmParameters parsing is 
already complicated due we support the original name (PBES2) and also 
the "friendly"  JCA naming convention of including the cipher name and 
KDF name. On top of it, the current webrev seem to trying to support 
parsing of all possible algorithms stated in PKCS#5 even when SunJCE 
provider only support the AES_xxx variants. If only parsing is done, 
the overhead may be acceptable, but then when there is also algorithm 
specific checking, I feel this is a bit much as I doubt that they will 
ever be used.


- There are a lot of String parsing inside the String-arg constructor 
which can be avoided if we replace this String-arg constructor with a 
2-arg constructor with PrfType and EncType. This should simplify the 
constructor code greatly and make it more robust. Then we probably can 
remove the getByName() method for both enum types. No need for the 
constructor to throw NoSuchAlgorithmException as all inputs are 
provided by provider and unsupported algorithm should be detected 
before calling this constructor.


- Line 259, add "," after the word "key"?

- The ordering of things under engineInit(AlgorithmParameterSpec 
paramSpec) seems a bit un-intuitive. User-supplied values should only 
be stored after pass validation. The assignment (line 324-326) should 
be done after the various checks. The check at line 338 should be 
moved up before assigning the default kdf type.


- Missing PBE subclasses for AES_192?

- Some of the static oid constants seems unnecessary as they are only 
used inside the enum and can be moved there.


- By convention, each init() is a fresh start and wipes out the effect 
previous init() calls. But in the current webrev, they seems to apply 
changes on top of each other. This may not be the right model of how 
things should be handled. In addition, with the existing code handles 
both PBES2 and PBEwithAnd, we may need extra logic to 
restore the fields back to when they are first constructed at the 
beginning of every engineInit(...).


- I am not too sure about the usefulness of "pbes2AlgorithmName" 
field. In the current impl, it is set in various places. If it is 
meant to reflect the latest KDF and CIPHER algorithm used, it's more 
robust to construct its value when needed. Otherwise, we need to 
remember updating this value whenever one of these 3 values, i.e. KDF, 
CIPHER, and keysize, changed.


- Consider grouping 

RFR 8241960: The SHA3 message digests are not thread safe when cloned

[Alexey Bakhtin]

Hi All,

Please review fix for SHA3 message digests thread safety.
Issue reproduced on the JDK11, JDK13 and JDK14
JTREG test is provided in the patch

JBS: https://bugs.openjdk.java.net/browse/JDK-8241960
Webrev: https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v0/

Regards
Alexey

Re: [15] RFR 8172680: Support SHA-3 based Hmac algorithms

[Michael StJohns]

Sorry - this one got past me.

For PKCS11 - the assignment of mechanism numbers can happen at any time 
and doesn't necessarily result in a new version of the specification.  
In this case, the API won't change, so there's no reason - since the 
mechanism numbers have been assigned since last May at least - to wait 
for V3.  Among other things, I would expect that various vendors have 
already implemented these in their 2.xx implementations.


One of the reasons I ended up using the SunPKCS11 wrapper classes 
directly quite a while ago was that the PKCS11 spec hadn't been updated, 
but that my PKCS11 provider was supplying various EC mechanisms that I 
needed.   Ideally, the JCA and SunPKCS11 provider support should 
*precede* any given underlying PKCS11 device support, not trail it by 
6-12 months.


The assignment of mechanism numbers is exactly equivalent to the 
assignment of TLS cipher suite numbers - the underlying protocol doesn't 
change, so this is mostly a change to the mapping tables and enclosed 
classes.


In any event, any given PKCS11 implementation may or may not support any 
given set of mechanisms - the provider really ought to be calling 
C_GetMechanismList() and using that as the list of supported JNA mechanisms.


Sorry - I'm dealing with a bit of lack of sleep here so I may be 
babbling, but I'm wondering if it might not be a bad idea to add some 
sort of PKCS11 specific mechanism naming convention to allow for the 
lag/lead problem?   E.g PKCS11_DIGEST_02B0 would be PKCS11's 
CKM_SHA3_256 hashing function given



#define CKM_SHA3_2560x02B0


Just a thought.

Thanks - Mike


On 3/19/2020 5:27 PM, Valerie Peng wrote:

Hi Mike,

Thanks for heads up. From what I can gather, SHA3 inclusion is part of 
PKCS#11 v3. Is this the next release which you are referring to? Or 
will there be an update to v2.40? Is there any schedule info for these 
update/release do you know?


Following the convention, we normally don't add something which the 
underlying library has no support yet. With the new 6-month JDK 
release cycle, it's much faster for the added functionality to be 
available. So, I'd still prefer to update SunPKCS11 provider with 
SHA-3 once they are officially included.


Valerie

On 3/18/2020 4:07 PM, Michael StJohns wrote:

On 3/18/2020 6:57 PM, Valerie Peng wrote:


Anyone has time to help review this straight forward RFE? It's to 
add SHA-3 support to Hmac.


RFE: https://bugs.openjdk.java.net/browse/JDK-8172680

Webrev: http://cr.openjdk.java.net/~valeriep/8172680/webrev.00/

Mach5 run is clean.

Thanks,
Valerie


Valerie -

I know the RFE says no PKCS11 because 2.40 doesn't include those 
items, but OASIS PKCS11 has proposed  SHA3 identifiers at 
https://github.com/oasis-tcs/pkcs11/blob/master/working/identifier_db/sha3.result 
- maybe reach out and ask if these have been allocated pending the 
next release?


Mike


#define CKM_SHA3_256  0x02b0UL
 #define CKM_SHA3_256_HMAC 0x02b1UL
 #define CKM_SHA3_256_HMAC_GENERAL 0x02b2UL
 #define CKM_SHA3_224  0x02b5UL
 #define CKM_SHA3_224_HMAC 0x02b6UL
 #define CKM_SHA3_224_HMAC_GENERAL 0x02b7UL
 #define CKM_SHA3_384  0x02c0UL
 #define CKM_SHA3_384_HMAC 0x02c1UL
 #define CKM_SHA3_384_HMAC_GENERAL 0x02c2UL
 #define CKM_SHA3_512  0x02d0UL
 #define CKM_SHA3_512_HMAC 0x02d1UL
 #define CKM_SHA3_512_HMAC_GENERAL 0x02d2UL

Re: Possible regression in JDK 14 related to SSLSessionContext / SSLSession on the server side

[Jamil Nimeh]

Thanks Norman, I'm going to file a bug on this one.  After playing with 
it a bit more I found cases where even SSLServerSockets do run into the 
issue but it doesn't always happen.  Still working on characterizing it.


--Jamil

On 3/31/2020 7:11 AM, Norman Maurer wrote:

Yes thats about right… if setting to false it works as expected.


Bye
Norman


On 31. Mar 2020, at 01:50, Jamil Nimeh > wrote:


Hi Norman,

I've been able to run your test code and I can reproduce it.  
Interestingly enough, it appears to happen when 
-Djdk.tls.server.enableSessionTicketExtension=true, which is the 
default position.  With session tickets enabled, I would see the 
issue in TLS 1.3 and 1.2 connections just as you did.  Setting the 
above property to false however allowed me to make successful 
connections.  Would you mind setting that property to false, just to 
make sure you and I see the same thing?


I did go back and run SSLServerSocket-based connections just to see 
if the session ticket settings had any impact on things, but they 
don't.  I can make connections to a socket based SSL server 
regardless of the property value on the server side.


Thanks,

--Jamil

On 3/30/2020 5:31 AM, Norman Maurer wrote:

Hey Sean,

There is not much to share as its just a simple handshake :)

Anyway here is a reproducer:

https://github.com/normanmaurer/jdk_ssl_session_context_reproducer

It basically does nothing more then complete the handshake and then 
calling engine.getSession().getSessionContext() which will return 
null on the server side since JDK 14 (earlier versions work).
I tested it with TLSv1.2 and TLSv1.3 and both times it produced the 
error on JDK 14.



Bye
Norman


On 30. Mar 2020, at 13:22, Seán Coffey > wrote:


Looks interesting Norman. Do you want to share some more details 
about the peculiarities of this handshake before considering a 
fully fledged testcase ?


regards,
Sean.

On 27/03/2020 12:48, Norman Maurer wrote:

Hi there,

I am just about to add JDK14 to the test matrix for netty and 
think I found a regression. Before I will invest time to write a 
standalone reproducer please let me know if you think this is a 
regression or not.
Basically after the handshake is complete 
SSLEngine.getSession().getSessionContext() returns null on the 
serverside when using JDK14. Running the same test with any 
previous version (JDK13 and earlier) doesn’t show the same result.


Does this sounds like a regression and if so should I provide a 
standalone reproducer here ?


Bye
Norman

Re: Possible regression in JDK 14 related to SSLSessionContext / SSLSession on the server side

[Norman Maurer]

Yes thats about right… if setting to false it works as expected.


Bye
Norman


> On 31. Mar 2020, at 01:50, Jamil Nimeh  wrote:
> 
> Hi Norman,
> 
> I've been able to run your test code and I can reproduce it.  Interestingly 
> enough, it appears to happen when 
> -Djdk.tls.server.enableSessionTicketExtension=true, which is the default 
> position.  With session tickets enabled, I would see the issue in TLS 1.3 and 
> 1.2 connections just as you did.  Setting the above property to false however 
> allowed me to make successful connections.  Would you mind setting that 
> property to false, just to make sure you and I see the same thing?
> 
> I did go back and run SSLServerSocket-based connections just to see if the 
> session ticket settings had any impact on things, but they don't.  I can make 
> connections to a socket based SSL server regardless of the property value on 
> the server side.
> 
> Thanks,
> 
> --Jamil
> 
> On 3/30/2020 5:31 AM, Norman Maurer wrote:
>> Hey Sean,
>> 
>> There is not much to share as its just a simple handshake :)
>> 
>> Anyway here is a reproducer:
>> 
>> https://github.com/normanmaurer/jdk_ssl_session_context_reproducer 
>> 
>> 
>> It basically does nothing more then complete the handshake and then calling 
>> engine.getSession().getSessionContext() which will return null on the server 
>> side since JDK 14 (earlier versions work).
>> I tested it with TLSv1.2 and TLSv1.3 and both times it produced the error on 
>> JDK 14.
>> 
>> 
>> Bye
>> Norman
>> 
>> 
>>> On 30. Mar 2020, at 13:22, Seán Coffey >> > wrote:
>>> 
>>> Looks interesting Norman. Do you want to share some more details about the 
>>> peculiarities of this handshake before considering a fully fledged testcase 
>>> ?
>>> 
>>> regards,
>>> Sean.
>>> 
>>> On 27/03/2020 12:48, Norman Maurer wrote:
 Hi there,
 
 I am just about to add JDK14 to the test matrix for netty and think I 
 found a regression. Before I will invest time to write a standalone 
 reproducer please let me know if you think this is a regression or not.
 Basically after the handshake is complete 
 SSLEngine.getSession().getSessionContext() returns null on the serverside 
 when using JDK14. Running the same test with any previous version (JDK13 
 and earlier) doesn’t show the same result.
 
 Does this sounds like a regression and if so should I provide a standalone 
 reproducer here ?
 
 Bye
 Norman
 
>>