From 1194ea8dc3b51a35c9947ed276f38436abee5743 Mon Sep 17 00:00:00 2001 From: Andy Polyakov Date: Tue, 26 Jul 2016 16:42:41 +0200 Subject: [PATCH] crypto/pkcs12: facilitate accessing data with non-interoperable password. Originally PKCS#12 subroutines treated password strings as ASCII. It worked as long as they were pure ASCII, but if there were some none-ASCII characters result was non-interoperable. But fixing it poses problem accessing data protected with broken password. In order to make asscess to old data possible add retry with old-style password. Reviewed-by: Richard Levitte --- .gitattributes | 1 + crypto/pkcs12/p12_crpt.c | 31 +++++++++++-- crypto/pkcs12/p12_lcl.h | 9 ++++ crypto/pkcs12/p12_mutl.c | 84 +++++++++++++++++++++++++++++++---- crypto/pkcs12/p12_utl.c | 9 +++- doc/apps/pkcs12.pod | 10 +++++ include/openssl/pkcs12.h | 16 ++----- test/recipes/80-test_pkcs12.t | 11 +++++ 8 files changed, 144 insertions(+), 27 deletions(-) diff --git a/.gitattributes b/.gitattributes index f33e22acf6..15121c861c 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,2 +1,3 @@ *.der binary /fuzz/corpora/** binary +*.pfx binary diff --git a/crypto/pkcs12/p12_crpt.c b/crypto/pkcs12/p12_crpt.c index 1fe140ad0a..d30aab379a 100644 --- a/crypto/pkcs12/p12_crpt.c +++ b/crypto/pkcs12/p12_crpt.c @@ -17,6 +17,16 @@ void PKCS12_PBE_add(void) { } +#undef PKCS12_key_gen +/* + * See p12_multi.c:PKCS12_verify_mac() for details... + */ +extern int (*PKCS12_key_gen)(const char *pass, int passlen, + unsigned char *salt, int slen, + int id, int iter, int n, + unsigned char *out, + const EVP_MD *md_type); + int PKCS12_PBE_keyivgen(EVP_CIPHER_CTX *ctx, const char *pass, int passlen, ASN1_TYPE *param, const EVP_CIPHER *cipher, const EVP_MD *md, int en_de) @@ -25,6 +35,19 @@ int PKCS12_PBE_keyivgen(EVP_CIPHER_CTX *ctx, const char *pass, int passlen, int saltlen, iter, ret; unsigned char *salt; unsigned char key[EVP_MAX_KEY_LENGTH], iv[EVP_MAX_IV_LENGTH]; + int (*pkcs12_key_gen)(const char *pass, int passlen, + unsigned char *salt, int slen, + int id, int iter, int n, + unsigned char *out, + const EVP_MD *md_type); + + if (PKCS12_key_gen == NULL || en_de) + /* + * Default to UTF-8, but force it in encrypt case. + */ + pkcs12_key_gen = PKCS12_key_gen_utf8; + else + pkcs12_key_gen = PKCS12_key_gen; if (cipher == NULL) return 0; @@ -43,14 +66,14 @@ int PKCS12_PBE_keyivgen(EVP_CIPHER_CTX *ctx, const char *pass, int passlen, iter = ASN1_INTEGER_get(pbe->iter); salt = pbe->salt->data; saltlen = pbe->salt->length; - if (!PKCS12_key_gen(pass, passlen, salt, saltlen, PKCS12_KEY_ID, - iter, EVP_CIPHER_key_length(cipher), key, md)) { + if (!(*pkcs12_key_gen)(pass, passlen, salt, saltlen, PKCS12_KEY_ID, + iter, EVP_CIPHER_key_length(cipher), key, md)) { PKCS12err(PKCS12_F_PKCS12_PBE_KEYIVGEN, PKCS12_R_KEY_GEN_ERROR); PBEPARAM_free(pbe); return 0; } - if (!PKCS12_key_gen(pass, passlen, salt, saltlen, PKCS12_IV_ID, - iter, EVP_CIPHER_iv_length(cipher), iv, md)) { + if (!(*pkcs12_key_gen)(pass, passlen, salt, saltlen, PKCS12_IV_ID, + iter, EVP_CIPHER_iv_length(cipher), iv, md)) { PKCS12err(PKCS12_F_PKCS12_PBE_KEYIVGEN, PKCS12_R_IV_GEN_ERROR); PBEPARAM_free(pbe); return 0; diff --git a/crypto/pkcs12/p12_lcl.h b/crypto/pkcs12/p12_lcl.h index 8930875153..9a27f2fa9e 100644 --- a/crypto/pkcs12/p12_lcl.h +++ b/crypto/pkcs12/p12_lcl.h @@ -42,3 +42,12 @@ struct pkcs12_bag_st { } value; }; +#undef PKCS12_key_gen +/* + * See p12_multi.c:PKCS12_verify_mac() for details... + */ +extern int (*PKCS12_key_gen)(const char *pass, int passlen, + unsigned char *salt, int slen, + int id, int iter, int n, + unsigned char *out, + const EVP_MD *md_type); diff --git a/crypto/pkcs12/p12_mutl.c b/crypto/pkcs12/p12_mutl.c index 79639c2169..325da0c207 100644 --- a/crypto/pkcs12/p12_mutl.c +++ b/crypto/pkcs12/p12_mutl.c @@ -66,9 +66,40 @@ static int pkcs12_gen_gost_mac_key(const char *pass, int passlen, return 1; } +#undef PKCS12_key_gen +/* + * |PKCS12_key_gen| is used to convey information about old-style broken + * password being used to PKCS12_PBE_keyivgen in decrypt cases. Workflow + * is if PKCS12_verify_mac notes that password encoded with compliant + * PKCS12_key_gen_utf8 conversion subroutine isn't right, while encoded + * with legacy non-compliant one is, then it sets |PKCS12_key_gen| to + * legacy PKCS12_key_gen_asc conversion subroutine, which is then picked + * by PKCS12_PBE_keyivgen. This applies to reading data. Written data + * on the other hand is protected with standard-compliant encoding, i.e. + * in backward-incompatible manner. Note that formally the approach is + * not MT-safe. Rationale is that in order to access PKCS#12 files from + * MT or even production application, you would be required to convert + * data to correct interoperable format. In which case this variable + * won't have to change. Conversion would have to be done with pkcs12 + * utility, which is not MT, and hence can tolerate it. In other words + * goal is not to make this heuristic approach work in general case, + * but in one specific one, apps/pkcs12.c. + */ +int (*PKCS12_key_gen)(const char *pass, int passlen, + unsigned char *salt, int slen, + int id, int iter, int n, + unsigned char *out, + const EVP_MD *md_type) = NULL; + + /* Generate a MAC */ -int PKCS12_gen_mac(PKCS12 *p12, const char *pass, int passlen, - unsigned char *mac, unsigned int *maclen) +static int pkcs12_gen_mac(PKCS12 *p12, const char *pass, int passlen, + unsigned char *mac, unsigned int *maclen, + int (*pkcs12_key_gen)(const char *pass, int passlen, + unsigned char *salt, int slen, + int id, int iter, int n, + unsigned char *out, + const EVP_MD *md_type)) { const EVP_MD *md_type; HMAC_CTX *hmac = NULL; @@ -79,6 +110,11 @@ int PKCS12_gen_mac(PKCS12 *p12, const char *pass, int passlen, const X509_ALGOR *macalg; const ASN1_OBJECT *macoid; + if (pkcs12_key_gen == NULL) + pkcs12_key_gen = PKCS12_key_gen; + if (pkcs12_key_gen == NULL) + pkcs12_key_gen = PKCS12_key_gen_utf8; + if (!PKCS7_type_is_data(p12->authsafes)) { PKCS12err(PKCS12_F_PKCS12_GEN_MAC, PKCS12_R_CONTENT_TYPE_NOT_DATA); return 0; @@ -111,8 +147,8 @@ int PKCS12_gen_mac(PKCS12 *p12, const char *pass, int passlen, return 0; } } else - if (!PKCS12_key_gen(pass, passlen, salt, saltlen, PKCS12_MAC_ID, iter, - md_size, key, md_type)) { + if (!(*pkcs12_key_gen)(pass, passlen, salt, saltlen, PKCS12_MAC_ID, + iter, md_size, key, md_type)) { PKCS12err(PKCS12_F_PKCS12_GEN_MAC, PKCS12_R_KEY_GEN_ERROR); return 0; } @@ -128,6 +164,12 @@ int PKCS12_gen_mac(PKCS12 *p12, const char *pass, int passlen, return 1; } +int PKCS12_gen_mac(PKCS12 *p12, const char *pass, int passlen, + unsigned char *mac, unsigned int *maclen) +{ + return pkcs12_gen_mac(p12, pass, passlen, mac, maclen, NULL); +} + /* Verify the mac */ int PKCS12_verify_mac(PKCS12 *p12, const char *pass, int passlen) { @@ -139,14 +181,36 @@ int PKCS12_verify_mac(PKCS12 *p12, const char *pass, int passlen) PKCS12err(PKCS12_F_PKCS12_VERIFY_MAC, PKCS12_R_MAC_ABSENT); return 0; } - if (!PKCS12_gen_mac(p12, pass, passlen, mac, &maclen)) { + if (!pkcs12_gen_mac(p12, pass, passlen, mac, &maclen, + PKCS12_key_gen_utf8)) { PKCS12err(PKCS12_F_PKCS12_VERIFY_MAC, PKCS12_R_MAC_GENERATION_ERROR); return 0; } X509_SIG_get0(p12->mac->dinfo, NULL, &macoct); - if ((maclen != (unsigned int)ASN1_STRING_length(macoct)) - || CRYPTO_memcmp(mac, ASN1_STRING_get0_data(macoct), maclen)) + if (maclen != (unsigned int)ASN1_STRING_length(macoct)) return 0; + + if (CRYPTO_memcmp(mac, ASN1_STRING_get0_data(macoct), maclen) != 0) { + if (pass == NULL) + return 0; + /* + * In order to facilitate accessing old data retry with + * old-style broken password ... + */ + if (!pkcs12_gen_mac(p12, pass, passlen, mac, &maclen, + PKCS12_key_gen_asc)) { + PKCS12err(PKCS12_F_PKCS12_VERIFY_MAC, PKCS12_R_MAC_GENERATION_ERROR); + return 0; + } + if ((maclen != (unsigned int)ASN1_STRING_length(macoct)) + || CRYPTO_memcmp(mac, ASN1_STRING_get0_data(macoct), maclen) != 0) + return 0; + else + PKCS12_key_gen = PKCS12_key_gen_asc; + /* + * ... and if suceeded, pass it on to PKCS12_PBE_keyivgen. + */ + } return 1; } @@ -166,7 +230,11 @@ int PKCS12_set_mac(PKCS12 *p12, const char *pass, int passlen, PKCS12err(PKCS12_F_PKCS12_SET_MAC, PKCS12_R_MAC_SETUP_ERROR); return 0; } - if (!PKCS12_gen_mac(p12, pass, passlen, mac, &maclen)) { + /* + * Note that output mac is forced to UTF-8... + */ + if (!pkcs12_gen_mac(p12, pass, passlen, mac, &maclen, + PKCS12_key_gen_utf8)) { PKCS12err(PKCS12_F_PKCS12_SET_MAC, PKCS12_R_MAC_GENERATION_ERROR); return 0; } diff --git a/crypto/pkcs12/p12_utl.c b/crypto/pkcs12/p12_utl.c index 7d471f5c0d..07014786f6 100644 --- a/crypto/pkcs12/p12_utl.c +++ b/crypto/pkcs12/p12_utl.c @@ -173,11 +173,16 @@ char *OPENSSL_uni2utf8(const unsigned char *uni, int unilen) int asclen, i, j; char *asctmp; + /* string must contain an even number of bytes */ + if (unilen & 1) + return NULL; + for (asclen = 0, i = 0; i < unilen; ) { j = bmp_to_utf8(NULL, uni+i, unilen-i); /* - * falling back to OPENSSL_uni2asc makes lesser sense, it's - * done rather to maintain symmetry... + * falling back to OPENSSL_uni2asc makes lesser sense [than + * falling back to OPENSSL_asc2uni in OPENSSL_utf82uni above], + * it's done rather to maintain symmetry... */ if (j < 0) return OPENSSL_uni2asc(uni, unilen); if (j == 4) i += 4; diff --git a/doc/apps/pkcs12.pod b/doc/apps/pkcs12.pod index 2f2c4d143d..e851018cfd 100644 --- a/doc/apps/pkcs12.pod +++ b/doc/apps/pkcs12.pod @@ -325,6 +325,16 @@ encrypted private keys, then the option B<-keypbe PBE-SHA1-RC2-40> can be used to reduce the private key encryption to 40 bit RC2. A complete description of all algorithms is contained in the B manual page. +Prior 1.1 release passwords containing non-ASCII characters were encoded +in non-compliant manner, which limited interoperability, in first hand +with Windows. But switching to standard-compliant password encoding +poses problem accessing old data protected with broken encoding. For +this reason even legacy encodings is attempted when reading the +data. If you use PKCS#12 files in production application you are advised +to convert the data, because implemented heuristic approach is not +MT-safe, its sole goal is to facilitate the data upgrade with this +utility. + =head1 EXAMPLES Parse a PKCS#12 file and output it to a file: diff --git a/include/openssl/pkcs12.h b/include/openssl/pkcs12.h index 37e2847b3f..deaded9df9 100644 --- a/include/openssl/pkcs12.h +++ b/include/openssl/pkcs12.h @@ -30,19 +30,9 @@ extern "C" { # define PKCS12_SALT_LEN 8 -/* Uncomment out next line for unicode password and names, otherwise ASCII */ - -/* - * #define PBE_UNICODE - */ - -# ifdef PBE_UNICODE -# define PKCS12_key_gen PKCS12_key_gen_uni -# define PKCS12_add_friendlyname PKCS12_add_friendlyname_uni -# else -# define PKCS12_key_gen PKCS12_key_gen_utf8 -# define PKCS12_add_friendlyname PKCS12_add_friendlyname_utf8 -# endif +/* It's not clear if these are actually needed... */ +# define PKCS12_key_gen PKCS12_key_gen_utf8 +# define PKCS12_add_friendlyname PKCS12_add_friendlyname_utf8 /* MS key usage constants */ diff --git a/test/recipes/80-test_pkcs12.t b/test/recipes/80-test_pkcs12.t index 681ce45fcb..bb3e0c84ee 100644 --- a/test/recipes/80-test_pkcs12.t +++ b/test/recipes/80-test_pkcs12.t @@ -20,6 +20,17 @@ if (eval { require Win32::Console; 1; }) { $savedcp = Win32::Console::OutputCP(); Win32::Console::OutputCP(1253); $pass = Encode::encode("cp1253",Encode::decode("utf-8",$pass)); +} else { + # Running MinGW tests transparenly under Wine apparently requires + # UTF-8 locale... + + foreach(`locale -a`) { + s/\R$//; + if ($_ =~ m/^C\.UTF\-?8/i) { + $ENV{LC_ALL} = $_; + last; + } + } } # just see that we can read shibboleth.pfx protected with $pass -- 2.25.1