Bug 99271 - make it possible to extract digital signature data (also in pdfsig)
Summary: make it possible to extract digital signature data (also in pdfsig)
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
: 102007 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-01-04 08:33 UTC by Peter Gervai
Modified: 2017-12-23 15:35 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch fixing this bug and adding support for specification of a validation time (23.28 KB, patch)
2017-05-16 09:06 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (23.31 KB, patch)
2017-05-16 10:41 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (24.13 KB, patch)
2017-05-17 12:36 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (24.13 KB, patch)
2017-05-29 14:07 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (24.37 KB, patch)
2017-06-07 12:22 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (23.42 KB, patch)
2017-06-15 09:15 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (23.42 KB, patch)
2017-06-15 09:20 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (23.25 KB, patch)
2017-06-19 10:12 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (23.59 KB, patch)
2017-06-19 13:50 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (30.29 KB, patch)
2017-06-20 13:27 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (32.00 KB, patch)
2017-06-26 10:05 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for specification of a validation time (32.09 KB, patch)
2017-06-26 11:41 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for signatures with SubFilter "ETSI.CAdES.detached" and specification of a validation time (31.95 KB, patch)
2017-06-28 11:49 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for signatures with SubFilter "ETSI.CAdES.detached" and specification of a validation time (31.39 KB, patch)
2017-07-04 07:58 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Patch fixing this bug and adding support for signatures with SubFilter "ETSI.CAdES.detached" and specification of a validation time (31.60 KB, patch)
2017-08-02 11:23 UTC, Hans-Ulrich Jüttner
Details | Splinter Review
Valgrind log of pdfsig with the file from comment 34 (37.82 KB, text/x-log)
2017-08-10 18:55 UTC, oliver.sander
Details
Patch fixing this bug and adding support for signatures with SubFilter "ETSI.CAdES.detached" and specification of a validation time (31.63 KB, patch)
2017-08-15 08:34 UTC, Hans-Ulrich Jüttner
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Gervai 2017-01-04 08:33:50 UTC
I am happy to see that `pdfsig` (and poppler) started to handle digital signatures, which is neat.

As far as I see poppler doesn't offer to get the signature data, which would be required to be able to export the signatures, which in turn would make it possible to see all the data, id fields, etc. and make external verification, for example against private CAs, check the chains, etc.

Right now `pdfsig` doesn't even tell the data, apart from CN, which is pretty thin.
Comment 1 Hans-Ulrich Jüttner 2017-05-04 12:26:03 UTC
A PDF signature specifies two ranges in the byte stream to be hashed for the signature. Usually they just exclude the digital signature itself.

Therefore, it would also be very beneficial to have an interface giving access to these ranges. In addition method telling whether the whole PDF document except for the signature itself is authenticated by the signature would be desirable.
Comment 2 Hans-Ulrich Jüttner 2017-05-16 09:06:04 UTC
Created attachment 131371 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

The proposed patch would fix this bug.

The Qt5 interface in qt5/src/poppler-form.h gets the following new methods:
QString signerSubjectDN() const;
QString getHashAlgorithmName() const;
const QByteArray& getSignature() const;
QList<qint64> signedRangeBounds() const;
bool signsTotalDocument() const;

Moreover, the interface of method validate in qt5/src/poppler-form.h is extended 
to take an optional validation time:
SignatureValidationInfo validate(int opt, const QDateTime& validationTime = QDateTime()) const;

The binary pdfsig with this patch also tells the full Distinguished Name of the
signer and the Hash Algorithm used for signing.
Comment 3 Hans-Ulrich Jüttner 2017-05-16 10:41:44 UTC
Created attachment 131373 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

Added a forgotten include statement.
Comment 4 Albert Astals Cid 2017-05-16 22:27:54 UTC
Some quick comments:
  * Don't return a QByteArray & just a QByteArray. QByteArray is implicetely shared.
  * Don't call the getters in the qt frontend getFoo, just foo
  * SignatureValidationInfoPrivate::readSignature seems like "too much code" for it to be in the qt frontend. Seems like something that should be in the core since other frontends will need to do the same too?
Comment 5 Hans-Ulrich Jüttner 2017-05-17 12:36:22 UTC
Created attachment 131387 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

Thanks for your comments, I changed the criticized points.
The code from readSignature has moved to
FormWidgetSignature::getCheckedSignature() in poppler/Form.cc.
Comment 6 Hans-Ulrich Jüttner 2017-05-29 14:07:11 UTC
Created attachment 131568 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

Made another small bugfix.
Comment 7 Hans-Ulrich Jüttner 2017-06-07 12:22:19 UTC
Created attachment 131770 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

I've added another bugfix in SignatureHandler::getSigningTime() to this patch.
Comment 8 oliver.sander 2017-06-08 15:03:57 UTC
Friendly ping!  A review of this patch would be highly appreciated.
Comment 9 oliver.sander 2017-06-14 15:08:20 UTC
I installed the patch and it builds fine.  The patched pdfsig program
seems to do something reasonable with the limited set of signed pdf
files that I have.  valgrind does not complain.

I also went through the patch line-by-line, and found only minor things.



--- a/poppler/Form.cc
+++ b/poppler/Form.cc
@@ -441,11 +442,127 @@ FormWidgetSignature::FormWidgetSignature(PDFDoc *docA, Object *aobj, unsigned nu
 	FormWidget(docA, aobj, num, ref, p)

+GooString* FormWidgetSignature::getCheckedSignature()
+{

Consider using a std::unique_ptr here, to make it clear that the method does not
retain ownership.

[snip]

+    // or (0x80 + n) for ASN1 DER definite length encoding
+    // where n is the number of following length bytes.

Can you reformulate this?  I am not sure whether I understand what
"the number of following length bytes" means.

+    if (gstr.getLength() >= end && gstr.getChar(start) == '<' && gstr.getChar(end-1) == '>')
+    {
+      ++start;
+      --end;
+      int len = end-start;

'end' and 'start' are of type Goffset.  So shouldn't 'len' be Goffset, too?

+      if (gstr.getChar(start) == '3' && gstr.getChar(start+1) == '0')
+      {
+        // ASN1 DER indefinite length encoding:
+        // We only check that all characters up to the enclosing '>'
+        // are hex characters and that there are two hex encoded 0 bytes
+        // just before the enclosing '>' marking the end of the indefinite
+        // length encoding.

This comment seems misplaced -- it doesn't appear to refer to the code that
follows it.  Testing for hex-ness happens much further below.  And I don't
readily see the 'check for two hex-encoded 0' either.

+        else if (gstr.getChar(start+2) == '8')
+        {
+          // ASN1 DER definite length encoding:
+          // We calculate the length of from the length bytes and check that

Missing word

--- a/poppler/SignatureHandler.cc
+++ b/poppler/SignatureHandler.cc
@@ -40,13 +41,51 @@ unsigned int SignatureHandler::digestLength(SECOidTag digestAlgId)

 char *SignatureHandler::getSignerName()
 {
-  if (!CMSSignerInfo)
-      return NULL;
+  if (CMSSignerInfo == nullptr)
+    return nullptr;

This is purely cosmetic.  Not sure if that is allowed.

+const char * SignatureHandler::getSignerSubjectDN()
+{
+  if (CMSSignerInfo == nullptr)
+    return nullptr;
+

if (!CMSSignerInfo) seems to be the prefered pattern in poppler.  The if (... == nullptr)
way appears various times in this patch.  Not sure whether it is okay to keep it.

+const char *SignatureHandler::getHashAlgorithmName()
+{
+  if (hash_context != nullptr && hash_context->hashobj != nullptr)
+  {
+    switch(hash_context->hashobj->type)
+    {
+      case HASH_AlgMD2:
+        return "MD2";
[snip]
+      case HASH_AlgSHA224:
+        return "SHA-224";
+      default:
+        return nullptr;

Should there be a warning when an unsupported hash algorithm is found?

@@ -54,7 +93,7 @@ time_t SignatureHandler::getSigningTime()
   if (NSS_CMSSignerInfo_GetSigningTime (CMSSignerInfo, &sTime) != SECSuccess)
     return 0;

-  return (time_t) sTime/1000000;
+  return static_cast<time_t>(sTime/1000000);

Purely cosmetic?


@@ -309,7 +353,7 @@ SignatureValidationStatus SignatureHandler::NSS_SigTranslate(NSSCMSVerificationS
     case NSSCMSVS_BadSignature:
       return SIGNATURE_INVALID;

-      case NSSCMSVS_DigestMismatch:
+    case NSSCMSVS_DigestMismatch:

Purely cosmetic?

--- a/poppler/SignatureInfo.cc
+++ b/poppler/SignatureInfo.cc
@@ -22,7 +23,9 @@ SignatureInfo::SignatureInfo()
 {
   sig_status = SIGNATURE_NOT_VERIFIED;
   cert_status = CERTIFICATE_NOT_VERIFIED;
-  signer_name = NULL;
+  signer_name = nullptr;

Cosmetic change?

@@ -31,7 +34,9 @@ SignatureInfo::SignatureInfo(SignatureValidationStatus sig_val_status, Certifica
 {
   sig_status = sig_val_status;
   cert_status = cert_val_status;
-  signer_name = NULL;
+  signer_name = nullptr;

Cosmetic change?

@@ -53,11 +58,21 @@ CertificateValidationStatus SignatureInfo::getCertificateValStatus()
   return cert_status;
 }

--- a/qt5/src/poppler-form.cc
+++ b/qt5/src/poppler-form.cc
@@ -445,14 +447,18 @@ bool FormFieldChoice::canBeSpellChecked() const


 struct SignatureValidationInfoPrivate {
-	SignatureValidationInfo::SignatureStatus signature_status;
-	SignatureValidationInfo::CertificateStatus certificate_status;
-
-	QString signer_name;
-	time_t signing_time;
+        SignatureValidationInfo::SignatureStatus signature_status;
+        SignatureValidationInfo::CertificateStatus certificate_status;

Tabs replaced by spaces here.  Intentional?

+
+        QByteArray signature;
+        QString signer_name;
+        QString signer_subject_dn;
+        QString hash_name;
+        time_t signing_time;
+        QList<qint64> range_bounds;
+        qint64 docLength;
 };

-

Cosmetic change (removed empty line)

+bool SignatureValidationInfo::signsTotalDocument() const
+{
+  Q_D(const SignatureValidationInfo);
+  if (d->range_bounds.size() == 4 && d->range_bounds.first() == 0 &&
+      d->range_bounds.value(1) >= 0 &&
+      d->range_bounds.value(2) > d->range_bounds.value(1) &&
+      d->range_bounds.value(3) >= d->range_bounds.value(2))

What does this conditional test?  Is range_bounds.first() the same as range_bounds.value(0) ?


--- a/qt5/src/poppler-form.h
+++ b/qt5/src/poppler-form.h
@@ -463,11 +492,11 @@ namespace Poppler {


 	private:
 	Q_DISABLE_COPY(FormFieldSignature)
-	};
+    };

Cosmetic change?
Comment 10 Albert Astals Cid 2017-06-14 21:22:42 UTC
Don't assign bugs to yourself, by doing so you have removed the poppler bugs list and basically made sure i won't ever realize anyone is commenting in this bug and thus never care.
Comment 11 Albert Astals Cid 2017-06-14 21:27:11 UTC
> Consider using a std::unique_ptr here, to make it clear that the method does
> not  retain ownership.

No std::unique_ptr for now please, we don't really use them in core yet.

>  {
> -  if (!CMSSignerInfo)
> -      return NULL;
> +  if (CMSSignerInfo == nullptr)
> +    return nullptr;
> 
> This is purely cosmetic.  Not sure if that is allowed.

Cosmetic changes are evil because make reviewing harder for no reason, one has to carefully look at the change to see if the logic broke or not, please refrain for any cosmetic change

> 
> +const char * SignatureHandler::getSignerSubjectDN()
> +{
> +  if (CMSSignerInfo == nullptr)
> +    return nullptr;
> +
> 
> if (!CMSSignerInfo) seems to be the prefered pattern in poppler.  The if
> (... == nullptr)
> way appears various times in this patch.  Not sure whether it is okay to
> keep it.

Yes, don't do == nullptr
Comment 12 Hans-Ulrich Jüttner 2017-06-15 08:24:50 UTC
Thank you for the review. I have a few replies:

(In reply to oliver.sander from comment #9)

> +    // or (0x80 + n) for ASN1 DER definite length encoding
> +    // where n is the number of following length bytes.
> 
> Can you reformulate this?  I am not sure whether I understand what
> "the number of following length bytes" means.

The DER encoding of an ASN1 SEQUENCE starts after the tag 0x30 either with 0x80
for indefinite length encoding or a hex value v > 0x80. In the latter case
n = v - 0x80 is the number of subsequent "length bytes" which big-endian encode
the length of the content of the SEQUENCE following the length bytes.
I'll improve on that comment.

> 
> +    if (gstr.getLength() >= end && gstr.getChar(start) == '<' &&
> gstr.getChar(end-1) == '>')
> +    {
> +      ++start;
> +      --end;
> +      int len = end-start;
> 
> 'end' and 'start' are of type Goffset.  So shouldn't 'len' be Goffset, too?
> 
> +      if (gstr.getChar(start) == '3' && gstr.getChar(start+1) == '0')
> +      {
> +        // ASN1 DER indefinite length encoding:
> +        // We only check that all characters up to the enclosing '>'
> +        // are hex characters and that there are two hex encoded 0 bytes
> +        // just before the enclosing '>' marking the end of the indefinite
> +        // length encoding.
> 
> This comment seems misplaced -- it doesn't appear to refer to the code that
> follows it.  Testing for hex-ness happens much further below.  And I don't
> readily see the 'check for two hex-encoded 0' either.

Ok, I'll move it further down.

> 
> +        else if (gstr.getChar(start+2) == '8')
> +        {
> +          // ASN1 DER definite length encoding:
> +          // We calculate the length of from the length bytes and check that
> 
> Missing word

Will be fixed.

> 
> --- a/poppler/SignatureHandler.cc
> +++ b/poppler/SignatureHandler.cc
> @@ -40,13 +41,51 @@ unsigned int SignatureHandler::digestLength(SECOidTag
> digestAlgId)
> 
>  char *SignatureHandler::getSignerName()
>  {
> -  if (!CMSSignerInfo)
> -      return NULL;
> +  if (CMSSignerInfo == nullptr)
> +    return nullptr;
> 
> This is purely cosmetic.  Not sure if that is allowed.

The usage of nullptr instead of NULL isn't purely cosmetic. Introduced with
c++11 nullptr has the advantage that the compiler type checks assignments
and comparisons with nullptr for pointer types. But I'll remove this and
other such changes from the patch for easier review.

> 
> +const char * SignatureHandler::getSignerSubjectDN()
> +{
> +  if (CMSSignerInfo == nullptr)
> +    return nullptr;
> +
> 
> if (!CMSSignerInfo) seems to be the prefered pattern in poppler.  The if
> (... == nullptr)
> way appears various times in this patch.  Not sure whether it is okay to
> keep it.
> 
> +const char *SignatureHandler::getHashAlgorithmName()
> +{
> +  if (hash_context != nullptr && hash_context->hashobj != nullptr)
> +  {
> +    switch(hash_context->hashobj->type)
> +    {
> +      case HASH_AlgMD2:
> +        return "MD2";
> [snip]
> +      case HASH_AlgSHA224:
> +        return "SHA-224";
> +      default:
> +        return nullptr;
> 
> Should there be a warning when an unsupported hash algorithm is found?

I think library code printing to the console isn't good. Exceptions aren't
used by poppler as far as I've seen yet. Therefore returning a nullptr
to me seems the best thing to do in that case.

> 
> @@ -54,7 +93,7 @@ time_t SignatureHandler::getSigningTime()
>    if (NSS_CMSSignerInfo_GetSigningTime (CMSSignerInfo, &sTime) !=
> SECSuccess)
>      return 0;
> 
> -  return (time_t) sTime/1000000;
> +  return static_cast<time_t>(sTime/1000000);
> 
> Purely cosmetic?

No, definitely not! This fixes an integer overrun. The old code casts
sTime before dividing by 1000000. On 32-bit systems this leads to wrong
times returned.

> 
> 
> @@ -309,7 +353,7 @@ SignatureValidationStatus
> SignatureHandler::NSS_SigTranslate(NSSCMSVerificationS
>      case NSSCMSVS_BadSignature:
>        return SIGNATURE_INVALID;
> 
> -      case NSSCMSVS_DigestMismatch:
> +    case NSSCMSVS_DigestMismatch:
> 
> Purely cosmetic?
> 
> --- a/poppler/SignatureInfo.cc
> +++ b/poppler/SignatureInfo.cc
> @@ -22,7 +23,9 @@ SignatureInfo::SignatureInfo()
>  {
>    sig_status = SIGNATURE_NOT_VERIFIED;
>    cert_status = CERTIFICATE_NOT_VERIFIED;
> -  signer_name = NULL;
> +  signer_name = nullptr;
> 
> Cosmetic change?
> 
> @@ -31,7 +34,9 @@ SignatureInfo::SignatureInfo(SignatureValidationStatus
> sig_val_status, Certifica
>  {
>    sig_status = sig_val_status;
>    cert_status = cert_val_status;
> -  signer_name = NULL;
> +  signer_name = nullptr;
> 
> Cosmetic change?
> 
> @@ -53,11 +58,21 @@ CertificateValidationStatus
> SignatureInfo::getCertificateValStatus()
>    return cert_status;
>  }
> 
> --- a/qt5/src/poppler-form.cc
> +++ b/qt5/src/poppler-form.cc
> @@ -445,14 +447,18 @@ bool FormFieldChoice::canBeSpellChecked() const
> 
> 
>  struct SignatureValidationInfoPrivate {
> -	SignatureValidationInfo::SignatureStatus signature_status;
> -	SignatureValidationInfo::CertificateStatus certificate_status;
> -
> -	QString signer_name;
> -	time_t signing_time;
> +        SignatureValidationInfo::SignatureStatus signature_status;
> +        SignatureValidationInfo::CertificateStatus certificate_status;
> 
> Tabs replaced by spaces here.  Intentional?
> 
> +
> +        QByteArray signature;
> +        QString signer_name;
> +        QString signer_subject_dn;
> +        QString hash_name;
> +        time_t signing_time;
> +        QList<qint64> range_bounds;
> +        qint64 docLength;
>  };
> 
> -
> 
> Cosmetic change (removed empty line)
> 
> +bool SignatureValidationInfo::signsTotalDocument() const
> +{
> +  Q_D(const SignatureValidationInfo);
> +  if (d->range_bounds.size() == 4 && d->range_bounds.first() == 0 &&
> +      d->range_bounds.value(1) >= 0 &&
> +      d->range_bounds.value(2) > d->range_bounds.value(1) &&
> +      d->range_bounds.value(3) >= d->range_bounds.value(2))
> 
> What does this conditional test?  Is range_bounds.first() the same as
> range_bounds.value(0) ?

It checks that these bounds are for two ranges the first starting at 0
and the second after the end of the first. I'll change range_bound.first()
to the equivalent range_bounds.value(0).

> 
> 
> --- a/qt5/src/poppler-form.h
> +++ b/qt5/src/poppler-form.h
> @@ -463,11 +492,11 @@ namespace Poppler {
> 
> 
>  	private:
>  	Q_DISABLE_COPY(FormFieldSignature)
> -	};
> +    };
> 
> Cosmetic change?
Comment 13 Hans-Ulrich Jüttner 2017-06-15 09:15:18 UTC
Created attachment 131972 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

Some changes requested by the review of Oliver Sander.
Comment 14 Hans-Ulrich Jüttner 2017-06-15 09:20:40 UTC
Created attachment 131973 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

And another change requested by the review which I overlooked at first.
Comment 15 oliver.sander 2017-06-15 13:36:10 UTC
Thanks for the new patch.  Three of my issues remain.  Albert, what do you say?

+    if (gstr.getLength() >= end && gstr.getChar(start) == '<' && gstr.getChar(end-1) == '>')
+    {
+      ++start;
+      --end;
+      int len = end-start;

'end' and 'start' are of type Goffset.  So shouldn't 'len' be Goffset, too?

+const char *SignatureHandler::getHashAlgorithmName()
+{
+  if (hash_context != nullptr && hash_context->hashobj != nullptr)
+  {
+    switch(hash_context->hashobj->type)
+    {
+      case HASH_AlgMD2:
+        return "MD2";
[snip]
+      case HASH_AlgSHA224:
+        return "SHA-224";
+      default:
+        return nullptr;

Should there be a warning when an unsupported hash algorithm is found?
(Albert, you did say that this was an issue for you, but what is the proposed solution?)

 struct SignatureValidationInfoPrivate {
-	SignatureValidationInfo::SignatureStatus signature_status;
-	SignatureValidationInfo::CertificateStatus certificate_status;
-
-	QString signer_name;
-	time_t signing_time;
+        SignatureValidationInfo::SignatureStatus signature_status;
+        SignatureValidationInfo::CertificateStatus certificate_status;

Tabs replaced by spaces here.  Intentional?
Comment 16 Hans-Ulrich Jüttner 2017-06-19 10:12:44 UTC
Created attachment 132053 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

I changed the types of 'start' and 'end' to int as they are put into
GooString::getChar() which takes an int and would anyway produce an integer
overflow for values >= 2GB. I think that should fix the complaint about the
differing type of 'len'.

I looked for a solution of presenting a warning on invalid hash algorithm types
in method SignatureHandler::getHashAlgorithmName(). I found out that code using
poppler may call setErrorCallback() to prevent method error() from printing to
the console. So I think it's fine to use method error() for presenting the
warning.

Although I dislike tabs in code I now use them in the declaration of
struct SignatureValidationInfoPrivate in order to not replace the tabs
which were already there.

I also added a bugfix from the patch #99416 to method FormWidgetSignature::getCheckedSignature() here:
-            if (sigLen > 0 && 2*(sigLen+lenBytes) < len-4)
+            if (sigLen > 0 && 2*(sigLen+lenBytes) <= len-4)
As Oliver Sander pointed out in a review to that patch, it does not belong
there. It definitely belongs here.

I hope you will be satisfied with the new patch.
Comment 17 Adrian Johnson 2017-06-19 11:22:19 UTC
(In reply to Hans-Ulrich Jüttner from comment #16)
> Created attachment 132053 [details] [review] [review]
> Patch fixing this bug and adding support for specification of a validation
> time
> 
> I changed the types of 'start' and 'end' to int as they are put into
> GooString::getChar() which takes an int and would anyway produce an integer
> overflow for values >= 2GB. I think that should fix the complaint about the
> differing type of 'len'.

The start and end needs to be of type Goffset to support PDF files > 2GB.

+  if (end >= start+6)
+  {
+    BaseStream* stream = doc->getBaseStream();
+    file_size = stream->getLength();
+    GooString gstr;
+    stream->fillGooString(&gstr);

You're reading the entire PDF file into a GooString! Use stream->setPos(start) to seek to the start of the signature then read what you need into a GooString. Is there an upper limit to the signature size? You should set some limit on the amount of data read into a GooString. Or use the stream operators (setPos(), getChar(), lookChar() etc) to read directly from the stream.

It would also be useful to make pdfsig to print the ranges and signsTotalDocument info.
Comment 18 Hans-Ulrich Jüttner 2017-06-19 12:21:17 UTC
(In reply to Adrian Johnson from comment #17)
> (In reply to Hans-Ulrich Jüttner from comment #16)
> > Created attachment 132053 [details] [review] [review] [review]
> > Patch fixing this bug and adding support for specification of a validation
> > time
> > 
> > I changed the types of 'start' and 'end' to int as they are put into
> > GooString::getChar() which takes an int and would anyway produce an integer
> > overflow for values >= 2GB. I think that should fix the complaint about the
> > differing type of 'len'.
> 
> The start and end needs to be of type Goffset to support PDF files > 2GB.
> 
> +  if (end >= start+6)
> +  {
> +    BaseStream* stream = doc->getBaseStream();
> +    file_size = stream->getLength();
> +    GooString gstr;
> +    stream->fillGooString(&gstr);
> 
> You're reading the entire PDF file into a GooString! Use
> stream->setPos(start) to seek to the start of the signature then read what
> you need into a GooString. Is there an upper limit to the signature size?
> You should set some limit on the amount of data read into a GooString. Or
> use the stream operators (setPos(), getChar(), lookChar() etc) to read
> directly from the stream.
> 

Ok, but then I can't use fillGoString() anymore as it does a reset()
internally. I'll rewrite method getCheckedSignature to use a buffer and
doGetChars() instead.

> It would also be useful to make pdfsig to print the ranges and
> signsTotalDocument info.

signsTotalDocument() is currently only in the qt5 interface which pdfsig
doesn't use. I'll think about adding a similar method to poppler/Form.cc.
Comment 19 Hans-Ulrich Jüttner 2017-06-19 13:50:34 UTC
Created attachment 132058 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

Added support for PDF files > 2GB in method getCheckedSignatue().
The proposed enhancement of pdfsig would duplicate methods from qt5 interface.
I finally think it should be done on another Bugzilla issue.
Comment 20 Albert Astals Cid 2017-06-19 22:09:50 UTC
Some comments:
 * Please provide a commit log
 * The code is full of memory leaks, every time you "use" an Object, you need to free() it, yes the API is awful, maybe you just want to wait until i merge my better_object branch so that you don't have to worry about it
 * Please move the getHashAlgorithmName function to pdfsig, it is something that i guess some people would like to translate (CJK?, Arabic?) and we don't provide any translations support on the poppler core, so better not to have things that people may want to show on a UI on poppler core either
 * Don't break the ABI on the Qt side of things, just introduce another function
 * The extra code in FormFieldSignature::validate seems like it really should be in the core since other bindings will want to do the same?
Comment 21 Hans-Ulrich Jüttner 2017-06-20 13:27:41 UTC
Created attachment 132087 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

New patch regarding the criticisms of Albert Astal Cid.

I checked pdfsig with several signed PDF files with valgrind and
it reported 0 bytes lost.

Moving the extra code in FormFieldSignature::validate() from the Qt5 interface
to FormWidgetSignature::getSignedRangeBounds() in the poppler core opened the
opportunity to satisfy the request of Adrian Johnson to print the ranges and
signsTotalDocument info in pdfsig.
Comment 22 Albert Astals Cid 2017-06-25 22:11:27 UTC
You're still breaking the ABI of the qt5 code, you can't do this

-	SignatureValidationInfo validate(ValidateOptions opt) const;
+	SignatureValidationInfo validate(int opt, const QDateTime& validationTime = QDateTime()) const;

just add the new function.

And you brought the problem of untranslatable strings up to the qt5 code, that doesn't fix anything, please remove it, anyone that wants to show the name of the hash algorithm will have to provide the translations themselves.
Comment 23 Hans-Ulrich Jüttner 2017-06-26 10:05:48 UTC
Created attachment 132247 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

I changed these points in the new patch.
Comment 24 Hans-Ulrich Jüttner 2017-06-26 11:41:43 UTC
Created attachment 132250 [details] [review]
Patch fixing this bug and adding support for specification of a validation time

I also fixed a memory leak in this patch.
Now valgrind for pdfsig reports 0 bytes lost.
Comment 25 Albert Astals Cid 2017-06-26 22:30:59 UTC
Should

+  if (subfilterName.isName("adbe.pkcs7.sha1")) {
+    signature_type = adbe_pkcs7_sha1;
+    signature_info->setSubFilterSupport(true);
+  }
+  if (subfilterName.isName("adbe.pkcs7.detached")) {

be an else if?



Also can you explain getSignedRangeBounds vs getCheckedSignature ? In one you assume an indefinite number of ranges in getByteRange but the other one only works if there's exactly 2 ranges (4 elements in getByteRange). Why is that?
Comment 26 Hans-Ulrich Jüttner 2017-06-27 07:32:34 UTC
(In reply to Albert Astals Cid from comment #25)
> Should
> 
> +  if (subfilterName.isName("adbe.pkcs7.sha1")) {
> +    signature_type = adbe_pkcs7_sha1;
> +    signature_info->setSubFilterSupport(true);
> +  }
> +  if (subfilterName.isName("adbe.pkcs7.detached")) {
> 
> be an else if?
> 

Yes, of course.

> 
> 
> Also can you explain getSignedRangeBounds vs getCheckedSignature ? In one
> you assume an indefinite number of ranges in getByteRange but the other one
> only works if there's exactly 2 ranges (4 elements in getByteRange). Why is
> that?

A document from Adobe
https://www.adobe.com/devnet-docs/acrobatetk/tools/DigSig/Acrobat_DigitalSignatures_in_PDF.pdf
describes a signature in PDF files to have exactly four numbers defining
2 ranges:

"ByteRange is an array of four numbers. The first number in each pair is the
offset in the file (from the beginning, starting from 0) of the beginning of a
stream of bytes to be included in the hash. The second number is the length of
that stream. The two pairs define two sequences of bytes that define what is to
be hashed. The actual signature value is stored in the /Contents key between
the end of the first sequence and the beginning of the second one." (Page 5)

I abided to this in getCheckedSignature() as the name of this function indicates
that the signature has been checked according to the standard. On the other hand
in getByteRange I wanted to return what actually is in the PDF document. Because
of the definition of the byte range as containing pairs of offset and length,
I had to assume the number to be even. But we can be more restrictive here too,
returning an empty list if there aren't exactly four elements in the byte range.

What do you think?
Comment 27 Albert Astals Cid 2017-06-27 23:03:55 UTC
> What do you think?

I don't really see a need of getSignedRangeBounds supporting broken documents, i think we should enforce the "it has to be 4 numbers" at that level and not check it on the callers (it seems you do it at least in 3 different places).

Also i think it's better if you save yourself some trouble and make getSignedRangeBounds return a std::list<Goffset>, we don't use std:: stuff much but in this case using a GooList is making your life harder for no reason.
Comment 28 Hans-Ulrich Jüttner 2017-06-28 11:49:43 UTC
Created attachment 132301 [details] [review]
Patch fixing this bug and adding support for signatures with SubFilter "ETSI.CAdES.detached" and specification of a validation time

New patch following to the last comments.
Comment 29 Albert Astals Cid 2017-06-30 22:28:22 UTC
I realize it should be a vector more than a list, but i'll change it locally when commiting, no need to commit, do i understand that the beginning of getCheckedSignature can be replaced with 

  Goffset start = 0;
  Goffset end = 0;
  const std::vector<Goffset> ranges = getSignedRangeBounds();
  if (ranges.size() > 2)
  {
    start = ranges[1];
    end = ranges[2];
  }
?
Comment 30 Hans-Ulrich Jüttner 2017-07-03 07:14:09 UTC
(In reply to Albert Astals Cid from comment #29)
> I realize it should be a vector more than a list, but i'll change it locally
> when commiting, no need to commit, do i understand that the beginning of
> getCheckedSignature can be replaced with 
> 
>   Goffset start = 0;
>   Goffset end = 0;
>   const std::vector<Goffset> ranges = getSignedRangeBounds();
>   if (ranges.size() > 2)
>   {
>     start = ranges[1];
>     end = ranges[2];
>   }
> ?

Yes, a std::vector would be handier for getSignedRangeBounds().
I chose a std::list only because of your suggestion.
Comment 31 Hans-Ulrich Jüttner 2017-07-04 07:58:10 UTC
Created attachment 132423 [details] [review]
Patch fixing this bug and adding support for signatures with SubFilter "ETSI.CAdES.detached" and specification of a validation time

New patch, putting into practice the last change request.
Comment 32 oliver.sander 2017-07-13 07:06:51 UTC
Friendly ping.  Can this be merged now?
Comment 33 Albert Astals Cid 2017-07-13 17:02:31 UTC
I won't have much time between 1-2 weeks since the KDE Yearly conference is coming and i'll be there doing things in person so that leaves not much time for internet things. I'll try to squeeze this in but can't promise much.
Comment 34 Gustavo 2017-07-30 07:32:12 UTC
Hi,

I just tried the latest patch version against the trunk and it compiles fine. 
I tried pdfsig with different PDF files on my end and it works as expected EXCEPT on one signed file and it crashes (coredump).

Maybe Hans-Ulrich could take a look at it using my file:

http://bit.ly/2u9AjQI

pdfsig does not crash without the patch, it just says 

"Unimplemented Feature (0): Unable to validate this type of signature".

Patched pdfsig says:

"Signature #1:
  - Signer Certificate Common Name: XXXXXXXXXXXXXX
Segmentation fault (core dumped)"

It does get the Common Name correctly.

Cheers
Comment 35 Hans-Ulrich Jüttner 2017-07-31 07:57:25 UTC
(In reply to Gustavo from comment #34)
> Hi,
> 
> I just tried the latest patch version against the trunk and it compiles
> fine. 
> I tried pdfsig with different PDF files on my end and it works as expected
> EXCEPT on one signed file and it crashes (coredump).
> 
> Maybe Hans-Ulrich could take a look at it using my file:
> 
> http://bit.ly/2u9AjQI

I tried pdfsig with the patch and your file and it didn't crash.
It says:

Digital Signature Info of: pdfsig_error_coredump.pdf
Signature #1:
  - Signer Certificate Common Name: MICHAEL ANDREY VARGAS BARRANTES (FIRMA)
  - Signer full Distinguished Name: CN=MICHAEL ANDREY VARGAS BARRANTES (FIRMA),OU=CIUDADANO,O=PERSONA FISICA,C=CR,givenName=MICHAEL ANDREY,SN=VARGAS BARRANTES,serialNumber=CPF-03-0394-0670
  - Signing Time: Jul 04 2017 21:33:17
  - Signing Hash Algorithm: SHA-256
  - Signature Type: ETSI.CAdES.detached
  - Signed Ranges: [0 - 122708], [166258 - 171742]
  - Not total document signed
  - Signature Validation: Signature is Valid.
  - Certificate Validation: Certificate issuer isn't Trusted.

However, the "- Not total document signed" part is a bug.
I'll fix this with a revised patch.

> 
> pdfsig does not crash without the patch, it just says 
> 
> "Unimplemented Feature (0): Unable to validate this type of signature".
> 
> Patched pdfsig says:
> 
> "Signature #1:
>   - Signer Certificate Common Name: XXXXXXXXXXXXXX
> Segmentation fault (core dumped)"
> 
> It does get the Common Name correctly.
> 
> Cheers

Concerning your crash, have you pulled the latest state of the master branch
before applying the patch? What processor architecture and system (32-bit or
64-bit) are you working on? I tried pdfsig on an i386 (32-bit) system and had
no crash as pointed out above.

Best regards
Hans-Ulrich
Comment 36 Hans-Ulrich Jüttner 2017-08-02 11:23:52 UTC
Created attachment 133194 [details] [review]
Patch fixing this bug and adding support for signatures with SubFilter "ETSI.CAdES.detached" and specification of a validation time

New patch adapted to changes from the merged better_object branch.
I also fixed the bug in pdfsig which reported "Not total document signed"
when actually the total document was signed.
Comment 37 Albert Astals Cid 2017-08-03 19:05:22 UTC
*** Bug 102007 has been marked as a duplicate of this bug. ***
Comment 38 oliver.sander 2017-08-10 18:54:44 UTC
I tried to reproduce the crash with the file from comment 34, but I couldn't.  pdfsig appears to do what it is supposed to do.

However, valgrind complains about a few things.  Not sure whether that means bugs in poppler, or bugs in libnss, though.  I'll attach the log.
Comment 39 oliver.sander 2017-08-10 18:55:31 UTC
Created attachment 133427 [details]
Valgrind log of pdfsig with the file from comment 34
Comment 40 Hans-Ulrich Jüttner 2017-08-14 10:17:42 UTC
(In reply to oliver.sander from comment #38)
> I tried to reproduce the crash with the file from comment 34, but I
> couldn't.  pdfsig appears to do what it is supposed to do.
> 
> However, valgrind complains about a few things.  Not sure whether that means
> bugs in poppler, or bugs in libnss, though.  I'll attach the log.

As this is in the destructor, which as well as the constructor hasn't been
touched by the patch, I would say this is a problem valgrind has with libnss3.

However, in my version on a 32-bit system the problem doesn't show up.
Maybe you could check pdfsig from the unpatched master branch with valgrind
and a signed file which pdfsig from the master branch can handle
(e.g. https://kbdeveloper.qoppa.com/wp-content/uploads/blank_signed.pdf).
Comment 41 oliver.sander 2017-08-14 18:23:42 UTC
I tried that, and indeed I get pretty much the same valgrind errors.  So the cause is not your patch but something in libnss.

My platform is a 64bit Debian system.
Comment 42 Hans-Ulrich Jüttner 2017-08-15 08:34:46 UTC
Created attachment 133523 [details] [review]
Patch fixing this bug and adding support for signatures with SubFilter "ETSI.CAdES.detached" and specification of a validation time

Adapted the patch to apply cleanly on the latest changes in the master branch.
Comment 43 Albert Astals Cid 2017-08-15 10:57:49 UTC
pushed, alongside with some fixes of my own https://cgit.freedesktop.org/poppler/poppler/commit/?id=b56a697c58bcf09063827b9c109be9c04a033b8a
Comment 44 Gustavo 2017-12-23 06:26:23 UTC
Hi,

I just downloaded the 0.62 release, compiled it in a ubuntu 16.04 64 bit updated.

I keep getting a coredump when trying to get the signature out of a PDF file with pdfsig.

But I noticed that sometimes, with some files, if I call "pdfsig myfile.pdf" it works, but if a call "/usr/bin/pdfsig myfile.pdf" it crashes. If I run "which pdfsig" I get "/usr/bin/pdfsig" which means both calls should be identical.

With other files, "/usr/bin/pdfsig" works, but "pdfsig" crashes.

And in some other cases both crash.

Any ideas?
Comment 45 Gustavo 2017-12-23 07:16:57 UTC
I just tried again, in a ubuntu 16.04 32bit, downloaded 0.62.0, and everything works fine.

So I think it is related to the 64bit scenario.

Any ideas?
Comment 46 Jason Crain 2017-12-23 15:35:01 UTC
(In reply to Gustavo from comment #45)
> I just tried again, in a ubuntu 16.04 32bit, downloaded 0.62.0, and
> everything works fine.
> 
> So I think it is related to the 64bit scenario.
> 
> Any ideas?

Hard to say because it could be a bug in poppler or it could be you installed it wrong. You mentioning "/usr/bin/pdfsig" makes me think it's installed wrong because "/usr/local/bin/pdfsig" would be a more typical path for locally compiled software.

To debug it, we'll need a stack trace, any error messages you are getting, and attach a PDF that makes it crash. And put it in a new bug report instead of one that was closed months ago.


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