ubuntu pdf doc viewer will not let me sign a document

[leonard janetzke]

Just updated in last few weeks, i think ubuntu 12.4

To sign the document i have to send it to my neighbors windows computer, open it, sign it, then send it, then I get a note from echo sign that the document was sent with my signature.

ProblemType: Bug
DistroRelease: Ubuntu 11.10
Package: evince 3.2.1-0ubuntu2.3
ProcVersionSignature: Ubuntu 3.0.0-27.44-generic 3.0.45
Uname: Linux 3.0.0-27-generic i686
ApportVersion: 1.23-0ubuntu4
Architecture: i386
Date: Fri Nov 30 18:13:25 2012
ExecutablePath: /usr/bin/evince
InstallationMedia: Ubuntu 11.04 "Natty Narwhal" - Release i386 (20110427.1)
ProcEnviron:
 PATH=(custom, no user)
 LANG=en_US.UTF-8
 SHELL=/bin/bash
SourcePackage: evince
UpgradeStatus: Upgraded to oneiric on 2012-11-18 (12 days ago)

[Rastos (rastos)]

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

[Carlos Garcia Campos (carlosgc)]

*** Bug 19120 has been marked as a duplicate of this bug. ***

[Advax (advax)]

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

[Markus Kilås (markuskilas)]

Created attachment 37425
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
<email address hidden>

[Markus Kilås (markuskilas)]

Sample signed document:
http://wwwpriv.primekey.se/~markus/pdfsigner/SignServer3.1.3-signed.pdf

[Albert Astals Cid (aacid)]

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.

[Brad Hards (bradh)]

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.

[Brad Hards (bradh)]

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.

[Markus Kilås (markuskilas)]

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...

[Albert Astals Cid (aacid)]

Brad, you are the "expert" here, you think it makes sense commiting the patch (maybe fixing the style?) ?

[Albert Astals Cid (aacid)]

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

[Cmorve69-w (cmorve69-w)]

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

[Jelle de Jong (jelledejong)]

Any progress or possible ETA for digital signature support for PDF documents?

[iroli (roland-lezuo)]

bump

[nodata (ubuntu-nodata)]

Since there hasn't been any progress for a couple of years, would it be possible to close this bug?

[sjjh (simon-harhues)]

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?

[André Guerreiro (aguerreiro)]

Created attachment 66786
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.

[Albert Astals Cid (aacid)]

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?

[Josh Triplett (joshtriplett)]

Since the patch provided supports verifying signatures, any thoughts on adding support for creating signatures, ideally in a manner compatible with Adobe's implementation?

[André Guerreiro (aguerreiro)]

@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.

[André Guerreiro (aguerreiro)]

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

[Markus Kilås (markuskilas)]

(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.

[André Guerreiro (aguerreiro)]

(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

[Albert Astals Cid (aacid)]

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.

[Albert Astals Cid (aacid)]

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.

[Albert Astals Cid (aacid)]

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

[André Guerreiro (aguerreiro)]

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.

[André Guerreiro (aguerreiro)]

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).

[Chpe (chpe)]

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.

[André Guerreiro (aguerreiro)]

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.

[Carlos Garcia Campos (carlosgc)]

(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?

[Albert Astals Cid (aacid)]

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?

[Carlos Garcia Campos (carlosgc)]

(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

[André Guerreiro (aguerreiro)]

OK, NSS with shared DB is what I'll pursue from now on.
Thanks everyone for the input.

[Weibo3721 (weibo3721)]

Comment on attachment 37425
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...

[Albert Astals Cid (aacid)]

weibo?

[José Aliste (jose-aliste)]

(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

[Sebastien Bacher (seb128)]

Thank you for your bug report, that's a known issue upstream in poppler and evince and work is ongoing to add signature support:
https://bugs.freedesktop.org/show_bug.cgi?id=16770
https://bugzilla.gnome.org/show_bug.cgi?id=614929

[Sebastien Bacher (seb128)]

see also bug #740506 about being able to check a signature

[Markus Kilås (markuskilas)]

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
---

[Markus Kilås (markuskilas)]

(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

[Adrian Johnson (ajohnson-redneon)]

(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.

[Markus Kilås (markuskilas)]

(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.

[Adrian Johnson (ajohnson-redneon)]

What is the output of "pkg-config --cflags nss"?

[Markus Kilås (markuskilas)]

(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

[Markus Kilås (markuskilas)]

Created attachment 119174
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".

[Adrian Johnson (ajohnson-redneon)]

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.

[Albert Astals Cid (aacid)]

(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)

[Albert Astals Cid (aacid)]

(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.

[André Guerreiro (aguerreiro)]

(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.

[Adrian Johnson (ajohnson-redneon)]

(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.

[Adrian Johnson (ajohnson-redneon)]

(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

[André Guerreiro (aguerreiro)]

Created attachment 119283
Manpage improvement

Here's an improvement to the manpage.
Corrected a typo and added some missing context

[Adrian Johnson (ajohnson-redneon)]

(In reply to Andre Guerreiro from comment #100)
> Created attachment 119283 [details] [review]
> Manpage improvement

Pushed

[Albert Astals Cid (aacid)]

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?

[Markus Kilås (markuskilas)]

(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?

[xormar (public-wernig)]

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.

[André Guerreiro (aguerreiro)]

Created attachment 120434
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()

[Markus Kilås (markuskilas)]

Created attachment 120758
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.

[Markus Kilås (markuskilas)]

(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 (?).

[Markus Kilås (markuskilas)]

Created attachment 120760
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.

[Albert Astals Cid (aacid)]

(In reply to Markus Kilås from comment #106)
> 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.

Pushed

[Albert Astals Cid (aacid)]

(In reply to Markus Kilås from comment #108)
> 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.

Adrian what do you think of this patch?

[Adrian Johnson (ajohnson-redneon)]

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.

[Adrian Johnson (ajohnson-redneon)]

Created attachment 120889
Check in ranges in ByteRange array

This is an alternative to the patch in comment 108. It checks all ranges in the ByteRange array.

[Adrian Johnson (ajohnson-redneon)]

(In reply to Andre Guerreiro from comment #105)
> 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()

Pushed

[Albert Astals Cid (aacid)]

(In reply to Adrian Johnson from comment #112)
> 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.

I'm not very convinced by the naming of
  GBool isInteger() { return type == objInt || type == objInt64; }
what do you think of
  isIntOrInt64() ?

[Adrian Johnson (ajohnson-redneon)]

(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.

[Adrian Johnson (ajohnson-redneon)]

Created attachment 120924
Check in ranges in ByteRange array v2

- renamed isInteger() to isIntOrInt64()
- added and use getIntOrInt64()

[André Guerreiro (aguerreiro)]

Created attachment 120992
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.

[Albert Astals Cid (aacid)]

Andre and Andre what do you think of Adrian's patch?

[André Guerreiro (aguerreiro)]

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.

[Albert Astals Cid (aacid)]

Ok, good, so Adrian can you commit your patch?

After that we have attachment 120992 "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?

[Adrian Johnson (ajohnson-redneon)]

(In reply to Albert Astals Cid from comment #120)
> Ok, good, so Adrian can you commit your patch?

Pushed

[Albert Astals Cid (aacid)]

Anyone against merging attachment 120992 and then merging the branch to master?

I'll do it next week if noone complains.

[Albert Astals Cid (aacid)]

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?

[André Guerreiro (aguerreiro)]

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.

[Carlos Garcia Campos (carlosgc)]

\o/ Great job guys!

[Albert Astals Cid (aacid)]

I've opened these bugs

https://bugs.freedesktop.org/show_bug.cgi?id=94376
https://bugs.freedesktop.org/show_bug.cgi?id=94377
https://bugs.freedesktop.org/show_bug.cgi?id=94378

Closing this one