On Sat, 19 Jun 2010 14:21:45 -0300 Klaus Heinrich Kiwi <[email protected]> wrote:
Please ignore this one as it was already sent in another thread. Saturday-noon sleepiness cause me to use the wrong starting point... -Klaus > The last couple of commits refactored some of the RSA mechanisms in > common/mech_rsa.c, mainly around PKCS#1 v1.5 padding handling. Those > also introduced a couple of issues that are being resolved by this > patch: > > * Segfault when rsa_format_block() was called with block type '2', > caused by un-initialized 'i' > * Possible presence of null-padding bytes generated by rng_generate() > rsa_format_block(), which would cause an invalid padding. Both this > and the above item were fixed by generating the random padding > bytes one-by-one, and replacing 0x00 by 0xff when needed. > * RSA Verify and VerifyRecover were calling rsa_format_block() with > block type '2', when the PKCS#1 specified block type '1'. > * rsa_parse_block() may return 'CKR_ENCRYPTED_DATA_INVALID' when it > verifies that the signature is invalid. PKCS#1 specifies that the > caller shouldn't be able to distinguish between padding errors and > invalid signatures. Fixed by adjusting some return codes as well as > the calling functions. RSA Verify and VerifyRecover should return > 'CKR_SIGNATURE_INVALID' > * Remove the '195 - RSA Parse block failed' log message as it may > indicate failure in decoding PKCS#1 v.1.5 padding, thus breaking > the spec. > > With the above, all tests in testcases/drivers/rsa_func.c are now > passing. > > Signed-off-by: Klaus Heinrich Kiwi <[email protected]> > --- > usr/lib/pkcs11/common/log.c | 4 +-- > usr/lib/pkcs11/common/mech_rsa.c | 50 > +++++++++++++++++++++++++------------- 2 files changed, 34 > insertions(+), 20 deletions(-) > > diff --git a/usr/lib/pkcs11/common/log.c b/usr/lib/pkcs11/common/log.c > index fd28e66..bce00b6 100755 > --- a/usr/lib/pkcs11/common/log.c > +++ b/usr/lib/pkcs11/common/log.c > @@ -673,8 +673,6 @@ struct messages err_msg[]={ > {"AES Wrap Get Data Failed"}, > {"AES Wrap Format Failed"}, > {"Domain Parameter Invalid"}, > - {"File \"%s\" could not be opened, errno=%d"}, > - //195 > - {"RSA Parse Block Failed"} > + {"File \"%s\" could not be opened, errno=%d"} > }; > > diff --git a/usr/lib/pkcs11/common/mech_rsa.c > b/usr/lib/pkcs11/common/mech_rsa.c index b0b0f34..076317c 100755 > --- a/usr/lib/pkcs11/common/mech_rsa.c > +++ b/usr/lib/pkcs11/common/mech_rsa.c > @@ -417,13 +417,19 @@ rsa_format_block( CK_BYTE * in_data, > * Where ?? is nonzero. > */ > case 2: > - rc = rng_generate(&out_data[2], padding_len); > - if (rc != CKR_OK){ > - st_err_log(130, __FILE__, __LINE__); > - return rc; > + for (i = 2; i < (padding_len + 2); i++) { > + rc = rng_generate(&out_data[i], 1); > + if (rc != CKR_OK) { > + st_err_log(130, __FILE__, __LINE__); > + return rc; > + } > + if (out_data[i] == (CK_BYTE)0) { > + /* avoid zeros by explicitly making them all > 0xff - > + * won't hurt entropy that bad, and it's better > than > + * looping over rng_generate */ > + out_data[i] = (CK_BYTE)0xff; > + } > } > - i = i + padding_len; > - > break; > > default: > @@ -564,14 +570,14 @@ rsa_parse_block( CK_BYTE * in_data, > * encryption blocks. > */ > if ((type == 1 || type == 2) && ((i - 3) < 8)) { > - st_err_log(112, __FILE__, __LINE__); > - rc = CKR_ENCRYPTED_DATA_LEN_RANGE; > + st_err_log(14, __FILE__, __LINE__); > + rc = CKR_ENCRYPTED_DATA_INVALID; > return rc; > } > > if (in_data_len <= i) { > - st_err_log(112, __FILE__, __LINE__); > - rc = CKR_ENCRYPTED_DATA_LEN_RANGE; > + st_err_log(14, __FILE__, __LINE__); > + rc = CKR_ENCRYPTED_DATA_INVALID; > return rc; > } > > @@ -711,7 +717,7 @@ rsa_pkcs_decrypt( SESSION *sess, > > rc = rsa_parse_block(out, modulus_bytes, out_data, out_data_len, > PKCS_BT_2); if (rc != CKR_OK) { > - st_err_log(195, __FILE__, __LINE__); > + st_err_log(133, __FILE__, __LINE__); > return rc; > } > > @@ -834,7 +840,7 @@ rsa_pkcs_verify( SESSION * sess, > if (rc == CKR_OK) { > CK_ULONG len; > > - rc = rsa_parse_block( out, modulus_bytes, out_data, > &out_data_len, PKCS_BT_2); > + rc = rsa_parse_block( out, modulus_bytes, out_data, > &out_data_len, PKCS_BT_1); if (rc == CKR_OK) { > if (in_data_len != out_data_len){ > st_err_log(47, __FILE__, __LINE__); > @@ -846,9 +852,13 @@ rsa_pkcs_verify( SESSION * sess, > return CKR_SIGNATURE_INVALID; > } > } > - else > - st_err_log(195, __FILE__, __LINE__); > - > + else if (rc == CKR_ENCRYPTED_DATA_INVALID ) { > + st_err_log(47, __FILE__, __LINE__); > + return CKR_SIGNATURE_INVALID; > + } else { > + st_err_log(4, __FILE__, __LINE__, __FUNCTION__); > + return CKR_FUNCTION_FAILED; > + } > } > else > st_err_log(132, __FILE__, __LINE__); > @@ -908,8 +918,14 @@ rsa_pkcs_verify_recover( SESSION * > sess, // > rc = ckm_rsa_encrypt( signature, modulus_bytes, out, key_obj ); > if (rc == CKR_OK) { > - rc = rsa_parse_block(out, modulus_bytes, out_data, > out_data_len, PKCS_BT_2); > - return rc; > + rc = rsa_parse_block(out, modulus_bytes, out_data, > out_data_len, PKCS_BT_1); > + if (rc == CKR_ENCRYPTED_DATA_INVALID ) { > + st_err_log(47, __FILE__, __LINE__); > + return CKR_SIGNATURE_INVALID; > + } else if (rc != CKR_OK) { > + st_err_log(4, __FILE__, __LINE__, __FUNCTION__); > + return rc; > + } > } > else > st_err_log(132, __FILE__, __LINE__); -- Klaus Heinrich Kiwi | [email protected] | http://blog.klauskiwi.com Open Source Security blog : http://www.ratliff.net/blog IBM Linux Technology Center : http://www.ibm.com/linux/ltc ------------------------------------------------------------------------------ ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo _______________________________________________ Opencryptoki-tech mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opencryptoki-tech
