Bug #289

invalid padding when using java pkcs11 provider

Added by Rich Lowe over 3 years ago. Updated over 3 years ago.

Status:Resolved Start date:2010-10-02
Priority:High Due date:
Assignee:Jason King % Done:

100%

Category:- Spent time: -
Target version:-
Difficulty:Medium Tags:needs-triage

Description

The detail for this bug are recorded in #255, filed against OpenIndiana (which I can't appear to copy as an illumos bug), so that the OI bug can track their workaround or application of the fix.

In summary, a Java application providing a service over an SSL protected socket will fail when clients connect, complaining of invalid padding. A test case is provided in #255

bug-289.diff (2.8 kB) Jason King, 10/07/2010 10:54 pm

History

Updated by Gordon Ross over 3 years ago

Interesting. So, it's probably this change set:

changeset: 12573:fb4ef506980f
user: Dina K Nimeh <>
date: Mon Jun 07 08:54:25 2010 -0700
description:
6875651 move asymmetric crypto to libsoftcrypto
6816864 collect together padding methods used by PKCS#11
6917508 bignum library needs big random number function
6249983 softtoken based RSA/DSA slow on Niagara
6917506 arcfour lint check missing from usr/src/uts/sun4v/Makefile
6917513 move softFipsDSAUtil.c to common/crypto/fips/fips_dsa_util.c
6834849 dsa_sign() produces invalid signature when pkcs11 engine is used via openssl(1) for certain keys

One way to track this down might be to build the version of this code before the above change set, and compare old and new DH behavior with dbx.

Updated by Rich Lowe over 3 years ago

Gordon Ross wrote:

Interesting. So, it's probably this change set:

changeset: 12573:fb4ef506980f

It is.

One way to track this down might be to build the version of this code before the above change set, and compare old and new DH behavior with dbx.

Can't do that strictly, because of it being at a time of signed crypto. What I've done instead is to back out the change, merge it forward, then re-apply it in logical chunks. Sadly, the bug I found (which caused me to open this CR, for the fix), turns out to not be the bug, but a bug. Most annoying. Work continues.

I'm convinced it'll be obvious, and I'm just blinded to it by hours of staring at it.

Updated by Rich Lowe over 3 years ago

  • Assignee changed from Rich Lowe to Jason King

Punting to Jason, who has the solution, and a fix with a related buffer sizing bug.

Updated by Jason King over 3 years ago

  • File bug-289.diff added
  • % Done changed from 0 to 90

After doing a lot of testing, it appears that the code confuses the size of x and y a lot, causing soft_dh_genkey_pair to copy the incorrect number of bytes for Y into the pkcs11 object.

A patch that should fix this is attached. I am running nightly now and will submit a webrev for review once that has completed successfully.

Updated by Jason King over 3 years ago

  • File bug-289.diff added

Err slight typo in diff, updating :)

Updated by Jason King over 3 years ago

  • File deleted (bug-289.diff)

Updated by Jason King over 3 years ago

  • File deleted (bug-289.diff)

Updated by Jason King over 3 years ago

Updated by Jason King over 3 years ago

Just to add some commentary to the bug, DH takes two known values (a base 'g', and a prime number 'p'), creates a private random number x, and generates a public key y = g^x mod p. When combined with the other party's y value, yields a secret key that cannot feasibly be computed from only the two public values.

The values of g, p, and x must obey these rules (there are a couple more that aren't relevant):
0 < g < p
0 < x < p-1
Since y = g^x mod p, 0 < y < p

Thus the sizes (used to store the values) of g, x, and p must obey:
  • sizeof (g) <= sizeof (p)
  • sizeof (x) <= sizeof (p)
  • sizeof (y) sizeof (p)

Since g, p, and sizeof (x) are given as parameters. In source:usr/src/lib/pkcs11/pkcs11_softtoken/softDH.c

1 /*
2   * The integer public value y shall be converted to an octet
3   * string PV of length k, the public value.
4   */
5 if ((rv = soft_genDHkey_set_attribute(pubkey, CKA_VALUE, public_y,
6     CRYPTO_BITS2BYTES(k.value_bits), B_TRUE)) != CKR_OK) {
7         goto ret;
8 } 

value_bits is the size of x. Thus when sizeof (x) < sizeof (y), this will only partially copy the public value into the pkcs11 key object.

Also, in source:usr/src/common/crypto/dh/dh_impl.c there is

1         /* Convert public-value y to bignum. */
2          if ((brv = big_extend(&(dhkey.y), CHARLEN2BIGNUMLEN(value_bytes))) !=
3              BIG_OK) {
4                  rv = convert_rv(brv);
5                  goto ret;
6          }
7          bytestring2bignum(&(dhkey.y), bkey->public_y, value_bytes);

In this case value_bytes is the size (in bytes instead of bits) of x. Again, this will truncate the value of y, which will produce an incorrect secret key. The remaining fixes should clarify the sizing used (the old code sometimes just use the size of the prime, while should be the largest sized number during the calculation, can lead to confusion when doing analysis).

While we're not sure if the original submitter tripped over this, there is another potential problem with 64-bit systems in the conversion of the secret key from the bignum format to a bytestring. In source:usr/src/common/crypto/dh/dh_impl.c

 1  #ifdef _KERNEL
 2          if ((s = kmem_alloc(P2ROUNDUP_TYPED(prime_bytes,
 3              sizeof (BIG_CHUNK_SIZE), size_t))) == NULL) {
 4  #else
 5          if ((s = malloc(P2ROUNDUP_TYPED(prime_bytes,
 6              sizeof (BIG_CHUNK_SIZE), size_t))) == NULL) {
 7  #endif
 8                  rv = CKR_HOST_MEMORY;
 9                  goto ret;
10          }

In this instance, it will always allocate a size rounded up to a multiple of 4 since BIG_CHUNK_SIZE is either '32' or '64' (i.e. sizeof(32) sizeof(64) == sizeof (int)). However, the bignum library words on chunks BIG_CHUNK_SIZE bits (i.e. 4 or 8 bytes, depending on if a 32 or 64-bit object is being built). This means that depending on the size of prime_bytes, it could possibly allocate a buffer that is insufficient to hold the most significant 1-8 bytes of the secret key (it will be truncated). If *secretkey_len (the desired length of the secret key, since an app can request a key size smaller than sizeof (p)) is sufficiently large, this will truncate the secret key value.

Updated by Jason King over 3 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 90 to 100

Resolved with changeset 13203

Also available in: Atom PDF