Bug 16770 - support for digital signature
Summary: support for digital signature
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other Linux (All)
: medium enhancement
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
: 19120 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-07-18 00:43 UTC by rastos
Modified: 2017-01-18 05:33 UTC (History)
25 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Initial patch for parsing digitally signed PDFs (5.28 KB, patch)
2010-07-28 11:07 UTC, Markus Kilås
Details | Splinter Review
PDF Signature verification support (29.52 KB, patch)
2012-09-07 12:11 UTC, Andre Guerreiro
Details | Splinter Review
PDF signature verification using NSS (39.40 KB, patch)
2015-08-23 23:43 UTC, Andre Guerreiro
Details | Splinter Review
memory leaks fixed (43.76 KB, patch)
2015-08-31 22:30 UTC, André Esser
Details | Splinter Review
poppler nss signature support - v3 (45.20 KB, text/plain)
2015-09-01 18:43 UTC, Andre Guerreiro
Details
poppler nss signature support - v4 (45.06 KB, patch)
2015-09-02 10:59 UTC, André Esser
Details | Splinter Review
poppler nss signature support - v5 - refactor (31.61 KB, patch)
2015-09-10 15:49 UTC, Andre Guerreiro
Details | Splinter Review
poppler nss signature support - v6 (31.64 KB, patch)
2015-09-10 16:25 UTC, Andre Guerreiro
Details | Splinter Review
Fix for Buffer overflow (873 bytes, patch)
2015-09-22 14:04 UTC, Andre Guerreiro
Details | Splinter Review
NSS conditional build (10.03 KB, patch)
2015-09-25 16:28 UTC, Andre Guerreiro
Details | Splinter Review
Support for adbe.pkcs7.sha1 signatures (2.09 KB, patch)
2015-09-25 17:36 UTC, Andre Guerreiro
Details | Splinter Review
Incremental hashing + large file support (7.80 KB, patch)
2015-10-08 00:40 UTC, Andre Guerreiro
Details | Splinter Review
Handle SEC_ERROR_UNTRUSTED_ISSUER (450 bytes, text/plain)
2015-10-24 14:32 UTC, Markus Kilås
Details
Manpage improvement (948 bytes, patch)
2015-10-29 11:12 UTC, Andre Guerreiro
Details | Splinter Review
Improve robustness of SignatureHandler::validateCertificate (1.91 KB, patch)
2015-12-09 16:04 UTC, Andre Guerreiro
Details | Splinter Review
Fix printf for unsigned int (628 bytes, patch)
2016-01-01 19:23 UTC, Markus Kilås
Details | Splinter Review
Considering offset 0 and signature only covering part of PDF (3.38 KB, patch)
2016-01-01 22:54 UTC, Markus Kilås
Details | Splinter Review
Check in ranges in ByteRange array (4.02 KB, patch)
2016-01-08 06:28 UTC, Adrian Johnson
Details | Splinter Review
Check in ranges in ByteRange array v2 (5.17 KB, patch)
2016-01-10 03:46 UTC, Adrian Johnson
Details | Splinter Review
Load NSS root certs module (564 bytes, patch)
2016-01-12 19:06 UTC, Andre Guerreiro
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description rastos 2008-07-18 00:43:49 UTC
Since poppler is the basis for most pdf-processing software on Linux it would be great if it provided some functionality to access digital signatures embedded in PDF documents, so that the applications can display details of signing certificate and verify the validity of signature.

An example of such signatures can be seen on
   http://www.aloaha.com/cache/multiplesignatures.pdf
Look at the objects along the right border of the page. On Windows the signatures can be checked using Adobe Acrobat 8.x
Comment 1 Carlos Garcia Campos 2008-12-17 00:44:47 UTC
*** Bug 19120 has been marked as a duplicate of this bug. ***
Comment 2 Andrew Daviel 2008-12-17 15:22:30 UTC
http://www.adobe.com/devnet/acrobat/pdfs/PDF32000_2008.pdf
I hacked Xpdf to tell me of the existence of SigFlags bits, but lack the skill to implement this properly in finite time
Comment 3 Markus Kilås 2010-07-28 11:07:28 UTC
Created attachment 37425 [details] [review]
Initial patch for parsing digitally signed PDFs

I have started to look at support for verifying signed PDF documents. 

The attached patched gives very basic support by providing methods for getting the signature data (/Contents), the signature type (/SubFilter i.e. PKCS7) and the ByteRanges that the verifier needs to calculate the digest over. Then the actual signature and certificate chain verification is not specific to PDF and could be implemented by the applications using any crypto library.

// Markus Kilås
markus@primekey.se
Comment 4 Markus Kilås 2010-07-28 11:25:18 UTC
Sample signed document:
http://wwwpriv.primekey.se/~markus/pdfsigner/SignServer3.1.3-signed.pdf
Comment 5 Albert Astals Cid 2010-07-28 15:01:38 UTC
Why force the applications to implement it? After all they are all going to do the same, so it makes sense to have it at the poppler level too.

Code related i don't see why you store contents as a GooString and the others as Objects, what's the reason?

Also before doing getArray() and getName() you need to check with isArray and isName, otherwise if the file is broken we will crash.
Comment 6 Brad Hards 2010-07-28 15:55:34 UTC
I do kind-of agree with Markus that the verification operations can be done externally. There is an application level dependency in that the certificate store could depend on the desktop / user environment.
Comment 7 Brad Hards 2010-07-28 16:05:21 UTC
It would be very useful to have example code that actually does the validation operations (e.g. in the glib or qt examples). Perhaps gnutls (LGPLv2+) may be suitable.
Comment 8 Markus Kilås 2010-07-29 02:48:37 UTC
I can see you point that the verification should be included if all applications were to use it. 

However, I was not just sure if it is good to add a dependency to a particular crypto library. There are Gnutls, openssl and NSS and possibly other? I have not used any of them for this purpose (I am mainly a Java developer now days and normally use the Bouncy Castle API). And as Brad mentions the trusted root certificates might be fetched from some keystore integrated with the desktop.

I think my initial idea was to have support in poppler to get only that is needed and then an application could implement the rest and later some of that could be refactored and moved back into poppler, but that's just and idea you know better how poppler works.

Regarding the code related question: I have not been using poppler before and I noticed while looking at this that there was at least two ways of doing it. I did not really understand the implications of choosing one over the other. What is recommended - storing the Object in the class or copy the string? I wasn't also sure I was freeing the memory correctly...
Comment 9 Albert Astals Cid 2010-07-29 12:09:38 UTC
Brad, you are the "expert" here, you think it makes sense commiting the patch (maybe fixing the style?) ?
Comment 10 Albert Astals Cid 2010-07-29 15:40:14 UTC
I've spoken with Brad off-line and i think i agree with him that we should have a working test/example showing how things work before commiting anything, otherwise it will just bitrot, no-one will be able to figure out how to use it, etc...

So if you could hack a quick test/example using the library of your choose it would be great
Comment 11 Cristian Morales 2010-08-19 10:18:58 UTC
I'm far from understanding all of this. But notice it seems NSS will be *the* Linux crypto library.
Fedora is pursuing it: http://fedoraproject.org/wiki/FedoraCryptoConsolidation
And it's in Linux Standard Base 4: http://ldn.linuxfoundation.org/article/lsb-40-the-cryptography-strategy
Comment 12 Jelle de Jong 2011-04-15 01:15:17 UTC
Any progress or possible ETA for digital signature support for PDF documents?
Comment 13 Roland Lezuo 2011-12-11 11:44:40 UTC
bump
Comment 14 lsof 2012-02-09 05:36:13 UTC
Since there hasn't been any progress for a couple of years, would it be possible to close this bug?
Comment 15 Simon.Harhues 2012-03-04 00:49:19 UTC
In favor of closing it, I would like to see some progress here. :)

I believe the importance for digital signing will increase in future. Maybe the poppler team could ask the developers/companies of the big linux distributions for help?
Comment 16 Andre Guerreiro 2012-09-07 12:11:03 UTC
Created attachment 66786 [details] [review]
PDF Signature verification support

Here's an initial attempt at solving this issue.


This patch adds signature verification support to poppler core.
It uses OpenSSL PKCS7 API for the crypto operations (signature and certificate Validations).

4 new functions were added at the glib wrapper level: 
    poppler_document_is_signed
    poppler_document_signature_validate
    poppler_document_signature_get_time
    poppler_document_signature_get_signername

I've coordinated with Vasco Dias to expose this feature in Evince and his work is in the latest patches attached to this bug: https://bugzilla.gnome.org/show_bug.cgi?id=614929

As the additional dependency on OpenSSL couldn't possibly satisfy everyone I made it optional at build-time with --enable-openssl for Autotools and -DENABLE_OPENSSL=ON for cmake

Current limitations: 
- Timestamps contained in the PKCS7 signature are not verified
- the new functionality is not yet exposed in the qt4 wrapper as I prioritized the glib wrapper to support Evince.
Comment 17 Albert Astals Cid 2012-09-07 12:58:34 UTC
Haven't looked at the code yet, but thanks anyway for trying to solve this.

openssl is not compatible with GPLv2 code (or at least its compatibility is discussed by some) any change you guys can use GNU TLS or Mozilla NSS which seem to be more in the clear?
Comment 18 Josh Triplett 2012-09-07 14:54:16 UTC
Since the patch provided supports verifying signatures, any thoughts on adding support for creating signatures, ideally in a manner compatible with Adobe's implementation?
Comment 19 Andre Guerreiro 2012-09-07 16:50:48 UTC
@Albert
OK, I can see the problem for poppler in terms of licensing.

A quick evaluation of the alternatives:
gnutls seems to be unsuited for this because it doesn't have a decent PKCS7 API that would allow me to parse the signature and access each component.
I've only found this in the docs: http://www.gnu.org/software/gnutls/manual/html_node/X509-certificate-API.html#X509-certificate-API

NSS seems to be more promising as I've found example code for PKCS#7 validation in its source tarball: mozilla/security/nss/cmd/p7verify/p7verify.c

The disadvantage I see with nss is that we won't be able to reuse the system certificate store usually in /etc/ssl/certs because it will need to use a particular Berkeley DB cert store as you can find in your Firefox/Thunderbird Profile. So we'd have an implicit dependency on .mozilla/... being present or worse we'll need to introduce our own cert store.   

I have no experience with gnutls or nss so if anyone can correct me or add something, feel free.
Comment 20 Andre Guerreiro 2012-09-07 17:05:36 UTC
Also I can see merit in Fedora's effort of consolidation around NSS but I think it's a really herculean effort to port over so many packages.  

http://fedoraproject.org/wiki/CryptoConsolidationScorecard
Comment 21 Markus Kilås 2012-09-07 17:26:12 UTC
(In reply to comment #19)

I know that LibreOffice uses NSS as when I look at digital signatures my certificates from Firefox is availble. However, I don't think the LibreOffice Ubuntu packages require the whole Firefox to be installed.
Comment 22 Andre Guerreiro 2012-09-07 17:43:10 UTC
(In reply to comment #21)
> (In reply to comment #19)
> 
> I know that LibreOffice uses NSS as when I look at digital signatures my
> certificates from Firefox is availble. However, I don't think the LibreOffice
> Ubuntu packages require the whole Firefox to be installed.

Yes it doesn't require Firefox or Thunderbird but if you didn't have any of them you wouldn't have any CA certs in LO.

http://wiki.openoffice.org/wiki/Certificate_Detection
Comment 23 Albert Astals Cid 2012-09-08 16:46:26 UTC
Andre: what's missing regarding PCKS#7 in http://www.gnu.org/software/gnutls/manual/html_node/X509-certificate-API.html#X509-certificate-API ? (note i'm a total noob regarding PCKS :D)

From what  i see you can go gnutls_pkcs7_export + gnutls_x509_crl_import and then the x509 thing offers lots of getter, no?

If that does not work for you, what big projects do with openssl (i.e. Qt) is dlopen it on runtime, that seems to avoid the gpl incompatibiltiy which if you ask me is a bit lame, but if it works for them i guess it should work for us.
Comment 24 Albert Astals Cid 2012-09-09 10:02:10 UTC
What is missing in gnutls? I think that you have to use the pcks7 export and pass the data to the x509 import and then you have all those useful functions.

If none of the options is enough, we can do what big projects do (e.g. Qt) and dlopen openssl, if it works for them, it ought to work for us.
Comment 25 Albert Astals Cid 2012-09-09 10:19:59 UTC
Doh, there was a glitch in bugzilla and it created both my yesterday comment that i got an error when pasting and today's one :-S
Comment 26 Andre Guerreiro 2012-09-10 09:41:39 UTC
What's missing in gnutls is a way to parse all the relevant components of the PKCS#7 object as present in a PDF signature.

It seems that in gnutls they assume those objects can only contain certificates and CRLs as you can confirm if you go through the functions that take gnutls_pkcs7_t as argument.

With openssl you can get the certificates, signature, and the digest of the signed content (these are the essential parts for detached signatures as used in PDF) as well as any optional timestamps or CRLs.
Comment 27 Andre Guerreiro 2012-09-10 09:44:47 UTC
Regarding the dlopen workaround I'll take a look at it this week. 

I'll try to minimize the pitfall of possible missing/different symbols by targeting only the last major version of openssl (1.0).
Comment 28 Christian Persch (GNOME) 2012-09-10 11:38:59 UTC
qt can link to openssl because its lgpl licence has an openssl exception, and not because dlopening would make it not 'link' to openssl. poppler has no such openssl exception; and even if it did, that wouldn't help the programmes using poppler which are gpl (e.g. evince) without such an exception.
Comment 29 Andre Guerreiro 2012-09-10 14:32:47 UTC
So if I understood correctly Qt is only using the dlopen approach to overcome restrictions to crypto exports but not (L)GPL incompatibilities, like stated here: 
http://qt-project.org/doc/qt-4.8/ssl.html

Getting back to our point I'll need the definitive opinion from Poppler maintainers on using dlopen'ed openssl or replacing it with NSS.

Both options are extra work but I'm willing to do the extra mile to get this accepted.
Comment 30 Carlos Garcia Campos 2012-09-10 14:59:35 UTC
(In reply to comment #26)
> What's missing in gnutls is a way to parse all the relevant components of the
> PKCS#7 object as present in a PDF signature.
> 
> It seems that in gnutls they assume those objects can only contain certificates
> and CRLs as you can confirm if you go through the functions that take
> gnutls_pkcs7_t as argument.
> 
> With openssl you can get the certificates, signature, and the digest of the
> signed content (these are the essential parts for detached signatures as used
> in PDF) as well as any optional timestamps or CRLs.

Would it be a lot of work to add support for that to gnutls?
Comment 31 Albert Astals Cid 2012-09-11 13:19:17 UTC
Maintainer hat on: I think it'd be easier for all if you can use NSS (now that gnutls has been ruled out because of lack of functionality). https://wiki.mozilla.org/NSS_Shared_DB_And_LINUX states a way to use the shared nss certificate, and seems that chrome is using that too http://code.google.com/p/chromium/wiki/LinuxCertManagement so it is not that "bad".

What do you think?
Comment 32 Carlos Garcia Campos 2012-09-11 15:00:50 UTC
(In reply to comment #30)
> (In reply to comment #26)
> > What's missing in gnutls is a way to parse all the relevant components of the
> > PKCS#7 object as present in a PDF signature.
> > 
> > It seems that in gnutls they assume those objects can only contain certificates
> > and CRLs as you can confirm if you go through the functions that take
> > gnutls_pkcs7_t as argument.
> > 
> > With openssl you can get the certificates, signature, and the digest of the
> > signed content (these are the essential parts for detached signatures as used
> > in PDF) as well as any optional timestamps or CRLs.
> 
> Would it be a lot of work to add support for that to gnutls?

Replying to myself:

<KaL> I wonder if it could be useful for glib-networking to implement the missing things in gnutls, or if we don't need that at all
<danw> reading...
<danw> chpe, KaL_out: both gnutls and glib-networking intentionally only do TLS, not crypto in general, so I don't think it makes sense to add the extra PKCS#7 functionality to either of them
<danw> NSS would be better than OpenSSL, and once all the p11-kit / NSS-shared-DB stuff gets figured out fully, then NSS-based apps will be able to access your gnome-keyring certificates via PKCS#11
Comment 33 Andre Guerreiro 2012-09-11 23:14:20 UTC
OK, NSS with shared DB is what I'll pursue from now on.
Thanks everyone for the input.
Comment 34 weibo 2012-10-30 07:38:58 UTC
Comment on attachment 37425 [details] [review]
Initial patch for parsing digitally signed PDFs

>diff --git a/poppler/Form.cc b/poppler/Form.cc
>index 4df8a7d..1da9776 100644
>--- a/poppler/Form.cc
>+++ b/poppler/Form.cc
>@@ -1107,11 +1107,28 @@ void FormFieldChoice::_createChoicesTab ()
> FormFieldSignature::FormFieldSignature(XRef *xrefA, Object *dict, const Ref& ref)
> 	: FormField(xrefA, dict, ref, formSignature)
> {
>+    Object v, contents;
>+
>+    if (dict->dictLookup("V", &v)->isDict()) {
>+        if (v.dictLookup("Contents", &contents)->isString()) {
>+            this->contents = contents.getString()->copy();
>+            v.dictLookup("ByteRange", &byteRange);
>+            v.dictLookup("Filter", &filter);
>+            v.dictLookup("SubFilter", &subFilter);
>+        }
>+        contents.free();
>+    }
>+    v.free();
> }
> 
> FormFieldSignature::~FormFieldSignature()
> {
>-
>+    if(contents) {
>+        delete contents;
>+}
>+    byteRange.free();
>+    filter.free();
>+    subFilter.free();
> }
> 
> //------------------------------------------------------------------------
>diff --git a/poppler/Form.h b/poppler/Form.h
>index 35d66af..e5d1a1f 100644
>--- a/poppler/Form.h
>+++ b/poppler/Form.h
>@@ -418,6 +418,18 @@ public:
>   FormFieldSignature(XRef *xrefA, Object *dict, const Ref& ref);
> 
>   virtual ~FormFieldSignature();
>+
>+  Array* GetByteRange() { return byteRange.getArray(); }
>+  GooString* GetContents() const { return contents; }
>+  char* GetFilter() { return filter.getName(); }
>+  char* GetSubFilter() { return subFilter.getName();}
>+
>+protected:
>+  GooString* contents;
>+  Object byteRange;
>+  Object filter;
>+  Object subFilter;
>+
> };
> 
> //------------------------------------------------------------------------
>diff --git a/poppler/PDFDoc.cc b/poppler/PDFDoc.cc
>index ade5fe4..80b5f6c 100644
>--- a/poppler/PDFDoc.cc
>+++ b/poppler/PDFDoc.cc
>@@ -358,6 +358,25 @@ GBool PDFDoc::checkEncryption(GooString *ownerPassword, GooString *userPassword)
>   return ret;
> }
> 
>+#define SIGFLAGS_SIGNATURES_EXIST   0x1
>+#define SIGFLAGS_APPEND_ONLY        0x2
>+
>+GBool PDFDoc::isSigned() {
>+    GBool ret;
>+    Object sigFlags;
>+
>+    getCatalog()->getAcroForm()->dictLookup("SigFlags", &sigFlags);
>+
>+    if (sigFlags.isInt()) {
>+        ret = sigFlags.getInt() & SIGFLAGS_SIGNATURES_EXIST;
>+    } else {
>+        ret = gFalse;
>+    }
>+    sigFlags.free();
>+
>+    return ret;
>+}
>+
> void PDFDoc::displayPage(OutputDev *out, int page,
> 			 double hDPI, double vDPI, int rotate,
> 			 GBool useMediaBox, GBool crop, GBool printing,
>diff --git a/poppler/PDFDoc.h b/poppler/PDFDoc.h
>index 6d7dea2..22fdb40 100644
>--- a/poppler/PDFDoc.h
>+++ b/poppler/PDFDoc.h
>@@ -176,6 +176,9 @@ public:
>   // Is the file encrypted?
>   GBool isEncrypted() { return xref->isEncrypted(); }
> 
>+  // Is the file signed?
>+  GBool isSigned();
>+
>   // Check various permissions.
>   GBool okToPrint(GBool ignoreOwnerPW = gFalse)
>     { return xref->okToPrint(ignoreOwnerPW); }
>diff --git a/utils/pdfinfo.cc b/utils/pdfinfo.cc
>index 2abe8b4..b4b2233 100644
>--- a/utils/pdfinfo.cc
>+++ b/utils/pdfinfo.cc
>@@ -49,6 +49,7 @@
> #include "PDFDocEncoding.h"
> #include "Error.h"
> #include "DateInfo.h"
>+#include "Form.h"
> 
> static void printInfoString(Dict *infoDict, char *key, char *text,
> 			    UnicodeMap *uMap);
>@@ -111,6 +112,8 @@ int main(int argc, char *argv[]) {
>   int exitCode;
>   int pg, i;
>   GBool multiPage;
>+  Form *form;
>+  FormField *formField;
> 
>   exitCode = 99;
> 
>@@ -225,6 +228,61 @@ int main(int argc, char *argv[]) {
>     printf("no\n");
>   }
> 
>+  // print signature info
>+  printf("Signed:         ");
>+  if (doc->isSigned()) {
>+      printf("yes (");
>+
>+      form = doc->getCatalog()->getForm();
>+      int sigCounter = 0;
>+
>+      for (int i = 0; i < form->getNumFields(); i++) {
>+        formField = form->getRootField(i);
>+        if (formField->getType() == formSignature) {
>+            FormFieldSignature* formFieldSig
>+                    = static_cast<FormFieldSignature*> (formField);
>+
>+            GooString* contents = formFieldSig->GetContents();
>+
>+            if (i != 0) {
>+                printf("\n                     ");
>+            }
>+
>+            printf("Signature %d: encoding:%s length:%d",
>+                    ++sigCounter,
>+                    formFieldSig->GetSubFilter(),
>+                    contents->getLength());
>+
>+//            for (int i = 0; i < contents->getLength(); i++) {
>+//                printf("%x ", contents->getChar(i) & 0xff);
>+//            }
>+//            printf("\n");
>+
>+            Array *byteRange = formFieldSig->GetByteRange();
>+
>+            for (int i = 0; i < byteRange->getLength(); i+=2) {
>+                Object start, length;
>+                byteRange->get(i, &start);
>+                byteRange->get(i+1, &length);
>+                printf(" bytes: %d-%d",
>+                        start.getInt(), start.getInt() + length.getInt());
>+
>+                int oldPos = doc->getBaseStream()->getPos();
>+                doc->getBaseStream()->setPos(start.getInt());
>+//                for (int j = 0; j < length.getInt(); j++) {
>+//                    printf("%x ", doc->getBaseStream()->getChar() & 0xff);
>+//                }
>+                doc->getBaseStream()->setPos(oldPos);
>+                start.free();
>+                length.free();
>+            }
>+        }
>+      }
>+      printf(")\n");
>+  } else {
>+      printf("no\n");
>+  }
>+
>   // print page size
>   for (pg = firstPage; pg <= lastPage; ++pg) {
>     w = doc->getPageCropWidth(pg);
Comment 35 Albert Astals Cid 2012-11-01 13:23:17 UTC
weibo?
Comment 36 Jose Aliste 2012-11-05 12:12:24 UTC
(In reply to comment #33)
> OK, NSS with shared DB is what I'll pursue from now on.
> Thanks everyone for the input.

Andre, any update on the status of this? Thanks
Comment 37 Nelson Benitez 2013-08-08 12:38:48 UTC
Hi, as I cannot help with coding (on this specific issue), I've opened a bounty on FreedomSponsors:

http://www.freedomsponsors.org/core/issue/319/support-for-digital-signatures

Anyone interested on this bug is welcome to pledge more money by visiting the above link, this money will probably not be enough for the needed work time, but I see it as a nice reward in exchange for the great contribution that completing this bug is.
Comment 38 Markus Kilås 2013-08-08 14:50:40 UTC
(In reply to comment #37)
> Hi, as I cannot help with coding (on this specific issue), I've opened a
> bounty on FreedomSponsors:
> 
> http://www.freedomsponsors.org/core/issue/319/support-for-digital-signatures
> 
> Anyone interested on this bug is welcome to pledge more money by visiting
> the above link, this money will probably not be enough for the needed work
> time, but I see it as a nice reward in exchange for the great contribution
> that completing this bug is.

Greate initiative Nelson!

I also put in some coins on this one.

Cheers,
Markus
Comment 39 Tomas Gustavsson 2013-08-10 09:15:23 UTC
(In reply to comment #38)
> (In reply to comment #37)
> > Hi, as I cannot help with coding (on this specific issue), I've opened a
> > bounty on FreedomSponsors:
> > 
> > http://www.freedomsponsors.org/core/issue/319/support-for-digital-signatures
> > 
> > Anyone interested on this bug is welcome to pledge more money by visiting
> > the above link, this money will probably not be enough for the needed work
> > time, but I see it as a nice reward in exchange for the great contribution
> > that completing this bug is.
> 
> Greate initiative Nelson!
> 
> I also put in some coins on this one.
> 
> Cheers,
> Markus

Really great initiative! Added 65USD.
Would be great for free software to have this, and not have to depend on acroread for digital signature verification.

Cheers,
Tomas
Comment 40 Andreas Bürki 2013-08-16 05:31:11 UTC
(In reply to comment #37)
> Hi, as I cannot help with coding (on this specific issue), I've opened a
> bounty on FreedomSponsors:
> 
> http://www.freedomsponsors.org/core/issue/319/support-for-digital-signatures
> 
> Anyone interested on this bug is welcome to pledge more money by visiting
> the above link, this money will probably not be enough for the needed work
> time, but I see it as a nice reward in exchange for the great contribution
> that completing this bug is.

Great, thanks for your initiative. Added an other 50US$. Actual status 220, a start.

In regards of "nice to have" for me as a Consumer, it would be helpful, after an initial signature verification (of course on-line), that I can (re)verify later once in the future; even off-line or when certificate is revoked.

Keep in mind, important is only, that the certificate was valid then signing the document, but it might happen, that I'll check the signature on a contract a few years later.

Again, thx for your efforts

PS Of course, UTF-8 is must
Comment 41 Markus Kilås 2014-03-29 16:52:32 UTC
There is now a new sponsor. Bounty is up to US $300.
I hope there will be some more progress here soon.
Comment 42 Markus Wernig 2014-10-27 00:17:24 UTC
Hi all

Funding is all very well, and +1 for the initiative!

But first we'll need to find developers who are willing to take up the task.

Is there anyone interested in implementing this (immensely important, believe me!) feature? Can we spur his or her interest with money? 

Or can we say that allowing this functionality to exist in one closed-source vendor's application alone is tantamount to having your passports checked at the border by a representative of a commercial enterprise, who does not tell you by what criteria it checks the document, bit still is entitled to deny you admission?

We did a fundraiser (http://wilhelmtux.ch/?MID=11&PID=96) recently for finishing the implementation of the *signing* part in LibreOffice, which is also in C++ and uses Mozilla NSS. So maybe some of the code can be reused. Most of the original code should be available here:

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9c8dc01d3a40ec905c9d816c733ceb5d621e0426

Search by author should also work:

http://cgit.freedesktop.org/libreoffice/core/log/?qt=author&q=eraslan

As this is still work in progress, please follow https://bugs.freedesktop.org/show_bug.cgi?id=83940 for updates.
Comment 43 Ralph Corderoy 2015-01-12 11:00:39 UTC
When was this bug's importance last considered?  It was opened in 2008.  Digital signing and validation of PDFs, including using multiple timestamps from Time Stamp Authorities, is now commonplace on Windows given support in Acrobat, commercial PDF-producing libraries, etc.  The free desktop needs to catch up to be a part of that commercial world.  I'd suggest in the six years that this bug has been open the importance has risen to high.  Please might it be re-appraised?
Comment 44 Albert Astals Cid 2015-01-12 11:06:16 UTC
The bug importance doesn't matter, what matters is that if there's someone with the time to work on this.

I'd say there's not much people (or noone) in the current team with such amount of time.
Comment 45 Andre Guerreiro 2015-08-23 23:43:52 UTC
Created attachment 117885 [details] [review]
PDF signature verification using NSS
Comment 46 Andre Guerreiro 2015-08-23 23:59:09 UTC
Sorry for the long absence and here's another attempt at solving this issue.

I just attached a patch developed by me and André Esser which adds signature verification support to poppler core and the glib frontend.
It uses the NSS CMS API for the crypto operations (signature and certificate Validations).

4 new functions were added at the glib wrapper level: 
    poppler_document_is_signed
    poppler_document_signature_validate
    poppler_document_signature_get_time
    poppler_document_signature_get_signername

We added a new test utility for this feature in utils/pdfsigverify and exposed the number of signatures in poppler-glib-demo.

The trusted certificate issue for Linux systems is tackled in the following way:
we try to load the NSS certificate DB in the default Firefox profile and if that fails we try to load certificates from the standard directory /etc/pki/nssdb which may or may not be populated depending on the distro setup. We're obviously open to suggestions in this area.

Current limitation:
- The CMake changes we're not done yet so Autotools build is required for now (we couldn't find an easy/clean way to find the NSS dependency using CMake)
Comment 47 Albert Astals Cid 2015-08-24 21:19:31 UTC
Nice work.

Some small things that need fixing.

the cmake way of using pkgconfig is very simple, see ./cmake/modules/FindGTK.cmake for example

You need to free more Objects, every time you do a get object an object you have to free it, e.g. in countSignatures you need to free fields, f, type, obj1

The indentation is a bit broken, i know poppler indentation (2 soft spaces, 8 hard tabs) is a craaaaazy, but at least it'd be cool if your own functions were consistent, in validateSignature you mix tabs and spaces.

Also running valgrind it shows some memory leaks, one example

==27912==    by 0x657248F: CERT_NewTempCertificate (in /usr/lib/x86_64-linux-gnu/libnss3.so)
==27912==    by 0x4F9C4A1: SignatureHandler::CMS_SignedDataCreate(NSSCMSMessageStr*) (SignatureHandler.cc:172)
==27912==    by 0x4F9C561: SignatureHandler::SignatureHandler(unsigned char*, int) (SignatureHandler.cc:122)
==27912==    by 0x4F9383F: PDFDoc::validateSignature(int, bool) (PDFDoc.cc:534)


The file in https://bugs.kde.org/attachment.cgi?id=91442 aborts
Comment 48 André Esser 2015-08-31 22:30:35 UTC
Created attachment 118027 [details] [review]
memory leaks fixed

So here I tried fixing most memory leaks.
André Guerreiro implemented the cmake way of pkgconfig.

thoughts?
Comment 49 Andre Guerreiro 2015-09-01 18:43:18 UTC
Created attachment 118036 [details]
poppler nss signature support  - v3
Comment 50 Andre Guerreiro 2015-09-01 18:44:49 UTC
Sorry for the succession of patches. This one fixes some remaining leaks in the new PDFDoc methods and improves the indentation
Comment 51 Albert Astals Cid 2015-09-01 22:33:16 UTC
Another file that aborts
https://bugs.freedesktop.org/attachment.cgi?id=12982

Also

+/* Build against libcurl. */
+#cmakedefine ENABLE_OPENSSL 1
+

i guess just sneaked in to the patch and is not needed?


Also please use our own error() function instead of perror


Also one question, does pdfsigverify exercise all the codepaths you're adding?
Comment 52 André Esser 2015-09-02 10:59:27 UTC
Created attachment 118050 [details] [review]
poppler nss signature support  - v4

Minor changes fixing what Albert pointed out.
Fixed bug in countSignatures() for documents without signatures

pdfsigverify does go through all code paths up to PDFDoc, it doesn't go up to poppler-doccument.

But we do have a patch for evince including this signature verification feature which makes use of the poppler-doccument functions and they have worked fine with all the test pdfs.
Comment 53 Albert Astals Cid 2015-09-02 22:56:48 UTC
I've had look at the code (in poppler/ not glib/ i'll leave that to the glib/ experts) and have some minor and some major comments.

Minor: 
 * There's 3 use of malloc i could find, for completion i guess it's better if you use gmalloc
 * SigStatusStruct should be a C++ class

Medium:
 * I'm not convinced getSigningTime should return a char *, i'd very much prefer it returning an integer and shifting the responsability to turn it into a string to upper layers, since allows for translations, etc.

Major:
 * There's lots of repeated code (the loops searching for "Annot"+"Sig" some of these with more checks for nulls than others) and some functions that make no sense in PDFDoc like Object *getByteRange(int index);

If you see in Form::Form we're already parsing "Fields" and creating FormFieldSignature for each "Sig" object.

My suggestion is that you expand FormFieldSignature with your implementation.

That also solves API issues like for for example getSignerName should not be in PDFDoc since you need to have called validateSignature before and then it returns the signer name for the signature of that index, so it should be part of a signature object, i.e. FormFieldSignature.

Do you think it makes sense?
Comment 54 Andre Guerreiro 2015-09-10 15:49:26 UTC
Created attachment 118193 [details] [review]
poppler nss signature support - v5 - refactor

Here's a new patch following Albert's recommendations.
We've expanded the FormFieldSignature and FormFieldWidget classes to expose the signature method.

We also added checking for non-supported signature types as we only support pkcs7.detached signatures at the moment.

This patch only includes the changes to poppler core and the pdfsigverify utility, glib still needs changes to move the validation from poppler_document to poppler_form_field.
Comment 55 Andre Guerreiro 2015-09-10 16:25:51 UTC
Created attachment 118195 [details] [review]
poppler nss signature support - v6

Sorry, there were still some missing NULL checks and a useless new(), here's a new one.
Comment 56 Albert Astals Cid 2015-09-14 18:28:12 UTC
I've pushed your patch with some fixlets from my side to http://cgit.freedesktop.org/poppler/poppler/log/?h=signatureHandling

I have three questions we need to answer before i merge it to master:
 * Do we really want to make NSS mandatory?
 * Do we want to provide a way for frontends to give their own path to a nss database?
 * Do we want to first support other signatures like adbe.x509.rsa_sha1 ?

Also i've no clue if the implementation is correct, it seems to do nice things with some files i have laying around.

Comments?
Comment 57 Adam Reichold 2015-09-14 20:02:35 UTC
I am not sure if I am just missing where this is verified earlier, but isn't

unsigned int signed_data_len = r2.getInt()+r4.getInt();
unsigned char *to_check = (unsigned char *)gmalloc(signed_data_len);

//Read the 2 slices of data that are signed
doc->getBaseStream()->setPos(0);
doc->getBaseStream()->doGetChars(r2.getInt(), to_check);
doc->getBaseStream()->setPos(r3.getInt());
doc->getBaseStream()->doGetChars(r4.getInt(), to_check+r2.getInt());

from FormFieldSignature::validateSignature susceptible to buffer overflow?

Meaning for example if r4.getInt() < 0, then signed_data_len < r2.getInt(), so we overflow to_check in the first call to doGetChars while the second one becomes a no-op for a least the Stream and FileStream implementations.

Best regards, Adam.
Comment 58 Andre Guerreiro 2015-09-15 00:32:28 UTC
@Albert

Thanks for the improvements.
Regarding your 3 questions:

1- I've no objection to make the feature optional. I understand there are people building more minimal versions of poppler that dislike additional dependencies.

2- Yes we should. Our defaults are meant to take advantage of the already configured and implicitly trusted NSS cert DBs in Firefox but we shouldn't limit the user's options.

3- We're already looking into supporting adbe.pkcs7.sha1 signatures which we found are very widespread. In Portugal we found that several big companies are still using this kind of signatures in signed PDF invoices as of now.
Comment 59 Mantas Mikulėnas 2015-09-15 04:12:00 UTC
Hmm, why does this search for Firefox's profile – why not use the shared nssdb (sql:~/.pki/nssdb) like Evolution and Chromium do?
Comment 60 Adrian Johnson 2015-09-15 11:34:31 UTC
I don't think NSS should be mandatory. It is not required for rendering PDFs.  configure/cmake should include NSS in the "building poppler with support for" summary.

>  byte_range->arrayGet(1, &r2);
>  byte_range->arrayGet(2, &r3);
>  byte_range->arrayGet(3, &r4);
>
>  unsigned int signed_data_len = r2.getInt()+r4.getInt();
>  unsigned char *to_check = (unsigned char *)gmalloc(signed_data_len);
>
>  //Read the 2 slices of data that are signed
>  doc->getBaseStream()->setPos(0);
>  doc->getBaseStream()->doGetChars(r2.getInt(), to_check);
>  doc->getBaseStream()->setPos(r3.getInt());
>  doc->getBaseStream()->doGetChars(r4.getInt(), to_check+r2.getInt());

This does not support large files (> 2GB). The code should check if the objects in byte_range are int64 and use the Goffset type for setting file offsets.

It would be better to process the range in chunks instead of reading the entire range into a buffer. We don't know how big the range could be and it is not necessary to read it all at once as HASH_Begin/HASH_Update/HASH_End can be used to process it in chunks.

I would prefer it if all the NSS includes were only included in the .cc files similar to what we do with the image formats (JPEG2000Stream.h, JpegWriter.h etc). It speeds up compilation, avoids clashes with other include files, and makes it easier to replace NSS and/or support other security libraries if we choose to do so in future.

The pdfsigutil utility doesn't seem to do much. Unless there are plans to add a lot of extra options and functionality would it be better to add an option to pdfinfo to check signatures instead of adding a new util?
Comment 61 Albert Astals Cid 2015-09-21 17:37:11 UTC
Andre*2, answers to the comments from Adam, Adrian and Mantas?
Comment 62 Andre Guerreiro 2015-09-22 14:04:18 UTC
Created attachment 118398 [details] [review]
Fix for Buffer overflow

Regarding the illegal ByteRange values which would cause overflow this patch should fix it
Comment 63 Andre Guerreiro 2015-09-22 14:17:14 UTC
@Adrian

Thanks for the tips on support for large files, progressive hashing and the NSS includes. We'll be posting our attempts to improve these issues as individual patches.

I also thought of adding the feature to pdfinfo but it seems wrong to mix up something which performs various computations and relies on external state (NSS cert DB) to pdfinfo which just reads metadata from the file itself.
Comment 64 Adrian Johnson 2015-09-23 11:33:44 UTC
(In reply to Andre Guerreiro from comment #63)
> I also thought of adding the feature to pdfinfo but it seems wrong to mix up
> something which performs various computations and relies on external state
> (NSS cert DB) to pdfinfo which just reads metadata from the file itself.

pdfinfo already performs various computations. If the file is encrypted it has to decrypt the objects to read the file. Reading from other sources is not an issue if a switch is used to enable the signature checking.

It seems pointless to me to add an entire new utility to perform one function that can be enabled by adding a switch to pdfinfo.

I'll leave it to Albert decide. But if we keep pdfsigverify I suggest making it more generic to allow for the possibility of later adding the capability to sign pdfs. eg call it pdfsig and if verifying signatures it not a fast operation, maybe enable the check with a switch and make the default operation (ie no switch specified) report whether the pdf contains a signature.
Comment 65 Albert Astals Cid 2015-09-24 22:00:21 UTC
I've commited the overflow patch, please Adrian's check if it makes you happy.

I also like the pdfsig idea.

What do we think it's missing before we cna merge this to the mainline?
Comment 66 Andre Guerreiro 2015-09-25 16:28:58 UTC
Created attachment 118446 [details] [review]
NSS conditional build

This patch makes the NSS dependency optional in the CMake and Autotools build systems.
Comment 67 Andre Guerreiro 2015-09-25 17:36:49 UTC
Created attachment 118449 [details] [review]
Support for adbe.pkcs7.sha1 signatures

This patch, to be applied over the previous one, adds support for adbe.pkcs7.sha1 signatures so now we should have a more complete coverage of actual signed PDFs.
Comment 68 Albert Astals Cid 2015-10-05 13:48:07 UTC
(In reply to Andre Guerreiro from comment #67)
> Created attachment 118449 [details] [review] [review]
> Support for adbe.pkcs7.sha1 signatures
> 
> This patch, to be applied over the previous one, adds support for
> adbe.pkcs7.sha1 signatures so now we should have a more complete coverage of
> actual signed PDFs.

I have a few adbe.x509.rsa_sha1 in case you need them for testing.
Comment 69 Albert Astals Cid 2015-10-05 14:48:43 UTC
(In reply to Andre Guerreiro from comment #66)
> Created attachment 118446 [details] [review] [review]
> NSS conditional build
> 
> This patch makes the NSS dependency optional in the CMake and Autotools
> build systems.

Commited too to the signatureHandling branch with some minor changes on my side to have fewer
Comment 70 Albert Astals Cid 2015-10-05 14:51:22 UTC
So we have two things missing, right?
 * Enabling the use of other keystores
 * The name of the pdfsigverify tool


I think the enabling the use of other keystores is important but maybe not critical for the first release.

For the pdfsigverify it seems we kind of agreeed on the compromise to call it pdfsig that only does verification right now but in the future may suppport signing?


Did i forget something?
Comment 71 Adrian Johnson 2015-10-05 21:35:36 UTC
(In reply to Albert Astals Cid from comment #70)
> So we have two things missing, right?
>  * Enabling the use of other keystores
>  * The name of the pdfsigverify tool
> 
> 
> I think the enabling the use of other keystores is important but maybe not
> critical for the first release.
> 
> For the pdfsigverify it seems we kind of agreeed on the compromise to call
> it pdfsig that only does verification right now but in the future may
> suppport signing?
> 
> 
> Did i forget something?

Large file support.
Comment 72 Albert Astals Cid 2015-10-05 21:56:02 UTC
(In reply to Adrian Johnson from comment #71)
> (In reply to Albert Astals Cid from comment #70)
> > So we have two things missing, right?
> >  * Enabling the use of other keystores
> >  * The name of the pdfsigverify tool
> > 
> > 
> > I think the enabling the use of other keystores is important but maybe not
> > critical for the first release.
> > 
> > For the pdfsigverify it seems we kind of agreeed on the compromise to call
> > it pdfsig that only does verification right now but in the future may
> > suppport signing?
> > 
> > 
> > Did i forget something?
> 
> Large file support.

Do you think this should be a blocker for the initial release or something we can improve later?
Comment 73 Adrian Johnson 2015-10-05 22:56:26 UTC
(In reply to Albert Astals Cid from comment #72)
> > Large file support.
> 
> Do you think this should be a blocker for the initial release or something
> we can improve later?

I think it should be included.
Comment 74 Albert Astals Cid 2015-10-06 12:36:24 UTC
(In reply to Adrian Johnson from comment #73)
> (In reply to Albert Astals Cid from comment #72)
> > > Large file support.
> > 
> > Do you think this should be a blocker for the initial release or something
> > we can improve later?
> 
> I think it should be included.

Do you have any file like that or a way to create it so the guys can test it?
Comment 75 Adrian Johnson 2015-10-06 21:48:00 UTC
(In reply to Albert Astals Cid from comment #74)
> (In reply to Adrian Johnson from comment #73)
> > (In reply to Albert Astals Cid from comment #72)
> > > > Large file support.
> > > 
> > > Do you think this should be a blocker for the initial release or something
> > > we can improve later?
> > 
> > I think it should be included.
> 
> Do you have any file like that or a way to create it so the guys can test it?

I don't have a file. Bug 44085 contains a large file test case. But I don't know how to sign it.

Implementing large file support is simply a case of using obj.isInt64()/ obj.getInt64() when getting the range offsets.

In addition, the code should avoid reading the entire range into memory instead processing it in smaller chunks. From a quick look a the nss headers it appears to have support for incremental hashing. This is good practice even for < 2GB files as I have seen scanned PDFs that are in the 100's of MB range.

I don't mind adding the getInt64() stuff myself as that is easy to do without needing to understand the rest of the code. But the incremental hashing would best be done by the original developer of the patch.

So at a a minimum if we can get the incremental hashing in then I can sort out the rest of the 64-bit support.
Comment 76 Andre Guerreiro 2015-10-08 00:40:42 UTC
Created attachment 118745 [details] [review]
Incremental hashing + large file support

With this patch I've implemented the incremental hashing plus the large file support.

This is still untested with files larger than 2GB but is correct for all the regular test cases I gathered before.
Comment 77 Adam Reichold 2015-10-08 06:29:49 UTC
(In reply to Andre Guerreiro from comment #76)
> Created attachment 118745 [details] [review] [review]
> Incremental hashing + large file support
> 
> With this patch I've implemented the incremental hashing plus the large file
> support.
> 
> This is still untested with files larger than 2GB but is correct for all the
> regular test cases I gathered before.

Some minor suggestions:

* The naming of BLOCK_SIZE and block_len in hashSignedDataBlock seems misleading to me, maybe CHUNK_SIZE and block_len?

* The method hashSignedDataBlock could probably be replaced by a static function taking the stream and the handler? This should give the compiler more optimization possibilities than if it is visible in other translation units.

* I think the while loop within could become a for loop for better readability with the case reduced to computing the number of bytes to read instead of two separate calls to doGetChars and updateHash.
Comment 78 Adrian Johnson 2015-10-08 11:34:49 UTC
(In reply to Adam Reichold from comment #77)
> Some minor suggestions:
> 
> * The naming of BLOCK_SIZE and block_len in hashSignedDataBlock seems
> misleading to me, maybe CHUNK_SIZE and block_len?
> 
> * The method hashSignedDataBlock could probably be replaced by a static
> function taking the stream and the handler? This should give the compiler
> more optimization possibilities than if it is visible in other translation
> units.
> 
> * I think the while loop within could become a for loop for better
> readability with the case reduced to computing the number of bytes to read
> instead of two separate calls to doGetChars and updateHash.

I don't think there is much benefit in making that function a static. It is not something that is called a frequently from an inner loop.

I think the function would read better as "hashSignedByteRange" as "byte range" is the terminology used in the PDF reference section in signatures. Instead of setting the stream offset before calling this function it would better easier to understand the code if the function took an offset and length.

I agree that the two calls to doGetChars and updateHash should be merged but I don't think a for loop is the best way to process loops where the increment is not exactly the same on each iteration. Maybe something like this:

  void FormFieldSignature::hashSignedByteRange(SignatureHandler *handler,
     Goffset start, Goffset len)
  {
    const int CHUNK_SIZE = 4096;
    unsigned char buffer[CHUNK_SIZE];
    Goffset i = 0;
    int byte_count = CHUNK_SIZE;

    doc->getBaseStream()->setPos(start);
    while (i < len)
    {
      if (i + CHUNK_SIZE > len)
        byte_count = len - i;

      doc->getBaseStream()->doGetChars(byte_count, buffer);
      handler->updateHash(buffer, byte_count);
      i += byte_count;
    }
  }

I renamed the "signed_data_buffer" to "buffer" as whenever I see "signed" in C/C++ I think of the integer type. "unsigned char signed_data_buffer" is confusing to read.

I would prefer the buffer be moved out to the class. It is better not to allocate large buffers on the stack. We may later increase the buffer size.

I don't mind if we fix all this later. It doesn't have to hold up the initial release.
Comment 79 Adrian Johnson 2015-10-08 11:41:38 UTC
+  r_values[0] = r2.isInt64() ? r2.getInt64() : r2.getInt();
+  r_values[1] = r3.isInt64() ? r3.getInt64() : r3.getInt();
+  r_values[2] = r4.isInt64() ? r4.getInt64() : r4.getInt();
 
According the PDF Reference, the ByteRange array contains pairs of (offset,length).

Why do we ignore the first offset and later assume it is 0? Why do we assume there are exactly two pairs.

I only skimmed over the digital signatures section so maybe I missed something.
Comment 80 Adrian Johnson 2015-10-08 11:55:09 UTC
(In reply to Albert Astals Cid from comment #70)
> For the pdfsigverify it seems we kind of agreeed on the compromise to call
> it pdfsig that only does verification right now but in the future may
> suppport signing?

This is the only thing left that I think needs fixing before the initial release. A couple of questions:

- Given that we want to keep the option open for using pdfsig to perform other signature operations, should the default behavior when only a pdf file is specified be to verify the signature? Or should we just indicate whether the pdf is signed and use an option to enable verification?

- Is '-c' the best option name for "don't perform certificate validation"? 

We also need a man page.
Comment 81 Adam Reichold 2015-10-08 13:51:25 UTC
(In reply to Adrian Johnson from comment #78)
> I agree that the two calls to doGetChars and updateHash should be merged but
> I don't think a for loop is the best way to process loops where the
> increment is not exactly the same on each iteration. Maybe something like
> this:
> 
>   void FormFieldSignature::hashSignedByteRange(SignatureHandler *handler,
>      Goffset start, Goffset len)
>   {
>     const int CHUNK_SIZE = 4096;
>     unsigned char buffer[CHUNK_SIZE];
>     Goffset i = 0;
>     int byte_count = CHUNK_SIZE;
> 
>     doc->getBaseStream()->setPos(start);
>     while (i < len)
>     {
>       if (i + CHUNK_SIZE > len)
>         byte_count = len - i;
> 
>       doc->getBaseStream()->doGetChars(byte_count, buffer);
>       handler->updateHash(buffer, byte_count);
>       i += byte_count;
>     }
>   }

I was thinking of something like

doc->getBaseStream()->setPos(start);
for (Goffset offset = 0; offset < len; offset += CHUNK_SIZE)
{
  const int byte_count = min(CHUNK_SIZE, len - offset);

  doc->getBaseStream()->doGetChars(byte_count, buffer);
  handler->updateHash(buffer, byte_count);
}

to make the loop more regular as we don't care if offset > len instead of offset == len after the last iteration. (That code was not tested or even compiled.)

> I don't mind if we fix all this later. It doesn't have to hold up the
> initial release.

Maybe if there is some external pressure to release this, but otherwise I'd propose polishing it now, since there is a certain momentum not to touch working code once it is released.

Best regards, Adam.
Comment 82 Albert Astals Cid 2015-10-14 20:43:29 UTC
(In reply to Adrian Johnson from comment #80)
> (In reply to Albert Astals Cid from comment #70)
> > For the pdfsigverify it seems we kind of agreeed on the compromise to call
> > it pdfsig that only does verification right now but in the future may
> > suppport signing?
> 
> This is the only thing left that I think needs fixing before the initial
> release. A couple of questions:
> 
> - Given that we want to keep the option open for using pdfsig to perform
> other signature operations, should the default behavior when only a pdf file
> is specified be to verify the signature? Or should we just indicate whether
> the pdf is signed and use an option to enable verification?

I think defaulting to verification makes sense as the "non destructive thing to do".

> - Is '-c' the best option name for "don't perform certificate validation"? 

Honestly i don't think spending much time on discussing the option name makes much sense, we just need to document it properly and that's it.

> 
> We also need a man page.

Yep.
Comment 83 Albert Astals Cid 2015-10-14 20:44:33 UTC
(In reply to Adam Reichold from comment #81)
> (In reply to Adrian Johnson from comment #78)
> > I agree that the two calls to doGetChars and updateHash should be merged but
> > I don't think a for loop is the best way to process loops where the
> > increment is not exactly the same on each iteration. Maybe something like
> > this:
> > 
> >   void FormFieldSignature::hashSignedByteRange(SignatureHandler *handler,
> >      Goffset start, Goffset len)
> >   {
> >     const int CHUNK_SIZE = 4096;
> >     unsigned char buffer[CHUNK_SIZE];
> >     Goffset i = 0;
> >     int byte_count = CHUNK_SIZE;
> > 
> >     doc->getBaseStream()->setPos(start);
> >     while (i < len)
> >     {
> >       if (i + CHUNK_SIZE > len)
> >         byte_count = len - i;
> > 
> >       doc->getBaseStream()->doGetChars(byte_count, buffer);
> >       handler->updateHash(buffer, byte_count);
> >       i += byte_count;
> >     }
> >   }
> 
> I was thinking of something like
> 
> doc->getBaseStream()->setPos(start);
> for (Goffset offset = 0; offset < len; offset += CHUNK_SIZE)
> {
>   const int byte_count = min(CHUNK_SIZE, len - offset);
> 
>   doc->getBaseStream()->doGetChars(byte_count, buffer);
>   handler->updateHash(buffer, byte_count);
> }
> 
> to make the loop more regular as we don't care if offset > len instead of
> offset == len after the last iteration. (That code was not tested or even
> compiled.)
> 
> > I don't mind if we fix all this later. It doesn't have to hold up the
> > initial release.
> 
> Maybe if there is some external pressure to release this, but otherwise I'd
> propose polishing it now, since there is a certain momentum not to touch
> working code once it is released.

If we're too picky it may just die in this branch and never be merged :D

> 
> Best regards, Adam.
Comment 84 Albert Astals Cid 2015-10-14 20:51:00 UTC
(In reply to Andre Guerreiro from comment #76)
> Created attachment 118745 [details] [review] [review]
> Incremental hashing + large file support
> 
> With this patch I've implemented the incremental hashing plus the large file
> support.
> 
> This is still untested with files larger than 2GB but is correct for all the
> regular test cases I gathered before.

I've pushed this, Adrian, Adam please suggest your improvements over it with a patch if you reached an agreement.
Comment 85 Adrian Johnson 2015-10-14 20:56:08 UTC
(In reply to Albert Astals Cid from comment #82)
> Honestly i don't think spending much time on discussing the option name
> makes much sense, we just need to document it properly and that's it.

I raised this issue because pdfimages uses -j for jpeg but -png, -jp2, -jbig2, -tiff, and -ccitt for the other formats. I don't want pdfsig to end up in the same situation.
Comment 86 Ralph Corderoy 2015-10-15 09:12:00 UTC
(In reply to Albert Astals Cid from comment #82)
> > - Is '-c' the best option name for "don't perform certificate validation"? 
> 
> Honestly i don't think spending much time on discussing the option name
> makes much sense, we just need to document it properly and that's it.

It's vital to put thought into "naming things" before a poor decision becomes permanent because it becomes public.  At the very least, "-c" suggests check and "-C" would often be the negative don't check, even if no -c option exists because it's the default.

And then there's the issue Adrian raises.
Comment 87 Markus Kilås 2015-10-24 10:23:29 UTC
I'm trying to build the branch (on Fedora 21) but obviously I am missing some dependency or not having the right versions etc. Anyone having an idea?

Build output:
---
[user@dev-21 poppler]$ sudo yum install nspr-devel
Loaded plugins: langpacks, post-transaction-actions, yum-qubes-hooks
Package nspr-devel-4.10.8-1.fc21.x86_64 already installed and latest version
Nothing to do
[user@dev-21 poppler]$ make
make  all-recursive
make[1]: Entering directory '/home/user/VersionControlled/poppler'
Making all in goo
make[2]: Entering directory '/home/user/VersionControlled/poppler/goo'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/home/user/VersionControlled/poppler/goo'
Making all in fofi
make[2]: Entering directory '/home/user/VersionControlled/poppler/fofi'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/home/user/VersionControlled/poppler/fofi'
Making all in splash
make[2]: Entering directory '/home/user/VersionControlled/poppler/splash'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/home/user/VersionControlled/poppler/splash'
Making all in poppler
make[2]: Entering directory '/home/user/VersionControlled/poppler/poppler'
make  all-am
make[3]: Entering directory '/home/user/VersionControlled/poppler/poppler'
  CXX      libpoppler_la-SignatureHandler.lo
In file included from SignatureHandler.cc:15:0:
SignatureHandler.h:20:24: fatal error: nspr/prprf.h: No such file or directory
 #include <nspr/prprf.h>
                        ^
compilation terminated.
Makefile:1084: recipe for target 'libpoppler_la-SignatureHandler.lo' failed
make[3]: *** [libpoppler_la-SignatureHandler.lo] Error 1
make[3]: Leaving directory '/home/user/VersionControlled/poppler/poppler'
Makefile:840: recipe for target 'all' failed
make[2]: *** [all] Error 2
make[2]: Leaving directory '/home/user/VersionControlled/poppler/poppler'
Makefile:642: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/user/VersionControlled/poppler'
Makefile:524: recipe for target 'all' failed
make: *** [all] Error 2
---
Comment 88 Markus Kilås 2015-10-24 10:57:07 UTC
(In reply to Markus Kilås from comment #87)
> I'm trying to build the branch (on Fedora 21) but obviously I am missing
> some dependency or not having the right versions etc. Anyone having an idea?
> 
> Build output:
> ---
> [user@dev-21 poppler]$ sudo yum install nspr-devel
> Loaded plugins: langpacks, post-transaction-actions, yum-qubes-hooks
> Package nspr-devel-4.10.8-1.fc21.x86_64 already installed and latest version
> Nothing to do
> [user@dev-21 poppler]$ make
> make  all-recursive
> make[1]: Entering directory '/home/user/VersionControlled/poppler'
> Making all in goo
> make[2]: Entering directory '/home/user/VersionControlled/poppler/goo'
> make[2]: Nothing to be done for 'all'.
> make[2]: Leaving directory '/home/user/VersionControlled/poppler/goo'
> Making all in fofi
> make[2]: Entering directory '/home/user/VersionControlled/poppler/fofi'
> make[2]: Nothing to be done for 'all'.
> make[2]: Leaving directory '/home/user/VersionControlled/poppler/fofi'
> Making all in splash
> make[2]: Entering directory '/home/user/VersionControlled/poppler/splash'
> make[2]: Nothing to be done for 'all'.
> make[2]: Leaving directory '/home/user/VersionControlled/poppler/splash'
> Making all in poppler
> make[2]: Entering directory '/home/user/VersionControlled/poppler/poppler'
> make  all-am
> make[3]: Entering directory '/home/user/VersionControlled/poppler/poppler'
>   CXX      libpoppler_la-SignatureHandler.lo
> In file included from SignatureHandler.cc:15:0:
> SignatureHandler.h:20:24: fatal error: nspr/prprf.h: No such file or
> directory
>  #include <nspr/prprf.h>
>                         ^
> compilation terminated.
> Makefile:1084: recipe for target 'libpoppler_la-SignatureHandler.lo' failed
> make[3]: *** [libpoppler_la-SignatureHandler.lo] Error 1
> make[3]: Leaving directory '/home/user/VersionControlled/poppler/poppler'
> Makefile:840: recipe for target 'all' failed
> make[2]: *** [all] Error 2
> make[2]: Leaving directory '/home/user/VersionControlled/poppler/poppler'
> Makefile:642: recipe for target 'all-recursive' failed
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory '/home/user/VersionControlled/poppler'
> Makefile:524: recipe for target 'all' failed
> make: *** [all] Error 2
> ---

I found a workaround for my build issue by creating symlinks:
/usr/include/nspr -> /usr/include/nspr4
/usr/inclyde/nss -> /usr/include/nss3
Comment 89 Adrian Johnson 2015-10-24 11:07:33 UTC
(In reply to Markus Kilås from comment #88)
> I found a workaround for my build issue by creating symlinks:
> /usr/include/nspr -> /usr/include/nspr4
> /usr/inclyde/nss -> /usr/include/nss3

Are you using configure or cmake? The nss pkgconfig file should contain the correct include path.
Comment 90 Markus Kilås 2015-10-24 12:05:06 UTC
(In reply to Adrian Johnson from comment #89)
> (In reply to Markus Kilås from comment #88)
> > I found a workaround for my build issue by creating symlinks:
> > /usr/include/nspr -> /usr/include/nspr4
> > /usr/inclyde/nss -> /usr/include/nss3
> 
> Are you using configure or cmake? The nss pkgconfig file should contain the
> correct include path.

After checking out the branch I read INSTALL but did not find any ./configure so I run ./autogen.sh. It first failed for many different reasons which eventually got resolved by installing missing dependencies. After that I run the now generated ./configure and then make.
Comment 91 Adrian Johnson 2015-10-24 12:08:11 UTC
What is the output of "pkg-config --cflags nss"?
Comment 92 Markus Kilås 2015-10-24 12:23:02 UTC
(In reply to Adrian Johnson from comment #91)
> What is the output of "pkg-config --cflags nss"?

[user@dev-21 poppler]$ pkg-config --cflags nss
-I/usr/include/nss3 -I/usr/include/nspr4
Comment 93 Markus Kilås 2015-10-24 14:32:14 UTC
Created attachment 119174 [details]
Handle SEC_ERROR_UNTRUSTED_ISSUER

When verifying a PDF signed by a certificate issued by a CA not in the trust store I would expect to get an error "Certificate isn't Trusted" however currently the error message actually is the more generic "Unknown issue with Certificate or corrupted data".

I tested with http://wwwpriv.primekey.se/~markus/pdfsigner/test-pdfs/signserver-res-test-pdf/sample-signed.pdf:

[user@dev-21 utils]$ ./pdfsigverify ~/Downloads/signserver-res-test-pdf/sample-signed.pdf 
Digital Signature Info of: /home/user/Downloads/signserver-res-test-pdf/sample-signed.pdf
Signature #1:
  - Signer Certificate Common Name: Signer 2
  - Signing Time: Nov 23 2011 17:09:42
  - Signature Validation: Signature is Valid.
  - Certificate Validation: Unknown issue with Certificate or corrupted data.

Then after adding http://wwwpriv.primekey.se/~markus/pdfsigner/test-pki/signserver-res-test-dss10/DSSRootCA10.cacert.pem as trusted in Firefox:

[user@dev-21 utils]$ ./pdfsigverify ~/Downloads/signserver-res-test-pdf/sample-signed.pdf 
Digital Signature Info of: /home/user/Downloads/signserver-res-test-pdf/sample-signed.pdf
Signature #1:
  - Signer Certificate Common Name: Signer 2
  - Signing Time: Nov 23 2011 17:09:42
  - Signature Validation: Signature is Valid.
  - Certificate Validation: Certificate is Trusted.

The attached patch adds the NSS error code for untrusted issuer so the error will be "Certificate isn't trusted".
Comment 94 Adrian Johnson 2015-10-27 12:47:23 UTC
I suggest we change the pdfsig "-c" switch to "-nocert". We can easily add a single character option if we find it is needed. But we can't get rid of it if we later regret it.

I'm still waiting for an answer to comment 79.
Comment 95 Albert Astals Cid 2015-10-27 21:28:56 UTC
(In reply to Markus Kilås from comment #93)
> Created attachment 119174 [details]
> Handle SEC_ERROR_UNTRUSTED_ISSUER

I pushed this and also some code to differentiate between an unknown and an untrusted issuer (no idea what's the difference but if nss has this difference i feel we also have to)
Comment 96 Albert Astals Cid 2015-10-27 21:30:11 UTC
(In reply to Adrian Johnson from comment #94)
> I suggest we change the pdfsig "-c" switch to "-nocert". We can easily add a
> single character option if we find it is needed. But we can't get rid of it
> if we later regret it.
> 
> I'm still waiting for an answer to comment 79.

As said please let's not spend too much time on arguing over switch names, if you really really want -nocert, just push a patch to the signatureHandling branch that switches to it.
Comment 97 Andre Guerreiro 2015-10-28 00:02:00 UTC
(In reply to Adrian Johnson from comment #79)
> +  r_values[0] = r2.isInt64() ? r2.getInt64() : r2.getInt();
> +  r_values[1] = r3.isInt64() ? r3.getInt64() : r3.getInt();
> +  r_values[2] = r4.isInt64() ? r4.getInt64() : r4.getInt();
>  
> According the PDF Reference, the ByteRange array contains pairs of
> (offset,length).
> 
> Why do we ignore the first offset and later assume it is 0? Why do we assume
> there are exactly two pairs.
> 
> I only skimmed over the digital signatures section so maybe I missed
> something.

Actually the PDF spec allows for more than 2 pairs of values in /ByteRange but it would mean that there is more than one gap in the signed data apart from the signature itself. Quoting from ISO 32000-1 section 12.8.1: 
"This range should be the entire file, including the signature dictionary but excluding the signature value itself (the Contents entry). Other ranges may be used but since they do not check for all changes to the document, their use is not recommended."

Obviously in a file with multiple signatures each signature should cover the latest revision present in the file when the signature was appended.
Comment 98 Adrian Johnson 2015-10-28 10:30:10 UTC
(In reply to Albert Astals Cid from comment #96)
> As said please let's not spend too much time on arguing over switch names,
> if you really really want -nocert, just push a patch to the
> signatureHandling branch that switches to it.

Done. I've also added a man page. If any further information that should be in the man page can be posted here I can format it into the man page format and push it out.
Comment 99 Adrian Johnson 2015-10-28 10:35:16 UTC
(In reply to Andre Guerreiro from comment #97)
> Quoting from ISO 32000-1 section 12.8.1: 
> "This range should be the entire file, including the signature dictionary
> but excluding the signature value itself (the Contents entry). Other ranges
> may be used but since they do not check for all changes to the document,
> their use is not recommended."

This advice is for PDF producers. As a PDF consumer we should accept PDFs that don't follow this advice.

https://en.wikipedia.org/wiki/Robustness_principle
Comment 100 Andre Guerreiro 2015-10-29 11:12:22 UTC
Created attachment 119283 [details] [review]
Manpage improvement

Here's an improvement to the manpage. 
Corrected a typo and added some missing context
Comment 101 Adrian Johnson 2015-10-29 11:31:26 UTC
(In reply to Andre Guerreiro from comment #100)
> Created attachment 119283 [details] [review] [review]
> Manpage improvement

Pushed
Comment 102 Albert Astals Cid 2015-11-24 22:24:24 UTC
So we're stuck on "need to use the offset" part, right?

Could someone try to do make the code use it even if we don't have any pdf that needs it?
Comment 103 Markus Kilås 2015-11-28 16:02:03 UTC
(In reply to Albert Astals Cid from comment #102)
> So we're stuck on "need to use the offset" part, right?
> 
> Could someone try to do make the code use it even if we don't have any pdf
> that needs it?

I am not sure if it is good to apply the robustness principle on security functions. In those cases it might be better to be defensive and reject signatures not following the recommendation.

In this case if the ByteRange does not cover the whole document there could be parts of the document that can be modified without invalidating the signature. Would it then be good to tell the user that the signature has been validated and the document is not modified even though in fact there are parts of the document for which we don't know?
Comment 104 Markus Wernig 2015-11-28 16:15:14 UTC
I think the correct statement to issue in this case would be that a part ("revision") of the PDF has been signed and to show the validation result for that revision, ideally giving the user a chance to view only the signed part.

This might even be indispensable when there are multiple signatures applied to the document.
Comment 105 Andre Guerreiro 2015-12-09 16:04:01 UTC
Created attachment 120434 [details] [review]
Improve robustness of SignatureHandler::validateCertificate

This patch adds additional NULL-checking in SignatureHandler::validateCertificate() which avoids segfault for some signatures like the one contained here: http://www.gpo.gov/fdsys/pkg/BUDGET-2015-BUD/pdf/BUDGET-2015-BUD.pdf

It also removes a useless branch in validateCertificate()
Comment 106 Markus Kilås 2016-01-01 19:23:27 UTC
Created attachment 120758 [details] [review]
Fix printf for unsigned int

In pdfsig.cc the NetBeans IDE gave warning:
Mismatching the argument type "unsigned int" and conversion specifier "d"

The attached patch changes from %d to %u in the printf.
Comment 107 Markus Kilås 2016-01-01 21:56:05 UTC
(In reply to Markus Wernig from comment #104)
> I think the correct statement to issue in this case would be that a part
> ("revision") of the PDF has been signed and to show the validation result
> for that revision, ideally giving the user a chance to view only the signed
> part.
> 
> This might even be indispensable when there are multiple signatures applied
> to the document.

So one idea could be to also use the first offset when checking the signature and in case it is not zero instead of return the status as SIGNATURE_VALID say something like "SIGNATURE_PARTLY_VALID".

The pdfsig tool already implements support for multiple signatures/"revisions" but it seems the API does not expose the ByteRange for which the signature covers nor provides an other way to obtain that revision of the document (?).
Comment 108 Markus Kilås 2016-01-01 22:54:49 UTC
Created attachment 120760 [details] [review]
Considering offset 0 and signature only covering part of PDF

Note that this patch has not been tested with a non-zero first offset document.
Comment 109 Albert Astals Cid 2016-01-06 22:37:33 UTC
(In reply to Markus Kilås from comment #106)
> Created attachment 120758 [details] [review] [review]
> Fix printf for unsigned int
> 
> In pdfsig.cc the NetBeans IDE gave warning:
> Mismatching the argument type "unsigned int" and conversion specifier "d"
> 
> The attached patch changes from %d to %u in the printf.

Pushed
Comment 110 Albert Astals Cid 2016-01-06 22:38:56 UTC
(In reply to Markus Kilås from comment #108)
> Created attachment 120760 [details] [review] [review]
> Considering offset 0 and signature only covering part of PDF
> 
> Note that this patch has not been tested with a non-zero first offset
> document.

Adrian what do you think of this patch?
Comment 111 Adrian Johnson 2016-01-07 12:31:05 UTC
I don't think we need to check if the byte range covers the entire document. Our job, when verifying the signature, is to use the byte ranges provided in the signature dictionary. It is up to the pdf producer to ensure the byte range covers the entire document (excluding the signature value). 

All we need to do is ensure we check all bytes ranges in the ByteRange array. We should also check that each byte range is within the file. eg check that each offset is >= 0 and offset + length <= file size.

While it would be nice to check if the byte range covers the entire document, poppler does not provide any easy way to determine the file offsets of a dictionary value. This makes it difficult to check if the excluded range only covers the signature value.
Comment 112 Adrian Johnson 2016-01-08 06:28:18 UTC
Created attachment 120889 [details] [review]
Check in ranges in ByteRange array

This is an alternative to the patch in comment 108. It checks all ranges in the ByteRange array.
Comment 113 Adrian Johnson 2016-01-08 06:35:56 UTC
(In reply to Andre Guerreiro from comment #105)
> Created attachment 120434 [details] [review] [review]
> Improve robustness of SignatureHandler::validateCertificate
> 
> This patch adds additional NULL-checking in
> SignatureHandler::validateCertificate() which avoids segfault for some
> signatures like the one contained here:
> http://www.gpo.gov/fdsys/pkg/BUDGET-2015-BUD/pdf/BUDGET-2015-BUD.pdf
> 
> It also removes a useless branch in validateCertificate()

Pushed
Comment 114 Albert Astals Cid 2016-01-08 22:21:13 UTC
(In reply to Adrian Johnson from comment #112)
> Created attachment 120889 [details] [review] [review]
> Check in ranges in ByteRange array
> 
> This is an alternative to the patch in comment 108. It checks all ranges in
> the ByteRange array.

I'm not very convinced by the naming of 
  GBool isInteger() { return type == objInt || type == objInt64; }
what do you think of 
  isIntOrInt64() ?
Comment 115 Adrian Johnson 2016-01-08 22:45:43 UTC
(In reply to Albert Astals Cid from comment #114)
> I'm not very convinced by the naming of 
>   GBool isInteger() { return type == objInt || type == objInt64; }
> what do you think of 
>   isIntOrInt64() ?

I prefer a single word instead of isXXXorYYY() but I can appreciate that isInteger() could easily be confused with isInt(). I'll change it to isIntOrInt64().

I'll also add a corresponding getIntOrInt64() and replace the
"xxx.isInt64() ? xxx.getInt64() : xxx.getInt()" expressions with a single getter.
Comment 116 Adrian Johnson 2016-01-10 03:46:21 UTC
Created attachment 120924 [details] [review]
Check in ranges in ByteRange array v2

- renamed isInteger() to isIntOrInt64()
- added and use getIntOrInt64()
Comment 117 Andre Guerreiro 2016-01-12 19:06:49 UTC
Created attachment 120992 [details] [review]
Load NSS root certs module

This change is needed to actually do certificate validation, because as it is NSS is trying to load the module which contains all the builtin root certs from the Firefox profile directory where it is usually missing. This way it will load the module from a system library directory.
Comment 118 Albert Astals Cid 2016-01-13 22:48:51 UTC
Andre and Andre what do you think of Adrian's patch?
Comment 119 Andre Guerreiro 2016-01-14 10:58:28 UTC
I'm in favour of Adrian's patch. It's an improvement with additional sanity checks on the ByteRange values. 
Indeed I tried to see if you could check if a given ByteRange covers the whole document and also found no easy way to do it with existing poppler functions/APIs.
Comment 120 Albert Astals Cid 2016-01-14 22:14:29 UTC
Ok, good, so Adrian can you commit your patch?

After that we have attachment 120992 [details] [review] "Load NSS root certs module" that honestly I don't understand at all but some googleing seems to confirm it's needed.

And that would be it to try to merge it into master?
Comment 121 Adrian Johnson 2016-01-14 22:22:32 UTC
(In reply to Albert Astals Cid from comment #120)
> Ok, good, so Adrian can you commit your patch?

Pushed
Comment 122 Albert Astals Cid 2016-02-16 22:42:15 UTC
Anyone against merging attachment 120992 [details] [review] and then merging the branch to master?

I'll do it next week if noone complains.
Comment 123 Albert Astals Cid 2016-03-01 23:41:15 UTC
Pushed \o/

Now, this is not "all the work".

We still need to do the glib/qt/cpp frontend work, and that will probably mean some changes to the new code, but getting this merged is an important step.

Wonder how to proceed, this bug has 122 comments.

Should we close this one and open different ones for the frontends api?
Comment 124 Andre Guerreiro 2016-03-02 11:11:49 UTC
Thanks Albert for merging it. 
Yes it's not finished and I'm intending to pick up last summer's work on the glib frontend part.

I agree that we should close this bug and open specific ones to track the frontend development or any other issue we find with the core code.
Comment 125 Carlos Garcia Campos 2016-03-02 14:23:14 UTC
\o/ Great job guys!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.