Re: PKCS#11 provider issues with min and max size

[Tomas Gustavsson]

I just realized that a natural place to configure provider behavior is
in the provider construction, which is also per provider, so you can
have multiple ones with different configuration. We are already using an
InputStream to construct SunPKCS11, and adding more parameters to
configure/override defaults would be natural.

I.e.:
-
name =  
library = ;
slot = slot
rsakeygenmech = CKM_RSA_X9_31_KEY_PAIR_GEN
rsakeygenmechminsize = 1024
rsakeygenmechmaxsize = 8192

attributes(*, CKO_PUBLIC_KEY, *) = {
  CKA_TOKEN = false
  CKA_ENCRYPT = true
  CKA_VERIFY = true
  CKA_WRAP = true
}
attributes(*, CKO_PRIVATE_KEY, *) = {
  CKA_DERIVE = false
  CKA_TOKEN = true
  CKA_PRIVATE = true
  CKA_SENSITIVE = true
  CKA_EXTRACTABLE = false
  CKA_DECRYPT = true
  CKA_SIGN = true
  CKA_UNWRAP = true
}
attributes(*, CKO_SECRET_KEY, *) = {
  CKA_SENSITIVE = true
  CKA_EXTRACTABLE = false
  CKA_ENCRYPT = true
  CKA_DECRYPT = true
  CKA_SIGN = true
  CKA_VERIFY = true
  CKA_WRAP = true
  CKA_UNWRAP = true
}
-

Cheers,
Tomas

On 2018-02-09 09:55, Tomas Gustavsson wrote:
> 
> Hi,
> 
> Thanks for the answer. (sorry I was out with the flu for a week)
> 
>> I am not too keen to add an env var/system property to accommodate this
>> kind of PKCS11 library bugs since this should be rare I hope.
>> Valerie
> 
> Unfortunately I don't see it as rare and the impact is huge due to the
> slow turnaround of HSM firmware. Due to FIPS certification and whatnot
> HSM vendors do not fix simple things like this sometimes in years. This
> puts customers to a complete halt in the meantime. These are heavily
> certified environments where re-certification alone takes at least 6 months.
> 
> Having worked with all major HSM vendors for many years I know that
> PKCS11 library bugs are not rare at all. If we'd be using PKCS#11
> natively, you can hack around by ignoring bad values, but when using
> SunPKCS#11 you are stuck with Java's rules unless you patch SunPKCS11
> which is not for everyone.
> 
> Due to the strictness of using SunPKCS11 compared to native PKCS#11,
> some more flexibility in configuration would help a lot.
> 
> For many years SunPKCS11 have worked great. But also the HSM world is
> moving faster than they did 5 years ago, and unfortunately this means
> that we're seeing a huge rise in PKCS#11 issues in the last year,
> requiring quite a lot of hacking in SunPKCS11 to workaround. In theory
> it should not be needed, but in practice it is. Faster evolution = more
> bugs.
> 
> I just showed two real world use cases that you really need to be able
> to work around. And these will not be the last. PKCS#11 is a complex
> standard and implementors will rarely get it exactly right.
> 
> Increased flexibility and more control around PKCS#11 will be needed in
> the future imho, or users will be forced to move to other solutions like
> JackNJI11. We'd much rather use SunPKCS11 but customers (end users)
> can't get stuck between Java and HSM vendors in a fight who does PKCS#11
> right or wrong.
> 
> Cheers,
> Tomas
> 
> On 2018-02-01 01:07, Valerie Peng wrote:
>>
>> Thanks for the feedback. I suppose we can ignore values which obviously
>> don't make sense such as 0 or max being less than min key size.
>> However, if the underlying PKCS11 library vendors forgot to update the
>> max value as in your comment#2, supposedly they should fix it.
>> I am not too keen to add an env var/system property to accommodate this
>> kind of PKCS11 library bugs since this should be rare I hope.
>> Valerie
>>
>> On 1/30/2018 12:22 AM, Tomas Gustavsson wrote:
>>> Hi,
>>>
>>> At some revision in the PKCS#11 provider there was introduced checking
>>> of C_GetMechanismInfo min and max sizes.
>>>
>>> This has turned out to be a bit fragile. Let me give two real world
>>> examples:
>>>
>>> 1. Amazon Cloud HSM report minSize and maxSize for EC keys to 0. The
>>> Java PKCS#11 provider will happily take 0 as maxSize and refuse to
>>> generate any EC keys at all. Needless to say, without the Java check it
>>> would be no problem.
>>>
>>> 131: C_GetMechanismInfo
>>> 2018-01-30 07:52:20.740
>>> [in] slotID = 0x1
>>>   CKM_EC_KEY_PAIR_GEN
>>> [out] pInfo:
>>> CKM_EC_KEY_PAIR_GEN   : min:0 max:0 flags:0x10001 ( Hardware
>>> KeyPair )
>>> Returned:  0 CKR_OK
>>>
>>> (we are reporting this to Amazon as well)
>>>
>>> 2. Thales HSMs (some?) report maxSize for RSA_PKCS key generation as
>>> 4096, but will happily generate 8192 bit keys. I.e. the reported maxSize
>>> is not true.
>>> We have customers who used to generate 8192 bit RSA keys, but after a
>>> Java update can not do so anymore, because Java compares against this
>>> value.
>>>
>>>
>>> * Suggestions:
>>>
>>> 1. In the constructor of P11KeyPairGenerator where minKeyLen and
>>> maxKeyLen are calculated, never allow maxKeyLen to be less than
>>> minKeyLen.
>>>
>>> I.e. change the part:
>>>  // auto-adjust default keysize in case it's out-of-range
>>>  if ((minKeyLen != -1) && (keySize < 

Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC

[Tomas Gustavsson]

Just FYI. SoftHSM2 from the OpenDNSSec project is a good P11 to test
with, and I believe it supports brainpool in recent versions.
https://github.com/opendnssec/SoftHSMv2

It works really good)

Regards,
Tomas

On 2018-02-09 02:03, Valerie Peng wrote:
> Hi Tobias,
> 
> Just curious, which PKCS11 library did you use to test your patch? After
> I applied your patch and ran the regression tests, I noticed that both
> the Solaris PKCS11 library and NSS skipped testing Brainpool curves with
> different error codes which may be due to lack of support...
> 
> Regards,
> Valerie
> 
> On 1/17/2018 3:02 PM, Tobias Wagner wrote:
>> -Ursprüngliche Nachricht-
>>> Von:Adam Petcher 
>>> Gesendet: Die 16 Januar 2018 18:38
>>> An: security-dev@openjdk.java.net
>>> Betreff: Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
>>>
>>> Great! I took a look at the patch, and I have some comments, the first
>>> of which probably needs to be addressed before I can test the change:
>>>
>>> 1) Is this patch against the http://hg.openjdk.java.net/jdk/jdk
>>> repository? I suspect it isn't because some of the paths are different
>>> than what I expect. We have made a lot of changes to the repositories in
>>> the last few months. If this patch is against an older repo, please send
>>> a patch against http://hg.openjdk.java.net/jdk/jdk .
>> I switched to that repository now. As described on
>> http://openjdk.java.net/contribute/, I was
>> working with the http://hg.openjdk.java.net/jdk9/jdk9 repository.
>>
>>> 2) TestECDH.java: It's probably better to remove the provider name check
>>> on line 116 and test on any providers that support the curve.
>> OK, it's removed. I was thinking the same.
>>
>>> 3) oid.c: I think you can remove the comments that say "XXX bounds
>>> check" (e.g. line 362). If I am interpreting these comments correctly,
>>> they are saying that memcmp may read out of bounds, but you fixed that
>>> problem by using oideql.
>> I left them in place - my interpretation is, that they are meant for a
>> check
>> before accessing the *_oids arrays, as C arrays have no implicit check
>> for that.
>> As long as only oids from CurveDB are used, there would be no problems.
>> The least significant byte of the requested oid is used as index. If
>> that index
>> exeeds the defined array length, something odd from the memory there
>> is returned.
>> At least that's my understynding of C and the comment there.
>>
>>> 4) Is there an existing test that exercises ECDSA with the new curves?
>>> Maybe there is something in the PKCS11 tests that does this already, but
>>> I didn't find it. I think we should have an ECDSA test to make sure that
>>> we didn't forget anything. ECDSA test vectors probably aren't
>>> necessary---a simple test that signs and verifies using the new curves
>>> should be sufficient.
>> Yes, there are tests in TestCurves, which runs through TestEC.
>> TestCurves gets a List
>> of all supported ECParameterSpec by the Provider and runs ECDSA
>> signatures and verifications
>> with all of them. The results of all curves are logged in the jtreg
>> report of TestEC.
>>
>> I also changed the InvalidCurve test to use brainpoolP160r1 now, as
>> brainpoolP256r1 is supported
>> by using this patch.
>>
>>> On 1/12/2018 9:12 AM, Tobias Wagner wrote:
 Hi,

 here is the next patch for brainpool curve support in SunEC.

 Differences from the first patch:

 * Brainpool curves with less than 256 bits are removed.
 Subsequently, the curve oid check is made more robust to avoid null
 pointer caused Segmentation Faults in memcmp calls.

 * Bug JDK-8189594 is fixed.

 * Known answer tests for each new curve are added to
 sun.security.pkcs11.ec.TestECDH. The tests are only executed, if the
 tested provider's name is "SunEC" and the tested provider claims to
 support the respective curve. For SunEC, these tests are
 executed during sun.security.ec.TestEC.

 I decided to add these test vectors to TestECDH to avoid code
 duplications, as TestECDH is describes exactly the test
 for that kind of test vectors.
 The superclass to TestECDH, TestPKCS11, is also adapted to provide a
 method to check, whether one particular curve is
 supported.

 While the test vectors for the 256, 384 and 512 bit curve are taken
 from [1], the test vector for brainpoolP320r1 comes from [2].
 The latter one is a draft version of RFC 6954.

 Regards,
 Tobias

 [1] https://tools.ietf.org/html/rfc7027#appendix-A
 [2]
 https://tools.ietf.org/html/draft-merkle-ikev2-ke-brainpool-00#appendix-A.5



> 

RFR: 8193892: Impact of noncloneable MessageDigest implementation

[Seán Coffey]

Looking to push a new test which helps test CloneableDigest code. It's 
something that arose during JDK-8193683 fix.


The test was contributed by Brad Wetmore. I've converted it to use the 
SSLSocketTemplate.


JBS : https://bugs.openjdk.java.net/browse/JDK-8193892
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8193892/webrev/

--
Regards,
Sean.

Re: PKCS#11 provider issues with min and max size

[Valerie Peng]

Oh-well, I suppose that we are all humans. ;)
Let me take a closer look on this and see if there are other ways to 
relax this constraint than adding env var which should be the very last 
resort in my opinion.

BTW, is there a bug filed for this?

Thanks for the feedback,
Valerie

On 2/9/2018 12:55 AM, Tomas Gustavsson wrote:

Hi,

Thanks for the answer. (sorry I was out with the flu for a week)


I am not too keen to add an env var/system property to accommodate this
kind of PKCS11 library bugs since this should be rare I hope.
Valerie

Unfortunately I don't see it as rare and the impact is huge due to the
slow turnaround of HSM firmware. Due to FIPS certification and whatnot
HSM vendors do not fix simple things like this sometimes in years. This
puts customers to a complete halt in the meantime. These are heavily
certified environments where re-certification alone takes at least 6 months.

Having worked with all major HSM vendors for many years I know that
PKCS11 library bugs are not rare at all. If we'd be using PKCS#11
natively, you can hack around by ignoring bad values, but when using
SunPKCS#11 you are stuck with Java's rules unless you patch SunPKCS11
which is not for everyone.

Due to the strictness of using SunPKCS11 compared to native PKCS#11,
some more flexibility in configuration would help a lot.

For many years SunPKCS11 have worked great. But also the HSM world is
moving faster than they did 5 years ago, and unfortunately this means
that we're seeing a huge rise in PKCS#11 issues in the last year,
requiring quite a lot of hacking in SunPKCS11 to workaround. In theory
it should not be needed, but in practice it is. Faster evolution = more
bugs.

I just showed two real world use cases that you really need to be able
to work around. And these will not be the last. PKCS#11 is a complex
standard and implementors will rarely get it exactly right.

Increased flexibility and more control around PKCS#11 will be needed in
the future imho, or users will be forced to move to other solutions like
JackNJI11. We'd much rather use SunPKCS11 but customers (end users)
can't get stuck between Java and HSM vendors in a fight who does PKCS#11
right or wrong.

Cheers,
Tomas

On 2018-02-01 01:07, Valerie Peng wrote:

Thanks for the feedback. I suppose we can ignore values which obviously
don't make sense such as 0 or max being less than min key size.
However, if the underlying PKCS11 library vendors forgot to update the
max value as in your comment#2, supposedly they should fix it.
I am not too keen to add an env var/system property to accommodate this
kind of PKCS11 library bugs since this should be rare I hope.
Valerie

On 1/30/2018 12:22 AM, Tomas Gustavsson wrote:

Hi,

At some revision in the PKCS#11 provider there was introduced checking
of C_GetMechanismInfo min and max sizes.

This has turned out to be a bit fragile. Let me give two real world
examples:

1. Amazon Cloud HSM report minSize and maxSize for EC keys to 0. The
Java PKCS#11 provider will happily take 0 as maxSize and refuse to
generate any EC keys at all. Needless to say, without the Java check it
would be no problem.

131: C_GetMechanismInfo
2018-01-30 07:52:20.740
[in] slotID = 0x1
   CKM_EC_KEY_PAIR_GEN
[out] pInfo:
CKM_EC_KEY_PAIR_GEN   : min:0 max:0 flags:0x10001 ( Hardware
KeyPair )
Returned:  0 CKR_OK

(we are reporting this to Amazon as well)

2. Thales HSMs (some?) report maxSize for RSA_PKCS key generation as
4096, but will happily generate 8192 bit keys. I.e. the reported maxSize
is not true.
We have customers who used to generate 8192 bit RSA keys, but after a
Java update can not do so anymore, because Java compares against this
value.


* Suggestions:

1. In the constructor of P11KeyPairGenerator where minKeyLen and
maxKeyLen are calculated, never allow maxKeyLen to be less than
minKeyLen.

I.e. change the part:
  // auto-adjust default keysize in case it's out-of-range
  if ((minKeyLen != -1) && (keySize < minKeyLen)) {
  keySize = minKeyLen;
  }
  if ((maxKeyLen != -1) && (keySize > maxKeyLen)) {
  keySize = maxKeyLen;
  }

To include something like:
  // auto-adjust default keysize in case it's out-of-range
  if ((minKeyLen != -1) && (keySize < minKeyLen)) {
  keySize = minKeyLen;
  }
  if ((maxKeyLen != -1) && (maxKeyLen < minKeyLen)) {
  maxKeyLen = minKeyLen;
  }
  if ((maxKeyLen != -1) && (keySize > maxKeyLen)) {
  keySize = maxKeyLen;
  }

2. Allow to ignore checking of maxKeyLen by some means, i.e. allow to
ignore checking against C_GetMechanismInfo if you know that the HSM does
not provide sane values. I.e. an environment variable for example
reverting back to the old behavior when these were ignored.

Regards,
Tomas Gustavsson

Re: PKCS#11 provider issues with min and max size

[Valerie Peng]

Hmm, seems reasonable to support this in the provider configuration file.
Or, on a similar note, but slightly different approach is to just add an 
configuration option for disabling checking the supported key size range.

Regards,
Valerie

On 2/9/2018 2:16 AM, Tomas Gustavsson wrote:

I just realized that a natural place to configure provider behavior is
in the provider construction, which is also per provider, so you can
have multiple ones with different configuration. We are already using an
InputStream to construct SunPKCS11, and adding more parameters to
configure/override defaults would be natural.

I.e.:
-
name =  
library = ;
slot = slot
rsakeygenmech = CKM_RSA_X9_31_KEY_PAIR_GEN
rsakeygenmechminsize = 1024
rsakeygenmechmaxsize = 8192

attributes(*, CKO_PUBLIC_KEY, *) = {
   CKA_TOKEN = false
   CKA_ENCRYPT = true
   CKA_VERIFY = true
   CKA_WRAP = true
}
attributes(*, CKO_PRIVATE_KEY, *) = {
   CKA_DERIVE = false
   CKA_TOKEN = true
   CKA_PRIVATE = true
   CKA_SENSITIVE = true
   CKA_EXTRACTABLE = false
   CKA_DECRYPT = true
   CKA_SIGN = true
   CKA_UNWRAP = true
}
attributes(*, CKO_SECRET_KEY, *) = {
   CKA_SENSITIVE = true
   CKA_EXTRACTABLE = false
   CKA_ENCRYPT = true
   CKA_DECRYPT = true
   CKA_SIGN = true
   CKA_VERIFY = true
   CKA_WRAP = true
   CKA_UNWRAP = true
}
-

Cheers,
Tomas

On 2018-02-09 09:55, Tomas Gustavsson wrote:

Hi,

Thanks for the answer. (sorry I was out with the flu for a week)


I am not too keen to add an env var/system property to accommodate this
kind of PKCS11 library bugs since this should be rare I hope.
Valerie

Unfortunately I don't see it as rare and the impact is huge due to the
slow turnaround of HSM firmware. Due to FIPS certification and whatnot
HSM vendors do not fix simple things like this sometimes in years. This
puts customers to a complete halt in the meantime. These are heavily
certified environments where re-certification alone takes at least 6 months.

Having worked with all major HSM vendors for many years I know that
PKCS11 library bugs are not rare at all. If we'd be using PKCS#11
natively, you can hack around by ignoring bad values, but when using
SunPKCS#11 you are stuck with Java's rules unless you patch SunPKCS11
which is not for everyone.

Due to the strictness of using SunPKCS11 compared to native PKCS#11,
some more flexibility in configuration would help a lot.

For many years SunPKCS11 have worked great. But also the HSM world is
moving faster than they did 5 years ago, and unfortunately this means
that we're seeing a huge rise in PKCS#11 issues in the last year,
requiring quite a lot of hacking in SunPKCS11 to workaround. In theory
it should not be needed, but in practice it is. Faster evolution = more
bugs.

I just showed two real world use cases that you really need to be able
to work around. And these will not be the last. PKCS#11 is a complex
standard and implementors will rarely get it exactly right.

Increased flexibility and more control around PKCS#11 will be needed in
the future imho, or users will be forced to move to other solutions like
JackNJI11. We'd much rather use SunPKCS11 but customers (end users)
can't get stuck between Java and HSM vendors in a fight who does PKCS#11
right or wrong.

Cheers,
Tomas

On 2018-02-01 01:07, Valerie Peng wrote:

Thanks for the feedback. I suppose we can ignore values which obviously
don't make sense such as 0 or max being less than min key size.
However, if the underlying PKCS11 library vendors forgot to update the
max value as in your comment#2, supposedly they should fix it.
I am not too keen to add an env var/system property to accommodate this
kind of PKCS11 library bugs since this should be rare I hope.
Valerie

On 1/30/2018 12:22 AM, Tomas Gustavsson wrote:

Hi,

At some revision in the PKCS#11 provider there was introduced checking
of C_GetMechanismInfo min and max sizes.

This has turned out to be a bit fragile. Let me give two real world
examples:

1. Amazon Cloud HSM report minSize and maxSize for EC keys to 0. The
Java PKCS#11 provider will happily take 0 as maxSize and refuse to
generate any EC keys at all. Needless to say, without the Java check it
would be no problem.

131: C_GetMechanismInfo
2018-01-30 07:52:20.740
[in] slotID = 0x1
   CKM_EC_KEY_PAIR_GEN
[out] pInfo:
CKM_EC_KEY_PAIR_GEN   : min:0 max:0 flags:0x10001 ( Hardware
KeyPair )
Returned:  0 CKR_OK

(we are reporting this to Amazon as well)

2. Thales HSMs (some?) report maxSize for RSA_PKCS key generation as
4096, but will happily generate 8192 bit keys. I.e. the reported maxSize
is not true.
We have customers who used to generate 8192 bit RSA keys, but after a
Java update can not do so anymore, because Java compares against this
value.


* Suggestions:

1. In the constructor of P11KeyPairGenerator where minKeyLen and
maxKeyLen are calculated, never allow maxKeyLen to be less than
minKeyLen.

I.e. change the part:
  // auto-adjust default keysize in case it's out-of-range
  if 

AW: [PATCH]: Support for brainpool curves from CurveDB in SunEC

[Tobias Wagner]

Hi Valerie,

these tests were initally meant for the new curves in SunEC, but the SunEC 
tests are using
the PKCS#11 tests. Even though I made these known answer tests for SunEC, I 
think it
might be a good idea not to limit them to SunEC but run them on tests using 
TestECDH with
any provider, which claims to support these curves. 

The supported/unsupported statements come from getSupportedECParameterSpec in 
PKCS11Test
and depend on the exception a provider throws. I moved that code to this method 
from the getKnowCurves
method, to check if one particular curve is available. So, yes, I think it lack 
of support in these providers,
and the log message depends on the exception type and the execptions message.

I executed the with an unpatched and a patched build of the OpenJDK. In both 
cases it was the SunEC provider.

The output for the unpatched reference build for sun.security.ec.TestEC using 
sun.security.pkcs11.ec.TestECDH:
...
>Running tests with SunEC provider...
>
>libsoftokn3 version = 3.16.  ECC Basic.
>Testing using parameters secp192r1 [NIST P-192, X9.62 prime192v1] 
>(1.2.840.10045.3.1.1)...
>Testing using parameters sect163r2 [NIST B-163] (1.3.132.0.15)...
> brainpoolP256r1: Unsupported: Key Length: Unsupported curve: brainpoolP256r1 
> (1.3.36.3.3.2.8.1.1.7)
> brainpoolP320r1: Unsupported: Key Length: Unsupported curve: brainpoolP320r1 
> (1.3.36.3.3.2.8.1.1.9)
> brainpoolP384r1: Unsupported: Key Length: Unsupported curve: brainpoolP384r1 
> (1.3.36.3.3.2.8.1.1.11)
> brainpoolP512r1: Unsupported: Key Length: Unsupported curve: brainpoolP512r1 
> (1.3.36.3.3.2.8.1.1.13)
>OK
...

and for the patched build:
...
>Running tests with SunEC provider...
>
>libsoftokn3 version = 3.16.  ECC Basic.
>Testing using parameters secp192r1 [NIST P-192, X9.62 prime192v1] 
>(1.2.840.10045.3.1.1)...
>Testing using parameters sect163r2 [NIST B-163] (1.3.132.0.15)...
> brainpoolP256r1: Supported
>Testing using parameters brainpoolP256r1 (1.3.36.3.3.2.8.1.1.7)...
> brainpoolP320r1: Supported
>Testing using parameters brainpoolP320r1 (1.3.36.3.3.2.8.1.1.9)...
> brainpoolP384r1: Supported
>Testing using parameters brainpoolP384r1 (1.3.36.3.3.2.8.1.1.11)...
> brainpoolP512r1: Supported
>Testing using parameters brainpoolP512r1 (1.3.36.3.3.2.8.1.1.13)...
>OK
...

When running sun.security.pkcs11.ec.TestECDH directly, I get the following:
...
>libsoftokn3 version = 3.16.  ECC Basic.
>Beginning test run TestECDH...
>Running test with provider SunPKCS11-NSS (security manager enabled) ...
>NSS only supports Basic ECC.  Skipping..
>Completed test with provider SunPKCS11-NSS (28 ms).
...

Regards,
Tobias

-Ursprüngliche Nachricht-
> Von:Valerie Peng 
> Gesendet: Fre 9 Februar 2018 02:03
> An: Tobias Wagner ; security-dev@openjdk.java.net
> Betreff: Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
> 
> Hi Tobias,
> 
> Just curious, which PKCS11 library did you use to test your patch? After 
> I applied your patch and ran the regression tests, I noticed that both 
> the Solaris PKCS11 library and NSS skipped testing Brainpool curves with 
> different error codes which may be due to lack of support...
> 
> Regards,
> Valerie
> 

Re: PKCS#11 provider issues with min and max size

[Tomas Gustavsson]

Hi,

Thanks for the answer. (sorry I was out with the flu for a week)

> I am not too keen to add an env var/system property to accommodate this
> kind of PKCS11 library bugs since this should be rare I hope.
> Valerie

Unfortunately I don't see it as rare and the impact is huge due to the
slow turnaround of HSM firmware. Due to FIPS certification and whatnot
HSM vendors do not fix simple things like this sometimes in years. This
puts customers to a complete halt in the meantime. These are heavily
certified environments where re-certification alone takes at least 6 months.

Having worked with all major HSM vendors for many years I know that
PKCS11 library bugs are not rare at all. If we'd be using PKCS#11
natively, you can hack around by ignoring bad values, but when using
SunPKCS#11 you are stuck with Java's rules unless you patch SunPKCS11
which is not for everyone.

Due to the strictness of using SunPKCS11 compared to native PKCS#11,
some more flexibility in configuration would help a lot.

For many years SunPKCS11 have worked great. But also the HSM world is
moving faster than they did 5 years ago, and unfortunately this means
that we're seeing a huge rise in PKCS#11 issues in the last year,
requiring quite a lot of hacking in SunPKCS11 to workaround. In theory
it should not be needed, but in practice it is. Faster evolution = more
bugs.

I just showed two real world use cases that you really need to be able
to work around. And these will not be the last. PKCS#11 is a complex
standard and implementors will rarely get it exactly right.

Increased flexibility and more control around PKCS#11 will be needed in
the future imho, or users will be forced to move to other solutions like
JackNJI11. We'd much rather use SunPKCS11 but customers (end users)
can't get stuck between Java and HSM vendors in a fight who does PKCS#11
right or wrong.

Cheers,
Tomas

On 2018-02-01 01:07, Valerie Peng wrote:
> 
> Thanks for the feedback. I suppose we can ignore values which obviously
> don't make sense such as 0 or max being less than min key size.
> However, if the underlying PKCS11 library vendors forgot to update the
> max value as in your comment#2, supposedly they should fix it.
> I am not too keen to add an env var/system property to accommodate this
> kind of PKCS11 library bugs since this should be rare I hope.
> Valerie
> 
> On 1/30/2018 12:22 AM, Tomas Gustavsson wrote:
>> Hi,
>>
>> At some revision in the PKCS#11 provider there was introduced checking
>> of C_GetMechanismInfo min and max sizes.
>>
>> This has turned out to be a bit fragile. Let me give two real world
>> examples:
>>
>> 1. Amazon Cloud HSM report minSize and maxSize for EC keys to 0. The
>> Java PKCS#11 provider will happily take 0 as maxSize and refuse to
>> generate any EC keys at all. Needless to say, without the Java check it
>> would be no problem.
>>
>> 131: C_GetMechanismInfo
>> 2018-01-30 07:52:20.740
>> [in] slotID = 0x1
>>   CKM_EC_KEY_PAIR_GEN
>> [out] pInfo:
>> CKM_EC_KEY_PAIR_GEN   : min:0 max:0 flags:0x10001 ( Hardware
>> KeyPair )
>> Returned:  0 CKR_OK
>>
>> (we are reporting this to Amazon as well)
>>
>> 2. Thales HSMs (some?) report maxSize for RSA_PKCS key generation as
>> 4096, but will happily generate 8192 bit keys. I.e. the reported maxSize
>> is not true.
>> We have customers who used to generate 8192 bit RSA keys, but after a
>> Java update can not do so anymore, because Java compares against this
>> value.
>>
>>
>> * Suggestions:
>>
>> 1. In the constructor of P11KeyPairGenerator where minKeyLen and
>> maxKeyLen are calculated, never allow maxKeyLen to be less than
>> minKeyLen.
>>
>> I.e. change the part:
>>  // auto-adjust default keysize in case it's out-of-range
>>  if ((minKeyLen != -1) && (keySize < minKeyLen)) {
>>  keySize = minKeyLen;
>>  }
>>  if ((maxKeyLen != -1) && (keySize > maxKeyLen)) {
>>  keySize = maxKeyLen;
>>  }
>>
>> To include something like:
>>  // auto-adjust default keysize in case it's out-of-range
>>  if ((minKeyLen != -1) && (keySize < minKeyLen)) {
>>  keySize = minKeyLen;
>>  }
>>  if ((maxKeyLen != -1) && (maxKeyLen < minKeyLen)) {
>>  maxKeyLen = minKeyLen;
>>  }
>>  if ((maxKeyLen != -1) && (keySize > maxKeyLen)) {
>>  keySize = maxKeyLen;
>>  }
>>
>> 2. Allow to ignore checking of maxKeyLen by some means, i.e. allow to
>> ignore checking against C_GetMechanismInfo if you know that the HSM does
>> not provide sane values. I.e. an environment variable for example
>> reverting back to the old behavior when these were ignored.
>>
>> Regards,
>> Tomas Gustavsson
>>
> 

Re: contribute to the OpenJDK security group

[Tomas Gustavsson]

Awesome.

I just posted in another thread to the mailing list with some examples
where more flexibility would be needed. "PKCS#11 provider issues with
min and max size".

Another example would be to add flexibility to be more crypto "agile".
I.e. when new algorithms come out, for example SHA3. It will be included
in the PKCS#11 spec eventually, and then it takes a decent round-trip
time before it's possible to use through SunPKCS#11. So an example would
be to be able to configure new mechanisms, that does not affect any
other code in SunPKCS11, dynamically.

Yet another example, AWS CloudHSM requires you to use
CKM_RSA_X9_31_KEY_PAIR_GEN instead of CKM_RSA_PKCS_KEY_PAIR_GEN.
Patching SunPKCS11 to use it is trivial (for java programmers that is),
just replace one string with another. If it could be done dynamically
without patching java files it would be much nicer...and more stable as
it survives JDK upgrades.

Cheers,
Tomas

On 2018-02-01 20:25, Martin Balao wrote:
> Hi Tomas,
> 
> As Andrew said, I've been working on some SunPKCS11 improvements related
> to native memory leaking. You can find details of this work here [1].
> Feedback is always welcomed.
> 
> What do you mean with "more flexibility"?
> 
> --
> [1]
> - http://mail.openjdk.java.net/pipermail/security-dev/2017-October/016400.html
> 
> On Wed, Jan 24, 2018 at 8:06 AM, Andrew Haley  > wrote:
> 
> On 24/01/18 10:39, Tomas Gustavsson wrote:
> > Imho the P11 layer always needs attention. To work properly we're
> > relying on some patches, where parts was recently merged into OpenJDK.
> > We just started testing the Amazon CloudHSM, and that requires changes
> > to SunPKCS11 as well to work. Not always bad in SunPKCS11 as some P11
> > libraries out there do strange non-conforming stuff, but there's room
> > for more flexibility nevertheless.
> 
> Martin Balao has been heavily reworking this layer because it leaks
> native memory.  I'll let him fill you in on the details.
> 
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. 
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
> 
> 

Re: RFR 8191438: jarsigner should print when a timestamp will expire

[Weijun Wang]

Updated again at http://cr.openjdk.java.net/~weijun/8191438/webrev.05/.

--Max

> On Jan 4, 2018, at 8:48 AM, Weijun Wang  wrote:
> 
> Please take a review at
> 
>  http://cr.openjdk.java.net/~weijun/8191438/webrev.04/
> 
> Major changes:
> 
> 1. Warnings on TSA cert chain: expired or expiring
> 
> 2. No more check on trusted certs
> 
> 3. More output at signing when -verbose is on
> 
> 4. Fine tune messages when TSA cert expires earlier than signer cert
> 
> 5. New test cases
> 
> 6. Existing tests modification so signer is not trusted
> 
> Thanks
> Max
>