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

[Nelson Benitez (gnel)]

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.

[Markus Kilås (markuskilas)]

(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

[2o6p-tomas-8c8g (2o6p-tomas-8c8g)]

(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

[Andreas Bürki (abuerki)]

(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

[Markus Kilås (markuskilas)]

There is now a new sponsor. Bounty is up to US $300.
I hope there will be some more progress here soon.

[xormar (public-wernig)]

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.

[Ralph-freedesktop (ralph-freedesktop)]

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?

[Albert Astals Cid (aacid)]

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.

[André Guerreiro (aguerreiro)]

Created attachment 117885
PDF signature verification using NSS

[André Guerreiro (aguerreiro)]

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)

[Albert Astals Cid (aacid)]

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

[Bepandre (bepandre)]

Created attachment 118027
memory leaks fixed

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

thoughts?

[André Guerreiro (aguerreiro)]

Created attachment 118036
poppler nss signature support - v3

[André Guerreiro (aguerreiro)]

Sorry for the succession of patches. This one fixes some remaining leaks in the new PDFDoc methods and improves the indentation

[Albert Astals Cid (aacid)]

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?

[Bepandre (bepandre)]

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

[Albert Astals Cid (aacid)]

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?

[André Guerreiro (aguerreiro)]

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

[André Guerreiro (aguerreiro)]

Created attachment 118195
poppler nss signature support - v6

Sorry, there were still some missing NULL checks and a useless new(), here's a new one.

[Albert Astals Cid (aacid)]

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?

[Adam Reichold (adamreichold)]

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.

[André Guerreiro (aguerreiro)]

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

[Mantas Mikulėnas (grawity)]

Hmm, why does this search for Firefox's profile – why not use the shared nssdb (sql:~/.pki/nssdb) like Evolution and Chromium do?

[Adrian Johnson (ajohnson-redneon)]

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?

[Albert Astals Cid (aacid)]

Andre*2, answers to the comments from Adam, Adrian and Mantas?

[André Guerreiro (aguerreiro)]

Created attachment 118398
Fix for Buffer overflow

Regarding the illegal ByteRange values which would cause overflow this patch should fix it

[André Guerreiro (aguerreiro)]

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

[Adrian Johnson (ajohnson-redneon)]

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

[Albert Astals Cid (aacid)]

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?

[André Guerreiro (aguerreiro)]

Created attachment 118446
NSS conditional build

This patch makes the NSS dependency optional in the CMake and Autotools build systems.

[André Guerreiro (aguerreiro)]

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

[Albert Astals Cid (aacid)]

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

I have a few adbe.x509.rsa_sha1 in case you need them for testing.

[Albert Astals Cid (aacid)]

(In reply to Andre Guerreiro from comment #66)
> Created attachment 118446 [details] [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

[Albert Astals Cid (aacid)]

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?

[Adrian Johnson (ajohnson-redneon)]

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

[Albert Astals Cid (aacid)]

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

[Adrian Johnson (ajohnson-redneon)]

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

[Albert Astals Cid (aacid)]

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

[Adrian Johnson (ajohnson-redneon)]

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

[André Guerreiro (aguerreiro)]

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

[Adam Reichold (adamreichold)]

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

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.

[Adrian Johnson (ajohnson-redneon)]

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

[Adrian Johnson (ajohnson-redneon)]

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

[Adrian Johnson (ajohnson-redneon)]

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

[Adam Reichold (adamreichold)]

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

[Albert Astals Cid (aacid)]

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

[Albert Astals Cid (aacid)]

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

[Albert Astals Cid (aacid)]

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

I've pushed this, Adrian, Adam please suggest your improvements over it with a patch if you reached an agreement.

[Adrian Johnson (ajohnson-redneon)]

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

[Ralph-freedesktop (ralph-freedesktop)]

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

[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