Bug 99327 - Wrong hash algorithm informed on signed PDF export
Summary: Wrong hash algorithm informed on signed PDF export
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Printing and PDF export (show other bugs)
Version:
(earliest affected)
5.0.5.2 release
Hardware: x86-64 (AMD64) Windows (All)
: medium normal
Assignee: Miklos Vajna
QA Contact:
URL:
Whiteboard: target:5.3.0
Keywords: filter:pdf
Depends on:
Blocks: PDF-Export
  Show dependency treegraph
 
Reported: 2016-04-15 14:31 UTC by Luis C. Serpa
Modified: 2016-12-03 08:47 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Certificate used (2.05 KB, application/octet-stream)
2016-04-15 14:31 UTC, Luis C. Serpa
Details
java error messages displayed on our internal document system (1.48 KB, text/plain)
2016-06-21 15:56 UTC, Luis C. Serpa
Details
A snapshot of Acrobat Reader warnings (107.40 KB, image/jpeg)
2016-06-21 15:57 UTC, Luis C. Serpa
Details
The signed PDF/A file. (69.83 KB, application/pdf)
2016-06-21 15:58 UTC, Luis C. Serpa
Details
The same PDF/A signed by our internal java signer. (89.13 KB, application/pdf)
2016-06-21 16:00 UTC, Luis C. Serpa
Details
Certificate (1.83 KB, application/octet-stream)
2016-06-21 16:03 UTC, Luis C. Serpa
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Luis C. Serpa 2016-04-15 14:31:44 UTC
Created attachment 124365 [details]
Certificate used

When I export as signed PDF/A with my personal certificate, the algorithm name save is SHA1WITHRSA. I suppose it should be only SHA1. One of the visible consequences is that, when opened in Adobe Reader, it shows an alert of altered pages. And worst, my signed documents are not being accepted on some systems
Comment 1 Tor Lillqvist 2016-06-21 13:06:28 UTC
Where do you see the SBA1WITHRSA? And do you have some comparable PDF signed by some other application where you instead see SHA1?
Comment 2 Luis C. Serpa 2016-06-21 15:56:39 UTC
Created attachment 125803 [details]
java error messages displayed on our internal document system

The java error messages displayed on our internal document system when I upload any LO exported PDF/A file with my signature.
Comment 3 Luis C. Serpa 2016-06-21 15:57:45 UTC
Created attachment 125804 [details]
A snapshot of Acrobat Reader warnings

A snapshot of Acrobat Reader DC showing a page modification warning on the same PDF/A signed (and untouched) file.
Comment 4 Luis C. Serpa 2016-06-21 15:58:56 UTC
Created attachment 125805 [details]
The signed PDF/A file.

The signed PDF/A file exported by LO that causes the problem.
Comment 5 Luis C. Serpa 2016-06-21 16:00:09 UTC
Created attachment 125806 [details]
The same PDF/A signed by our internal java signer.

The same PDF/A file, exported unsigned and signed by our internal java signer.
Comment 6 Luis C. Serpa 2016-06-21 16:03:22 UTC
Created attachment 125809 [details]
Certificate

The certification I used. This is a Brazilian official CA (www.serpro.gov.br)   issued certificate, used on many other applications w/o a problem.
Comment 7 Luis C. Serpa 2016-06-21 16:04:37 UTC
I attached some adsitional files; hope it will help to identify the problem.
Comment 8 Tor Lillqvist 2016-06-21 16:12:13 UTC
Thanks. No promises when I (or somebody) will have time to take a look, but the new information seems useful. If that is your real certificate, are you sure you should make that publicly available?
Comment 9 Luis C. Serpa 2016-06-21 18:24:38 UTC
Ok, thanx in advance. 
No problem it's only the pub key, to show that the algorithm field looks ok.
Comment 10 Daniel Miranda 2016-08-03 21:02:37 UTC
Hi, I am seeing the same problem here and the culprit seems to be this line in LibreOffice's source code:

https://cgit.freedesktop.org/libreoffice/core/tree/vcl/source/gdi/pdfwriter_impl.cxx#n7188


The following line:
 aSignerInfo.HashAlgorithm.pszObjId = const_cast<LPSTR>(szOID_RSA_SHA1RSA);
Should probably read:
 aSignerInfo.HashAlgorithm.pszObjId = const_cast<LPSTR>(szOID_OIWSEC_sha1);


I have traced that path from the pdfexport.cxx code, which creates an aContext structure and creates a pPDFWriter object at:

https://cgit.freedesktop.org/libreoffice/core/tree/filter/source/pdf/pdfexport.cxx#n787

The implementation of the code that does the actual signing is in pdfwriter_impl.cxx and creates the aSignerInfo structure of type CMSG_SIGNER_ENCODE_INFO, documented in

https://msdn.microsoft.com/pt-br/library/aa925156.aspx

The specific field that is set is aSignerInfo.HashAlgorithm.pszObjId, which is documented at:

https://msdn.microsoft.com/pt-br/library/office/aa381133.aspx.

The value currently in the code is:
 szOID_RSA_SHA1RSA
 "1.2.840.113549.1.1.5"
 which is NOT a digest algorithm, but an encryption and signing algorithn.

The likely value for that should be:
 szOID_OIWSEC_sha1
 "1.3.14.3.2.26"

Or, while we are at that, upgrade the algorithm to a more modern one not relying on sha1. It should have minor performance impact:
 szOID_NIST_sha256
 "2.16.840.1.101.3.4.2.1"
 or
 szOID_NIST_sha512
 "2.16.840.1.101.3.4.2.3" 


Other parts of the file should also be updated:
 https://cgit.freedesktop.org/libreoffice/core/tree/vcl/source/gdi/pdfwriter_impl.cxx#n7159
 (although it seems the aPara structure is not actually being used anywhere else)

CAVEAT:
  1. I am not familiar with LibreOffice's codebase, please check if these two lines are really all it takes.
  2. The digest algorithm upgrade seems to be a bit more complex than a fix for this bug, it requires changes in other parts of the file. Nonetheless, I think this is a wonderful time to do it, since sha1 is showing it's age. Bruce Scheiner blogged in 2005 - 11 years ago - that it was no longer safe even then. See https://www.schneier.com/blog/archives/2005/02/cryptanalysis_o.html
Comment 11 Daniel Miranda 2016-08-03 21:06:41 UTC
By the way, it does not seem to be an issue specific do PDF/A, but to all signed PDF exports.
Comment 12 Miklos Vajna 2016-11-18 10:32:23 UTC
I've changed the pdf export code to use SHA256 on Linux/macOS (NSS case) in commit 3ab31ae5db001021069f25257454b7dee78e6dba. I plan to do the same on Windows soon.
Comment 13 Daniel Miranda 2016-11-18 20:31:42 UTC
  Thanks for looking into this Miklos! I have taken some time to read the relevant bits in the source but I am far from being able to submit a proper pull request.

  I have got some feedback from my IT department and I would like to ask that the algorithm is cranked up to SHA512, if you are willing. Since there is no option presented to the user and the performance penalty should not be noticeable, they asked that we default to the highest practical security alternative.

  I have put together a diff of what I perceive to be the necessary changes in the next post, including the fix for the wrong szOID_RSA_SHA1RSA id.
Comment 14 Daniel Miranda 2016-11-18 20:32:35 UTC
--- a/vcl/source/gdi/pdfwriter_impl.cxx
+++ b/vcl/source/gdi/pdfwriter_impl.cxx
@@ -6278,7 +6278,7 @@
  * Hash ::= OCTET STRING
  *
  * ESSCertIDv2 ::= SEQUENCE {
- *     hashAlgorithm AlgorithmIdentifier DEFAULT {algorithm id-sha256},
+ *     hashAlgorithm AlgorithmIdentifier DEFAULT {algorithm id-sha512},
  *     certHash Hash,
  *     issuerSerial IssuerSerial OPTIONAL
  * }
@@ -6662,7 +6662,7 @@
         return nullptr;
     }
 
-    *cms_signer = NSS_CMSSignerInfo_Create(result, cert, SEC_OID_SHA256);
+    *cms_signer = NSS_CMSSignerInfo_Create(result, cert, SEC_OID_SHA512);
     if (!*cms_signer)
     {
         SAL_WARN("vcl.pdfwriter", "NSS_CMSSignerInfo_Create failed");
@@ -6703,7 +6703,7 @@
         return nullptr;
     }
 
-    if (NSS_CMSSignedData_SetDigestValue(*cms_sd, SEC_OID_SHA256, digest) != SECSuccess)
+    if (NSS_CMSSignedData_SetDigestValue(*cms_sd, SEC_OID_SHA512, digest) != SECSuccess)
     {
         SAL_WARN("vcl.pdfwriter", "NSS_CMSSignedData_SetDigestValue failed");
         NSS_CMSSignedData_Destroy(*cms_sd);
@@ -6748,7 +6748,7 @@
         return false;
     }
 
-    HashContextScope hc(HASH_Create(HASH_AlgSHA256));
+    HashContextScope hc(HASH_Create(HASH_AlgSHA512));
     if (!hc.get())
     {
         SAL_WARN("vcl.pdfwriter", "HASH_Create failed");
@@ -6762,15 +6762,15 @@
     HASH_Update(hc.get(), static_cast<const unsigned char*>(rContext.m_pByteRange2), rContext.m_nByteRange2);
 
     SECItem digest;
-    unsigned char hash[SHA256_LENGTH];
+    unsigned char hash[SHA512_LENGTH];
     digest.data = hash;
-    HASH_End(hc.get(), digest.data, &digest.len, SHA256_LENGTH);
+    HASH_End(hc.get(), digest.data, &digest.len, SHA512_LENGTH);
     hc.clear();
 
 #ifdef DBG_UTIL
     {
         FILE *out = fopen("PDFWRITER.hash.data", "wb");
-        fwrite(hash, SHA256_LENGTH, 1, out);
+        fwrite(hash, SHA512_LENGTH, 1, out);
         fclose(out);
     }
 #endif
@@ -6836,7 +6836,7 @@
         }
 #endif
 
-        HashContextScope ts_hc(HASH_Create(HASH_AlgSHA256));
+        HashContextScope ts_hc(HASH_Create(HASH_AlgSHA512));
         if (!ts_hc.get())
         {
             SAL_WARN("vcl.pdfwriter", "HASH_Create failed");
@@ -6847,16 +6847,16 @@
         HASH_Begin(ts_hc.get());
         HASH_Update(ts_hc.get(), ts_cms_signer->encDigest.data, ts_cms_signer->encDigest.len);
         SECItem ts_digest;
-        unsigned char ts_hash[SHA256_LENGTH];
+        unsigned char ts_hash[SHA512_LENGTH];
         ts_digest.type = siBuffer;
         ts_digest.data = ts_hash;
-        HASH_End(ts_hc.get(), ts_digest.data, &ts_digest.len, SHA256_LENGTH);
+        HASH_End(ts_hc.get(), ts_digest.data, &ts_digest.len, SHA512_LENGTH);
         ts_hc.clear();
 
 #ifdef DBG_UTIL
         {
             FILE *out = fopen("PDFWRITER.ts_hash.data", "wb");
-            fwrite(ts_hash, SHA256_LENGTH, 1, out);
+            fwrite(ts_hash, SHA512_LENGTH, 1, out);
             fclose(out);
         }
 #endif
@@ -6868,7 +6868,7 @@
 
         src.messageImprint.hashAlgorithm.algorithm.data = nullptr;
         src.messageImprint.hashAlgorithm.parameters.data = nullptr;
-        SECOID_SetAlgorithmID(nullptr, &src.messageImprint.hashAlgorithm, SEC_OID_SHA256, nullptr);
+        SECOID_SetAlgorithmID(nullptr, &src.messageImprint.hashAlgorithm, SEC_OID_SHA512, nullptr);
         src.messageImprint.hashedMessage = ts_digest;
 
         src.reqPolicy.type = siBuffer;
@@ -7097,11 +7097,11 @@
     // Write ESSCertIDv2.hashAlgorithm.
     aCertID.hashAlgorithm.algorithm.data = nullptr;
     aCertID.hashAlgorithm.parameters.data = nullptr;
-    SECOID_SetAlgorithmID(nullptr, &aCertID.hashAlgorithm, SEC_OID_SHA256, nullptr);
+    SECOID_SetAlgorithmID(nullptr, &aCertID.hashAlgorithm, SEC_OID_SHA512, nullptr);
     // Write ESSCertIDv2.certHash.
     SECItem aCertHashItem;
-    unsigned char aCertHash[SHA256_LENGTH];
-    HashContextScope aCertHashContext(HASH_Create(HASH_AlgSHA256));
+    unsigned char aCertHash[SHA512_LENGTH];
+    HashContextScope aCertHashContext(HASH_Create(HASH_AlgSHA512));
     if (!aCertHashContext.get())
     {
         SAL_WARN("vcl.pdfwriter", "HASH_Create() failed");
@@ -7111,7 +7111,7 @@
     HASH_Update(aCertHashContext.get(), reinterpret_cast<const unsigned char *>(rContext.m_pDerEncoded), rContext.m_nDerEncoded);
     aCertHashItem.type = siBuffer;
     aCertHashItem.data = aCertHash;
-    HASH_End(aCertHashContext.get(), aCertHashItem.data, &aCertHashItem.len, SHA256_LENGTH);
+    HASH_End(aCertHashContext.get(), aCertHashItem.data, &aCertHashItem.len, SHA512_LENGTH);
     aCertID.certHash = aCertHashItem;
     // Write SigningCertificateV2.certs.
     aCertIDs[0] = &aCertID;
@@ -7228,7 +7228,7 @@
     aPara.cbSize = sizeof(aPara);
     aPara.dwMsgEncodingType = PKCS_7_ASN_ENCODING | X509_ASN_ENCODING;
     aPara.pSigningCert = pCertContext;
-    aPara.HashAlgorithm.pszObjId = const_cast<LPSTR>(szOID_RSA_SHA1RSA);
+    aPara.HashAlgorithm.pszObjId = const_cast<LPSTR>(szOID_NIST_sha512);
     aPara.HashAlgorithm.Parameters.cbData = 0;
     aPara.cMsgCert = 1;
     aPara.rgpMsgCert = &pCertContext;
@@ -7257,7 +7257,7 @@
     aSignerInfo.pCertInfo = pCertContext->pCertInfo;
     aSignerInfo.hCryptProv = hCryptProv;
     aSignerInfo.dwKeySpec = nKeySpec;
-    aSignerInfo.HashAlgorithm.pszObjId = const_cast<LPSTR>(szOID_RSA_SHA1RSA);
+    aSignerInfo.HashAlgorithm.pszObjId = const_cast<LPSTR>(szOID_NIST_sha512);
     aSignerInfo.HashAlgorithm.Parameters.cbData = 0;
 
     CMSG_SIGNED_ENCODE_INFO aSignedInfo;
@@ -7392,7 +7392,7 @@
         if (!(*crts)(rContext.m_aSignTSA.getStr(),
                      0,
                      10000,
-                     szOID_NIST_sha256,
+                     szOID_NIST_sha512,
                      &aTsPara,
                      pDecodedSignerInfo->EncryptedHash.pbData,
                      pDecodedSignerInfo->EncryptedHash.cbData,
Comment 15 Miklos Vajna 2016-11-22 13:40:12 UTC
Oh, sorry, I set the state of this bug to "assigned" to signal that I plan to fix the problem, i.e. you should not spend your time on it to avoid duplicated work. :-)

How about:

1) I'll fix the algo & upgrade to SHA-256, so that LO on all platforms uses the same hashing (current NSS is SHA-256, mscrypto is SHA-1, which is inconsistent).

2) The sha256 -> sha512 part of your above patch would be still useful, but we take patches by you pushing to gerrit (more info at <https://wiki.documentfoundation.org/Development/gerrit>), not here pasted as bugzilla comments.

Given that the LO 5.3 feature freeze is ~today, I'll make sure that 1) is in for 5.3. I'm a bit worried doing one more algo change right before a release, so if you don't mind, I would recommend doing that in master only, i.e. have that enabled in LO 5.4 only.

I'll update this bug when I'm done with 1) (planned later this week), then you're more than welcome to push your SHA-512 patch to gerrit. ;-)
Comment 16 Commit Notification 2016-11-22 19:28:39 UTC
Miklos Vajna committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=4234b099f1ced2c1929a98ef6133b5280697f00a

tdf#99327 vcl PDF mscrypto sign: fix SHA-256 OID

It will be available in 5.3.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.
Comment 17 Miklos Vajna 2016-11-22 19:36:38 UTC
It's in -- please submit your sha256 -> sha512 changes to gerrit on top of this. Thanks! :-)
Comment 18 Daniel Miranda 2016-11-23 08:44:56 UTC
Hi, Miklos!
Thank you very much for your help in sorting out this bug!

I re-read my previous posts and I apologize for omitting some vital information. I failed to mention that the proposed diffs were not tested, or even verified to compile. I am unable to commit the time needed to set a build environment and do the basic tests - the codebase is too alien for me at this time and I am nearing deadlines for two other projects.

My intention was to clearly state what I thought the necessary changes were and to somehow help in the process. If you could review the changes in your build environment and check that they work, I will submit a patch as you indicated.

Thanks again!
--Daniel
Comment 19 Daniel Miranda 2016-11-23 08:49:31 UTC
Sorry for double posting - just clarifying that my patch addresses not only the algorithm change, but also the actual bug, which is the incorrect use of szOID_RSA_SHA1RSA.
Comment 20 Miklos Vajna 2016-11-29 14:58:54 UTC
No problem, but why did you reopen this bug? :-)

The above fixed problem is exactly the title of this bug, that szOID_RSA_SHAXXXRSA was used as the algo ID instead of szOID_NIST_shaXXX.

I plan to mark this resolved again in the near future, unless I hear I missed something.
Comment 21 Daniel Miranda 2016-12-03 08:47:18 UTC
Awesome Niklos! Thank you for your patience. I apologize for the noise, you are correct in that your patch solved the bug.

Best,
Daniel