<html> <head> <base href="https://bugs.freedesktop.org/"> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - make it possible to extract digital signature data (also in pdfsig)" href="https://bugs.freedesktop.org/show_bug.cgi?id=99271#c12">Comment # 12</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - make it possible to extract digital signature data (also in pdfsig)" href="https://bugs.freedesktop.org/show_bug.cgi?id=99271">bug 99271</a> from <a class="email" href="mailto:huj@froreich-bioscientia.de" title="Hans-Ulrich Jüttner <huj@froreich-bioscientia.de>"> Hans-Ulrich Jüttner</a> <pre>Thank you for the review. I have a few replies: (In reply to oliver.sander from <a href="show_bug.cgi?id=99271#c9">comment #9</a>) > + // 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?</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>