[PATCH 0/4] MR3729: crypt32: Avoid truncating unicode chars.
From: Dmitry Timoshkov <dmitry(a)baikal.ru> So that strings containing Cyrillic Capital Letter ER (0x420) won't be quoted when not needed. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/crypt32/str.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/crypt32/str.c b/dlls/crypt32/str.c index 855e5a1c123..63333e5f736 100644 --- a/dlls/crypt32/str.c +++ b/dlls/crypt32/str.c @@ -195,9 +195,9 @@ static DWORD quote_rdn_value_to_str_w(DWORD dwValueType, case CERT_RDN_BMP_STRING: case CERT_RDN_UTF8_STRING: strLen = len = pValue->cbData / sizeof(WCHAR); - if (pValue->cbData && isspace(pValue->pbData[0])) + if (pValue->cbData && iswspace(((LPCWSTR)pValue->pbData)[0])) needsQuotes = TRUE; - if (pValue->cbData && isspace(pValue->pbData[strLen - 1])) + if (pValue->cbData && iswspace(((LPCWSTR)pValue->pbData)[strLen - 1])) needsQuotes = TRUE; for (i = 0; i < strLen; i++) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3729
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/crypt32/tests/str.c | 145 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/dlls/crypt32/tests/str.c b/dlls/crypt32/tests/str.c index 9e4ca37bcd6..58a4e971f15 100644 --- a/dlls/crypt32/tests/str.c +++ b/dlls/crypt32/tests/str.c @@ -23,6 +23,7 @@ #include <winbase.h> #include <winerror.h> #include <wincrypt.h> +#include <winnls.h> #include "wine/test.h" @@ -1079,8 +1080,152 @@ static void test_CertGetNameString(void) CertFreeCertificateContext(context); } +static void test_quoted_RDN(void) +{ + static const struct + { + const char *CN; + const char *X500_CN; + } test[] = + { + { "1", "1" }, + { " 1", "\" 1\"" }, + { "1 ", "\"1 \"" }, + { "\"1\"", "\"\"\"1\"\"\"" }, + { "\" 1 \"", "\"\"\" 1 \"\"\"" }, + { "\"\"\"1\"\"\"", "\"\"\"\"\"\"\"1\"\"\"\"\"\"\"" }, + { "1+", "\"1+\"" }, + { "1=", "\"1=\"" }, + { "1\"", "\"1\"\"\"" }, + { "1<", "\"1<\"" }, + { "1>", "\"1>\"" }, + { "1#", "\"1#\"" }, + { "1+", "\"1+\"" }, + }; + CERT_RDN_ATTR attr; + CERT_RDN rdn; + CERT_NAME_INFO info; + CERT_NAME_BLOB blob; + BYTE *buf; + char str[256]; + WCHAR strW[256]; + DWORD size, ret, i; + + for (i = 0; i < ARRAY_SIZE(test); i++) + { + winetest_push_context("%lu", i); + + attr.pszObjId = (LPSTR)szOID_COMMON_NAME; + attr.dwValueType = CERT_RDN_PRINTABLE_STRING; + attr.Value.cbData = strlen(test[i].CN); + attr.Value.pbData = (BYTE *)test[i].CN; + rdn.cRDNAttr = 1; + rdn.rgRDNAttr = &attr; + info.cRDN = 1; + info.rgRDN = &rdn; + buf = NULL; + size = 0; + ret = CryptEncodeObjectEx(X509_ASN_ENCODING, X509_NAME, &info, CRYPT_ENCODE_ALLOC_FLAG, NULL, &buf, &size); + ok(ret, "CryptEncodeObjectEx error %08lx\n", GetLastError()); + + blob.pbData = buf; + blob.cbData = size; + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, 0, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strcmp(str, test[i].X500_CN), "got %s, expected %s\n", str, test[i].X500_CN); + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_SIMPLE_NAME_STR, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strcmp(str, test[i].X500_CN), "got %s, expected %s\n", str, test[i].X500_CN); + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_OID_NAME_STR, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strncmp(str, "2.5.4.3=", 8), "got %s\n", str); + ok(!strcmp(&str[8], test[i].X500_CN), "got %s, expected %s\n", &str[8], test[i].X500_CN); + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_X500_NAME_STR, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strncmp(str, "CN=", 3), "got %s\n", str); + ok(!strcmp(&str[3], test[i].X500_CN), "got %s, expected %s\n", &str[3], test[i].X500_CN); + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_X500_NAME_STR | CERT_NAME_STR_NO_QUOTING_FLAG, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strncmp(str, "CN=", 3), "got %s\n", str); + todo_wine_if(i != 0) + ok(!strcmp(&str[3], test[i].CN), "got %s, expected %s\n", &str[3], test[i].CN); + + LocalFree(buf); + + winetest_pop_context(); + } + + for (i = 0; i < ARRAY_SIZE(test); i++) + { + winetest_push_context("%lu", i); + + ret = MultiByteToWideChar(CP_ACP, 0, test[i].CN, strlen(test[i].CN), strW, ARRAY_SIZE(strW)); + ok(ret, "MultiByteToWideChar error %lu\n", GetLastError()); + + attr.pszObjId = (LPSTR)szOID_COMMON_NAME; + attr.dwValueType = CERT_RDN_UTF8_STRING; + attr.Value.cbData = ret * sizeof(WCHAR); + attr.Value.pbData = (BYTE *)strW; + rdn.cRDNAttr = 1; + rdn.rgRDNAttr = &attr; + info.cRDN = 1; + info.rgRDN = &rdn; + buf = NULL; + size = 0; + ret = CryptEncodeObjectEx(X509_ASN_ENCODING, X509_NAME, &info, CRYPT_ENCODE_ALLOC_FLAG, NULL, &buf, &size); + ok(ret, "CryptEncodeObjectEx error %08lx\n", GetLastError()); + + blob.pbData = buf; + blob.cbData = size; + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, 0, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strcmp(str, test[i].X500_CN), "got %s, expected %s\n", str, test[i].X500_CN); + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_SIMPLE_NAME_STR, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strcmp(str, test[i].X500_CN), "got %s, expected %s\n", str, test[i].X500_CN); + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_OID_NAME_STR, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strncmp(str, "2.5.4.3=", 8), "got %s\n", str); + ok(!strcmp(&str[8], test[i].X500_CN), "got %s, expected %s\n", &str[8], test[i].X500_CN); + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_X500_NAME_STR, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strncmp(str, "CN=", 3), "got %s\n", str); + ok(!strcmp(&str[3], test[i].X500_CN), "got %s, expected %s\n", &str[3], test[i].X500_CN); + + str[0] = 0; + ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_X500_NAME_STR | CERT_NAME_STR_NO_QUOTING_FLAG, str, sizeof(str)); + ok(ret, "CertNameToStr error %08lx\n", GetLastError()); + ok(!strncmp(str, "CN=", 3), "got %s\n", str); + todo_wine_if(i != 0) + ok(!strcmp(&str[3], test[i].CN), "got %s, expected %s\n", &str[3], test[i].CN); + + LocalFree(buf); + + winetest_pop_context(); + } +} + START_TEST(str) { + test_quoted_RDN(); test_CertRDNValueToStrA(); test_CertRDNValueToStrW(); test_CertNameToStrA(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3729
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/crypt32/str.c | 61 ++++++++++++++++++++++------------------ dlls/crypt32/tests/str.c | 2 -- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/dlls/crypt32/str.c b/dlls/crypt32/str.c index 63333e5f736..50c05dd88bd 100644 --- a/dlls/crypt32/str.c +++ b/dlls/crypt32/str.c @@ -139,8 +139,8 @@ static inline BOOL is_quotable_char(WCHAR c) } } -static DWORD quote_rdn_value_to_str_w(DWORD dwValueType, - PCERT_RDN_VALUE_BLOB pValue, LPWSTR psz, DWORD csz) +static DWORD quote_rdn_value_to_str_w(DWORD dwValueType, PCERT_RDN_VALUE_BLOB pValue, + DWORD dwStrType, LPWSTR psz, DWORD csz) { DWORD ret = 0, len, i, strLen; BOOL needsQuotes = FALSE; @@ -160,19 +160,22 @@ static DWORD quote_rdn_value_to_str_w(DWORD dwValueType, case CERT_RDN_VISIBLE_STRING: case CERT_RDN_GENERAL_STRING: len = pValue->cbData; - if (pValue->cbData && isspace(pValue->pbData[0])) - needsQuotes = TRUE; - if (pValue->cbData && isspace(pValue->pbData[pValue->cbData - 1])) - needsQuotes = TRUE; - for (i = 0; i < pValue->cbData; i++) + if (!(dwStrType & CERT_NAME_STR_NO_QUOTING_FLAG)) { - if (is_quotable_char(pValue->pbData[i])) + if (pValue->cbData && isspace(pValue->pbData[0])) needsQuotes = TRUE; - if (pValue->pbData[i] == '"') - len += 1; + if (pValue->cbData && isspace(pValue->pbData[pValue->cbData - 1])) + needsQuotes = TRUE; + for (i = 0; i < pValue->cbData; i++) + { + if (is_quotable_char(pValue->pbData[i])) + needsQuotes = TRUE; + if (pValue->pbData[i] == '"') + len += 1; + } + if (needsQuotes) + len += 2; } - if (needsQuotes) - len += 2; if (!psz || !csz) ret = len; else @@ -184,7 +187,8 @@ static DWORD quote_rdn_value_to_str_w(DWORD dwValueType, for (i = 0; i < pValue->cbData && ptr - psz < csz; ptr++, i++) { *ptr = pValue->pbData[i]; - if (pValue->pbData[i] == '"' && ptr - psz < csz - 1) + if (!(dwStrType & CERT_NAME_STR_NO_QUOTING_FLAG) && + pValue->pbData[i] == '"' && ptr - psz < csz - 1) *(++ptr) = '"'; } if (needsQuotes && ptr - psz < csz) @@ -195,19 +199,22 @@ static DWORD quote_rdn_value_to_str_w(DWORD dwValueType, case CERT_RDN_BMP_STRING: case CERT_RDN_UTF8_STRING: strLen = len = pValue->cbData / sizeof(WCHAR); - if (pValue->cbData && iswspace(((LPCWSTR)pValue->pbData)[0])) - needsQuotes = TRUE; - if (pValue->cbData && iswspace(((LPCWSTR)pValue->pbData)[strLen - 1])) - needsQuotes = TRUE; - for (i = 0; i < strLen; i++) + if (!(dwStrType & CERT_NAME_STR_NO_QUOTING_FLAG)) { - if (is_quotable_char(((LPCWSTR)pValue->pbData)[i])) + if (pValue->cbData && iswspace(((LPCWSTR)pValue->pbData)[0])) needsQuotes = TRUE; - if (((LPCWSTR)pValue->pbData)[i] == '"') - len += 1; + if (pValue->cbData && iswspace(((LPCWSTR)pValue->pbData)[strLen - 1])) + needsQuotes = TRUE; + for (i = 0; i < strLen; i++) + { + if (is_quotable_char(((LPCWSTR)pValue->pbData)[i])) + needsQuotes = TRUE; + if (((LPCWSTR)pValue->pbData)[i] == '"') + len += 1; + } + if (needsQuotes) + len += 2; } - if (needsQuotes) - len += 2; if (!psz || !csz) ret = len; else @@ -219,7 +226,8 @@ static DWORD quote_rdn_value_to_str_w(DWORD dwValueType, for (i = 0; i < strLen && ptr - psz < csz; ptr++, i++) { *ptr = ((LPCWSTR)pValue->pbData)[i]; - if (((LPCWSTR)pValue->pbData)[i] == '"' && ptr - psz < csz - 1) + if (!(dwStrType & CERT_NAME_STR_NO_QUOTING_FLAG) && + ((LPCWSTR)pValue->pbData)[i] == '"' && ptr - psz < csz - 1) *(++ptr) = '"'; } if (needsQuotes && ptr - psz < csz) @@ -323,8 +331,7 @@ static const WCHAR indent[] = L" "; DWORD cert_name_to_str_with_indent(DWORD dwCertEncodingType, DWORD indentLevel, const CERT_NAME_BLOB *pName, DWORD dwStrType, LPWSTR psz, DWORD csz) { - static const DWORD unsupportedFlags = CERT_NAME_STR_NO_QUOTING_FLAG | - CERT_NAME_STR_ENABLE_T61_UNICODE_FLAG; + static const DWORD unsupportedFlags = CERT_NAME_STR_ENABLE_T61_UNICODE_FLAG; DWORD ret = 0, bytes = 0; BOOL bRet; CERT_NAME_INFO *info; @@ -414,7 +421,7 @@ DWORD cert_name_to_str_with_indent(DWORD dwCertEncodingType, DWORD indentLevel, } if (psz && ret + 1 == csz) break; - chars = quote_rdn_value_to_str_w(rdn->rgRDNAttr[j].dwValueType, &rdn->rgRDNAttr[j].Value, + chars = quote_rdn_value_to_str_w(rdn->rgRDNAttr[j].dwValueType, &rdn->rgRDNAttr[j].Value, dwStrType, psz ? psz + ret : NULL, psz ? csz - ret - 1 : 0); ret += chars; if (j < rdn->cRDNAttr - 1) diff --git a/dlls/crypt32/tests/str.c b/dlls/crypt32/tests/str.c index 58a4e971f15..8510fed00ec 100644 --- a/dlls/crypt32/tests/str.c +++ b/dlls/crypt32/tests/str.c @@ -1157,7 +1157,6 @@ static void test_quoted_RDN(void) ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_X500_NAME_STR | CERT_NAME_STR_NO_QUOTING_FLAG, str, sizeof(str)); ok(ret, "CertNameToStr error %08lx\n", GetLastError()); ok(!strncmp(str, "CN=", 3), "got %s\n", str); - todo_wine_if(i != 0) ok(!strcmp(&str[3], test[i].CN), "got %s, expected %s\n", &str[3], test[i].CN); LocalFree(buf); @@ -1214,7 +1213,6 @@ static void test_quoted_RDN(void) ret = CertNameToStrA(X509_ASN_ENCODING, &blob, CERT_X500_NAME_STR | CERT_NAME_STR_NO_QUOTING_FLAG, str, sizeof(str)); ok(ret, "CertNameToStr error %08lx\n", GetLastError()); ok(!strncmp(str, "CN=", 3), "got %s\n", str); - todo_wine_if(i != 0) ok(!strcmp(&str[3], test[i].CN), "got %s, expected %s\n", &str[3], test[i].CN); LocalFree(buf); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3729
From: Dmitry Timoshkov <dmitry(a)baikal.ru> This makes the certificate selecting UI look similar to Windows. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/cryptui/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/cryptui/main.c b/dlls/cryptui/main.c index ee5d7037366..2bd6b0b9f6a 100644 --- a/dlls/cryptui/main.c +++ b/dlls/cryptui/main.c @@ -2598,14 +2598,14 @@ static WCHAR *field_format_detailed_cert_name(PCERT_NAME_BLOB name) { WCHAR *str = NULL; DWORD len = CertNameToStrW(X509_ASN_ENCODING, name, - CERT_X500_NAME_STR | CERT_NAME_STR_CRLF_FLAG, NULL, 0); + CERT_X500_NAME_STR | CERT_NAME_STR_CRLF_FLAG | CERT_NAME_STR_NO_QUOTING_FLAG, NULL, 0); if (len) { str = malloc(len * sizeof(WCHAR)); if (str) CertNameToStrW(X509_ASN_ENCODING, name, - CERT_X500_NAME_STR | CERT_NAME_STR_CRLF_FLAG, str, len); + CERT_X500_NAME_STR | CERT_NAME_STR_CRLF_FLAG | CERT_NAME_STR_NO_QUOTING_FLAG, str, len); } return str; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3729
Is it possible to get a review of this patchset please? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3729#note_45482
It would need tests with other Unicode space characters (u00a0, u3000, etc.) to show that iswspace() is the right thing to use. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3729#note_45495
It would need tests with other Unicode space characters (u00a0, u3000, etc.) to show that iswspace() is the right thing to use.
This patch is about avoiding truncation of unicode chararacters (in my case it's 0x420) to 'char'. If iswspace() is not covering full unicode range, and something else should be used instead (like IsCharSpaceW()) that could be a separate change with appropriate tests. I can look at it later, if desired. Does that sound acceptable? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3729#note_45515
On Fri Sep 15 10:53:50 2023 +0000, Dmitry Timoshkov wrote:
It would need tests with other Unicode space characters (u00a0, u3000, etc.) to show that iswspace() is the right thing to use. This patch is about avoiding truncation of unicode chararacters (in my case it's 0x420) to 'char'. If iswspace() is not covering full unicode range, and something else should be used instead (like IsCharSpaceW()) that could be a separate change with appropriate tests. I can look at it later, if desired. Does that sound acceptable? You are already adding tests, it doesn't seem hard to extend them to other space characters. Otherwise you'd need to explicitly check for 0x20 since that's the only thing that's tested.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3729#note_45526
You are already adding tests, it doesn't seem hard to extend them to other space characters. Otherwise you'd need to explicitly check for 0x20 since that's the only thing that's tested.
Right, I failed to realize that the being added tests already cover ' ' as a space character, and the reviewer could be confused by it, while the actual intent was different. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3729#note_45527
participants (3)
-
Alexandre Julliard (＠julliard) -
Dmitry Timoshkov
-
Dmitry Timoshkov (＠dmitry)