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.
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.
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.
Created attachment 131373 [details] [review] Patch fixing this bug and adding support for specification of a validation time Added a forgotten include statement.
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?
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.
Created attachment 131568 [details] [review] Patch fixing this bug and adding support for specification of a validation time Made another small bugfix.
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.
Friendly ping! A review of this patch would be highly appreciated.
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?
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.
> 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
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?
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.
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.
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?
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.
(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.
(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.
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.
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?
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.
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.
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.
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.
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?
(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?
> 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.
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.
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]; } ?
(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.
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.
Friendly ping. Can this be merged now?
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.
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
(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
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.
*** Bug 102007 has been marked as a duplicate of this bug. ***
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.
Created attachment 133427 [details] Valgrind log of pdfsig with the file from comment 34
(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).
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.
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.
pushed, alongside with some fixes of my own https://cgit.freedesktop.org/poppler/poppler/commit/?id=b56a697c58bcf09063827b9c109be9c04a033b8a
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?
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?
(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.