This bug is a follow-up to #66701. It focusses on the original problem reported there. When trying to create a digitally signed PDF on Windows, the signature operation fails. This is taken from the original report: ----- Steps to reproduce: 1. Install Digital Certificate under Windows /IE by double click onto your .p12 file and follow the Certificate Import Wizard guidance. 2. Enable 'experimental features' under Tools / Options / LibreOffice / Advanced menu. 3. Export one of your document to PDF with Digital signature via File / Export to PDF /Digital signature tab. Choose one of your installed certificate and provide a password for that. 4. Open the created PDF file in Acrobat Reader. Current behavior: Malfunctioned signature reported by Adobe Reader: Signature validity unknown. Error during signature verification. Error encountered while BER decoding. ----- The resulting PDF indeed hat all zeroes where the signature should be. In the meantime Tor has worked on the problem and found out a lot of related background: > On Windows I double-clicked the .p12 file I used in the Linux case, and > Windows apparently added it fine to its certificate database. It shows up in > LibreOffice and I am able to try to use it to sign a PDF, but the NSS API > returns failure from the NSS_CMSEncoder_Finish() call, though. And more: > Seems that the situation on Windows is that the code is quite incomplete and > confused at the moment. On the “PDF Options” page that opens up for > File>Export as PDF, the “Select” button on the “Digital Signatures” tab opens > up a dialog that offers certificates that are stored in Windows’s own place > for such, they are accessed using Windows APIs. > But then the code that does the actual PDF signing is hardcoded to use only > NSS APIs. And NSS apparently has no access to the Windows certificate store, > so it can’t find the one it is told to use. And: > I think I now understand what is going on with the PDF signing on Windows. > > On Linux (and OS X), we use NSS for all this, and its certificate store. The > Tools>Options>LibreOffice>Security page has a "Certificate Path" setting, > which is a directory where the NSS cert8.db, key3.db and secmod.db files > should exist. > > The code that handles the selection of certificate (the "Select" button on > the "Digital Signatures" tab of the File>Export as PDF dialog) has two > "backends": a NSS one that is used on Linux and OS X, and a Windows one. > > The NSS backend calls NSS_Init() with the certificate path directory as > parameter. The dialog shows the certificates storted in this NSS database. I > don't know what the "user-friendly" way to manage this database is, but one > can use the horrible certutil or pk12util command-line tools to add > certificates there... > > This code is located in the xmlsecurity module, because it was originally > written for ODF-signing functionality. (And perhaps signing of older, now > already obsolete, XML-based document formats?) (No idea if this stuff is also > used when signing OOXML.) > > On Windows, the dialog shows the certificates stored the user's Windows- > managed certificate store. (One adds certificates there by simply double- > clicking on .p12 or .cer files.) No NSS_Init() is called, of course. > > So far, so good. > > But, the PDF signing code uses NSS directly. (It couldn't use xmlsecurity > anyway, as PDF has nothing to do with XML.) It calls NSS_NoDB_Init(), which > is the function to initialise NSS to work *without* any database (the > cert8.db etc files), but that is actually irrelevant on Linux, as it is the > first of any NSS_Init() and NSS_NoDB_Init() calls that counts, subsequent > calls are no-ops. So, on Linux, the NSS_Init() call done when fetching the > list of certificates, which passed in the directory from the "Certificate > Path" setting, stays in force. > > On Windows, however, this NSS_Nodb_Init() call is what initialises NSS. This > then means that eventually the NSS_CMSEncoder_Finish() call fails. I donut > know the exact mechanism here. Why it is that particular the NSS call that > fails, the next-to-last NSS API called, I have no idea. > > Just as a test, I changed the NSS_NoDB_Init() call to instead call > NSS_Init(), passing in the pathname to the directory on my Linux machine with > a NSS database where I had stored the same certificate that I had added to my > Windows certificate store. Then the signing works! But this is only of > academic interest of course. > > As I see it, there are two possible ways forward to get this stuff to work on > Windows: > > 1) Set up a NSS database also on Windows. One would think there is a way to > get the stuff to be added to that out of the Windows certificate store, using > Windows APIs, and then store it in the database, using NSS APIs. Make sure to > call NSS_Init() with the path to this database. Then the PDF signing code > needs no change. > > 2) Add a Windows-specific implementation of the PDF signing. It is a single > function, as far as I can see, PDFWriterImpl::finalizeSignature(), that needs > ifdefs. This probably makes more sense. Presumably one just needs to figure > out the right WinCrypt APIs to use instead of the NSS APIs. The concepts > should be fairly similar, one hopes. It shouldn't be more than a handful > function calls, knock on wood.
> As I see it, there are two possible ways forward to get this stuff to work on > Windows: > > 1) Set up a NSS database also on Windows. One would think there is a way to > get the stuff to be added to that out of the Windows certificate store, using > Windows APIs, and then store it in the database, using NSS APIs. Make sure to > call NSS_Init() with the path to this database. Then the PDF signing code > needs no change. > > 2) Add a Windows-specific implementation of the PDF signing. It is a single > function, as far as I can see, PDFWriterImpl::finalizeSignature(), that needs > ifdefs. This probably makes more sense. Presumably one just needs to figure > out the right WinCrypt APIs to use instead of the NSS APIs. The concepts > should be fairly similar, one hopes. It shouldn't be more than a handful > function calls, knock on wood. I think that the signatures should work with both methods. If the user configures a NSS store under Windows in Tools->Options->Security->Certificate Path it should work as well as when the default Keystore is used.
Tor Lillqvist committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=7e3c931786c3cbe83ee170b8b0746d141b520ce6 fdo#87030: PDF signing using Windows API, work in progress It will be available in 4.5.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Setting to NEW. @Tor, did you want to take this for yourself as assigned?
I don't believe in "assigning" bugs in bugzilla. That is not what directs what paid developers work on.
For reference, here is a diff with various hacks that I have tried to get it to work, but no luck. It is interesting that if I don't use the certificate created directly by CertCreateCertificateContext() from the DER data, but look it up from the "MY" store using CertFindCertificateInStore(), then CertEnumCertificateContextProperties() finds various properties in the certificate. Also, it is interesting that the certificate provided to me (the "Collabora Dev Softtoken" one) is an AT_KEYEXCHANGE one!? That is what certutil -dump -v and CryptAcquireCertificatePrivateKey() tell me. I would have expected it to be an AT_SIGNATURE one. But probably I misunderstand something. diff --git a/vcl/source/gdi/pdfwriter_impl.cxx b/vcl/source/gdi/pdfwriter_impl.cxx index b459a8d..ab9e2cf --- a/vcl/source/gdi/pdfwriter_impl.cxx +++ b/vcl/source/gdi/pdfwriter_impl.cxx @@ -6215,13 +6215,31 @@ bool PDFWriterImpl::finalizeSignature() #else - PCCERT_CONTEXT pCertContext = CertCreateCertificateContext(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, reinterpret_cast<const BYTE*>(n_derArray), n_derLength); - if (pCertContext == NULL) + PCCERT_CONTEXT pCertContextFromDER = CertCreateCertificateContext(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, reinterpret_cast<const BYTE*>(n_derArray), n_derLength); + if (pCertContextFromDER == NULL) { SAL_WARN("vcl.pdfwriter", "CertCreateCertificateContext failed: " << WindowsError(GetLastError())); return false; } +#if 1 + HCERTSTORE hCertStore = CertOpenSystemStore(NULL, "MY"); + if (!hCertStore) + { + SAL_WARN("vcl.pdfwriter", "CertOpenSystemStore failed: " << WindowsError(GetLastError())); + return false; + } + + PCCERT_CONTEXT pCertContext = CertFindCertificateInStore(hCertStore, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, 0, CERT_FIND_EXISTING, (const void*) pCertContextFromDER, NULL); + if (pCertContext == NULL) + { + SAL_WARN("vcl.pdfwriter", "CertFindCertificateInStore failed: " << WindowsError(GetLastError())); + return false; + } +#else + PCCERT_CONTEXT pCertContext = pCertContextFromDER; +#endif + #if SAL_LOG_INFO DWORD nProperty(0); bool first(true); @@ -6234,8 +6252,7 @@ bool PDFWriterImpl::finalizeSignature() DWORD nSize(0); if (!CertGetCertificateContextProperty(pCertContext, nProperty, NULL, &nSize)) SAL_INFO("vcl.pdfwriter", " " << "(missing?) " << std::hex << nProperty); - else - { + else { boost::scoped_array<char> aData(new char[nSize]); if (!CertGetCertificateContextProperty(pCertContext, nProperty, aData.get(), &nSize)) SAL_INFO("vcl.pdfwriter", " " << "(missing?) " << std::hex:: << nProperty); @@ -6243,7 +6260,7 @@ bool PDFWriterImpl::finalizeSignature() SAL_INFO("vcl.pdfwriter", " " << CertificatePropertyNameAndData(nProperty, aData, nSize)); } #else - SAL_INFO("vcl.pdfwriter", " " << std::hex << nProperty); + SAL_INFO("vcl.pdfwriter", " " << nProperty); #endif } #endif @@ -6252,7 +6269,7 @@ bool PDFWriterImpl::finalizeSignature() CHECK_RETURN( (osl::File::E_None == m_aFile.setPos(osl_Pos_Absolut, 0)) ); HCRYPTPROV hCryptProvider; - if (!CryptAcquireContext(&hCryptProvider, NULL, MS_ENHANCED_PROV, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT)) + if (!CryptAcquireContext(&hCryptProvider, NULL, MS_ENH_RSA_AES_PROV, PROV_RSA_AES, CRYPT_VERIFYCONTEXT)) { SAL_WARN("vcl.pdfwriter", "CryptAcquireContext failed: " << WindowsError(GetLastError())); CertFreeCertificateContext(pCertContext); @@ -6268,6 +6285,7 @@ bool PDFWriterImpl::finalizeSignature() return false; } +#if 0 DWORD nHashSize; DWORD nHashSizeLen(sizeof(DWORD)); if (!CryptGetHashParam(hHash, HP_HASHSIZE, reinterpret_cast<BYTE *>(&nHashSize), &nHashSizeLen, 0)) @@ -6281,6 +6299,7 @@ bool PDFWriterImpl::finalizeSignature() assert(nHashSizeLen == sizeof(DWORD)); assert(nHashSize == 20); +#endif boost::scoped_array<char> buffer(new char[m_nSignatureContentOffset + 1]); sal_uInt64 bytesRead; @@ -6337,8 +6356,26 @@ bool PDFWriterImpl::finalizeSignature() OString pass = OUStringToOString( m_aContext.SignPassword, RTL_TEXTENCODING_UTF8 ); + HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey; + DWORD nKeySpecType; + BOOL bMustFree; + if (!CryptAcquireCertificatePrivateKey(pCertContext, 0, NULL, &hCryptProvOrNCryptKey, &nKeySpecType, &bMustFree)) + { + SAL_WARN("vcl.pdfwriter", "CryptAcquireCertificatePrivateKey failed: " << WindowsError(GetLastError())); + CryptDestroyHash(hHash); + CryptReleaseContext(hCryptProvider, 0); + CertFreeCertificateContext(pCertContext); + return false; + } + + SAL_INFO("vcl.pdfwriter", "Certificate has private key that is " << + (nKeySpecType == AT_KEYEXCHANGE ? OUString("for key exchange") : + (nKeySpecType == AT_SIGNATURE ? OUString("for signatures") : + (nKeySpecType == CERT_NCRYPT_KEY_SPEC ? OUString("a CNG key, whatever that is") : + OUString("unknown type (") + OUString::number(nKeySpecType, 16) + ")")))); + DWORD nSigLen(0); - if (!CryptSignHash(hHash, AT_SIGNATURE, NULL, 0, NULL, &nSigLen)) + if (!CryptSignHash(hHash, nKeySpecType, NULL, CRYPT_NOHASHOID, NULL, &nSigLen)) { SAL_WARN("vcl.pdfwriter", "CryptSignHash failed: " << WindowsError(GetLastError())); CryptDestroyHash(hHash); @@ -6348,7 +6385,7 @@ bool PDFWriterImpl::finalizeSignature() } boost::scoped_array<BYTE> pSig(new BYTE[nSigLen]); - if (!CryptSignHash(hHash, AT_SIGNATURE, NULL, 0, pSig.get(), &nSigLen)) + if (!CryptSignHash(hHash, nKeySpecType, NULL, CRYPT_NOHASHOID, pSig.get(), &nSigLen)) { SAL_WARN("vcl.pdfwriter", "CryptSignHash failed: " << WindowsError(GetLastError())); CryptDestroyHash(hHash);
Ah, the AT_KEYEXCHANGE / AT_SIGNATURE thing is a red herring, see http://microsoft.public.platformsdk.security.narkive.com/dsu6jLuu/at-signature-and-at-keyexchange for instance, "AT_SIGNATURE keys can ONLY be used to sign; AT_KEYEXCHANGE keys can be use both to sign and decrypt"
For what it's worth, I distilled the code for signing down to a minimal freestanding test program, and get the same error, "CryptSignHash failed: Keyset does not exist". (Build with "cl -MD sign.c" in a "VS2013 x64 Native Tools Command Prompt".) #include <cassert> #include <iostream> #include <windows.h> #include <wincrypt.h> #define SAL_WARN(tag,stream) std::cerr << stream << std::endl #define SAL_INFO(tag,stream) std::cerr << stream << std::endl #define OUString(s) s static char *WindowsError(DWORD nErrorCode) { LPSTR pMsgBuf; if (FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, NULL, nErrorCode, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (LPSTR)&pMsgBuf, 0, NULL) == 0) return "unknown"; if (pMsgBuf[strlen(pMsgBuf)-1] == '\n') pMsgBuf[strlen(pMsgBuf)-1] = '\0'; return pMsgBuf; } static bool foo () { HCERTSTORE hCertStore = CertOpenSystemStore(NULL, "MY"); if (!hCertStore) { SAL_WARN("vcl.pdfwriter", "CertOpenSystemStore failed: " << WindowsError(GetLastError())); return false; } PCCERT_CONTEXT pCertContext = CertFindCertificateInStore(hCertStore, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, 0, CERT_FIND_SUBJECT_STR, (const void*) L"collabora-dev-softtoken@wilhelmtux.ch" , NULL); if (pCertContext == NULL) { SAL_WARN("vcl.pdfwriter", "CertFindCertificateInStore failed: " << WindowsError(GetLastError())); return false; } DWORD nProperty(0); bool first(true); while ((nProperty = CertEnumCertificateContextProperties(pCertContext, nProperty)) != 0) { if (first) SAL_INFO("vcl.pdfwriter", "Certificate context properties:"); first = false; #if 0 DWORD nSize(0); if (!CertGetCertificateContextProperty(pCertContext, nProperty, NULL, &nSize)) SAL_INFO("vcl.pdfwriter", " " << "(missing?) " << std::hex << nProperty); else { boost::scoped_array<char> aData(new char[nSize]); if (!CertGetCertificateContextProperty(pCertContext, nProperty, aData.get(), &nSize)) SAL_INFO("vcl.pdfwriter", " " << "(missing?) " << std::hex:: << nProperty); else SAL_INFO("vcl.pdfwriter", " " << CertificatePropertyNameAndData(nProperty, aData, nSize)); } #else SAL_INFO("vcl.pdfwriter", " " << nProperty); #endif } HCRYPTPROV hCryptProvider; if (!CryptAcquireContext(&hCryptProvider, NULL, MS_ENH_RSA_AES_PROV, PROV_RSA_AES, CRYPT_VERIFYCONTEXT)) { SAL_WARN("vcl.pdfwriter", "CryptAcquireContext failed: " << WindowsError(GetLastError())); CertFreeCertificateContext(pCertContext); return false; } HCRYPTHASH hHash; if (!CryptCreateHash(hCryptProvider, CALG_SHA1, 0, 0, &hHash)) { SAL_WARN("vcl.pdfwriter", "CryptCreateHash failed: " << WindowsError(GetLastError())); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return false; } DWORD nHashSize; DWORD nHashSizeLen(sizeof(DWORD)); if (!CryptGetHashParam(hHash, HP_HASHSIZE, reinterpret_cast<BYTE *>(&nHashSize), &nHashSizeLen, 0)) { SAL_WARN("vcl.pdfwriter", "CryptGetHashParam failed: " << WindowsError(GetLastError())); CryptDestroyHash(hHash); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return false; } assert(nHashSizeLen == sizeof(DWORD)); assert(nHashSize == 20); //FIXME: Check if SHA1 is calculated from the correct byterange if (!CryptHashData(hHash, reinterpret_cast<const BYTE *>(foo), 2000, 0)) { SAL_WARN("vcl.pdfwriter", "CryptHashData failed: " << WindowsError(GetLastError())); CryptDestroyHash(hHash); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return false; } #if 0 // We don't actualy need the hash bytes unsigned char aHash[20]; if (!CryptGetHashParam(hHash, HP_HASHVAL, aHash, &nHashSize, 0)) { SAL_WARN("vcl.pdfwriter", "CryptGetHashParam failed: " << WindowsError(GetLastError())); CryptDestroyHash(hHash); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return false; } if (nHashSize != 20) { SAL_WARN("vcl.pdfwriter", "CryptGetHashParam returned unexpected size hash value: " << nHashSize); CryptDestroyHash(hHash); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return false; } #endif HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey; DWORD nKeySpecType; BOOL bMustFree; if (!CryptAcquireCertificatePrivateKey(pCertContext, 0, NULL, &hCryptProvOrNCryptKey, &nKeySpecType, &bMustFree)) { SAL_WARN("vcl.pdfwriter", "CryptAcquireCertificatePrivateKey failed: " << WindowsError(GetLastError())); CryptDestroyHash(hHash); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return false; } SAL_INFO("vcl.pdfwriter", "Certificate has private key that is " << (nKeySpecType == AT_KEYEXCHANGE ? OUString("for key exchange") : (nKeySpecType == AT_SIGNATURE ? OUString("for signatures") : (nKeySpecType == CERT_NCRYPT_KEY_SPEC ? OUString("a CNG key, whatever that is") : OUString("unknown type"))))); DWORD nSigLen(0); if (!CryptSignHash(hHash, nKeySpecType, NULL, CRYPT_NOHASHOID, NULL, &nSigLen)) { SAL_WARN("vcl.pdfwriter", "CryptSignHash failed: " << WindowsError(GetLastError())); CryptDestroyHash(hHash); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return false; } auto pSig(new BYTE[nSigLen]); if (!CryptSignHash(hHash, nKeySpecType, NULL, CRYPT_NOHASHOID, pSig, &nSigLen)) { SAL_WARN("vcl.pdfwriter", "CryptSignHash failed: " << WindowsError(GetLastError())); CryptDestroyHash(hHash); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return false; } // Release resources CryptDestroyHash(hHash); CryptReleaseContext(hCryptProvider, 0); CertFreeCertificateContext(pCertContext); return true; } int main (int argc, char **argv) { return foo() ? 0 : 1; }
There must be some pretty trivial detail I am missing... sigh. If only somebody who has experience of the Windows certificate, hashing and crypt API could have a look.
OK, I figured it out. Just had to stare even longer and harder at the sample code and finally it hit me how it differed from my code. Will commit fix later tonight or tomorrow.
Tor Lillqvist committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=070c93af73df9aa4eb333265c81060d123b530b9 fdo#87030: Prevent PDF signing using Windows API from failing It will be available in 4.5.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Tor Lillqvist committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=6e91763769a562b88882a4c2a94b1367c6ed4866 fdo#87030: Generate a proper PKCS#7 signature It will be available in 4.5.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Tor Lillqvist committed a patch related to this issue. It has been pushed to "libreoffice-4-4": http://cgit.freedesktop.org/libreoffice/core/commit/?id=be417961eecf6b096bd64fd55dccd4214c48836f&h=libreoffice-4-4 fdo#87030: Make PDF signing using Windows API work It will be available in 4.4.0.0.beta3. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Fix confirmed on Windows 7 with: Version: 4.5.0.0.alpha0+ Build ID: 170616e9f2d30c1302bbb5a7a4b588bc05cd5cc9 TinderBox: Win-x86@39, Branch:master, Time: 2014-12-12_01:58:46 Thanks guys, great work!