[Poppler-bugs] [Bug 99416] Sign PDF with digital signature
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Thu Jun 15 21:05:42 UTC 2017
https://bugs.freedesktop.org/show_bug.cgi?id=99416
--- Comment #7 from oliver.sander at tu-dresden.de ---
I tried the patch, but it doesn't apply cleanly. I suppose that's because it
depends on some incarnation of the patches in
https://bugs.freedesktop.org/show_bug.cgi?id=99271
So getting those in would help with reviewing here.
Anyway, I manually reviewed the code, and found a few (mainly cosmetic) issues.
Generally, the code would benefit from a bit of in-code documentation.
The patch does not add copyright attributions to all the files it touches.
Add your copyright line?
--- a/poppler/Form.cc
+++ b/poppler/Form.cc
@@ -455,6 +467,50 @@ Object* FormWidgetSignature::getByteRange()
return static_cast<FormFieldSignature*>(field)->getByteRange();
}
+GBool FormWidgetSignature::signDocument(const char* certNickname, const char*
digestName,
+ const char* password, const char*
reason)
+{
+ GBool ok = gFalse;
+ if (certNickname != nullptr)
Please use if (certNickname), for consistency.
+ {
+ time_t now = time(nullptr);
+ unsigned char tmp_buffer[4];
+ memcpy(tmp_buffer, "PDF", 4);
+ SignatureHandler sigHandler(certNickname,
SignatureHandler::getHashOidTag(digestName));
+ sigHandler.updateHash(tmp_buffer, 4);
+ // calculate a signature over tmp_buffer with the certificate to get it's
size
typo: it is "its size", not "it's size"
+ GooString* tmpSignature = sigHandler.signDetached(password, now);
+ if (tmpSignature != nullptr)
if (tmpSignature)
+ {
+ FormFieldSignature* signatureField =
static_cast<FormFieldSignature*>(field);
+ GooString gReason(reason != nullptr ? reason : "I approve this
document");
+ GooString gName(sigHandler.getSignerName());
+ Object vObj;
+ ok = createSignature(vObj, gName, gReason, tmpSignature, &now);
+ if (ok)
+ {
+ // calculate hash with preliminary values for ranges
+ sigHandler.restartHash();
+ prepareSignature(&sigHandler);
+ ok = updateSignature(vObj, tmpSignature);
+ }
+ if (ok)
+ {
I find this style of having consecutive blocks of "if (ok)" confusing.
You think they must all evaluate the same, until you realize that the
value of 'ok' changes in between. Personally, I'd prefer to have
if (!ok)
return gFalse;
at each location where 'ok' changes, rather than having these large
conditional blocks.
But that's subjective :-)
+ // recalculate hash with the correct ranges
+ sigHandler.restartHash();
+ prepareSignature(&sigHandler);
+ GooString* signature = sigHandler.signDetached(password, now);
+ if (signature != nullptr)
if (signature)
@@ -535,7 +591,7 @@ GooString* FormWidgetSignature::getCheckedSignature()
break;
}
}
- if (sigLen > 0 && 2*(sigLen+lenBytes) < len-4)
+ if (sigLen > 0 && 2*(sigLen+lenBytes) <= len-4)
This smells like a separate bugfix. Should this go into a separate patch?
@@ -565,6 +621,158 @@ GooString* FormWidgetSignature::getCheckedSignature()
return nullptr;
}
+GBool FormWidgetSignature::prepareSignature(SignatureHandler *handler)
+{
+ GBool ok = gFalse;
+ size_t size;
+ char* membuf;
+ FILE* mfp = open_memstream(&membuf, &size);
+ if (mfp != nullptr)
if (mfp)
+ {
+ FileOutStream* outStr = new FileOutStream(mfp, 0);
+ doc->saveAs(outStr, writeForceIncremental);
+ fclose(mfp);
+ if (membuf != nullptr)
+ {
if (membuf)
+ for (sig_start = 0; sig_start < size; ++sig_start)
+ {
The loop variable 'sig_start' is a data member, right?
Personally, I find this a bit confusing.
The following part would benefit from a bit a documentation.
+ if (strncmp(&membuf[sig_start], "/Contents <308", 14) == 0)
+ {
+ char* p = index(&membuf[sig_start+14], '>');
+ if (p != nullptr)
if (p)
+ {
+ sig_end = ++p - membuf;
+ p = strstr(&membuf[sig_end], "%%%%EOF\r\n");
+ for (file_size = sig_end; file_size < size; ++file_size)
+ {
+ if (membuf[file_size] == '%' && membuf[file_size+1] == '%' &&
+ membuf[file_size+2] == 'E' && membuf[file_size+3] == 'O' &&
+ membuf[file_size+4] == 'F' && membuf[file_size+5] == '\r' &&
+ membuf[file_size+6] == '\n')
Is there not a more C++-like way to do this check? Something like
if (std::string(membuf[file_size]) == "%%EOF\r\n") ?
Or use strncmp ?
+ if (ok)
+ {
+ ok = gFalse;
+ for (range_offset = sig_end; range_offset < file_size; ++range_offset)
+ {
+ if (strncmp(&membuf[range_offset], "/ByteRange [0 ", 14) == 0)
+ {
+ char* p = index(&membuf[range_offset+14], ']');
+ if (p != nullptr)
if (p)
+ {
+ range_size = (++p - membuf) - range_offset;
+ ok = gTrue;
+ break;
+ }
+ }
+ }
+ }
+ if (ok)
+ {
+ char* range_buf = new char[128];
Is it certain that 128 characters will always be enough?
+ Goffset end_size = file_size - sig_end;
+ Goffset s = sprintf(range_buf, "/ByteRange [0 %lld %lld %lld ]",
+ sig_start, sig_end, end_size);
+ char* buffer = new char[file_size-sig_end+64];
Maybe use end_size here instead of file_size-sig_end ?
+GBool FormWidgetSignature::createSignature(Object &vObj, const GooString&
name,
+ const GooString& reason,
+ const GooString* signature,
Why are two strings passed by-reference, and one as a pointer?
+ const time_t* sTime)
+{
+ GBool ok = obj.isDict();
+ if (ok)
+ {
+ file_size = doc->getBaseStream()->getLength();
+ Object bObj, obj1;
+ obj.dictSet("V", vObj.initDict(xref));
+ vObj.dictAdd(copyString("Type"), obj1.initName("Sig"));
+ vObj.dictAdd(copyString("Filter"), obj1.initName("Adobe.PPKLite"));
+ switch (signatureType()) {
+ case adbe_pkcs7_sha1:
+ // we don't support signing with SubFilter "adbe.pkcs7.sha1"
This will fall through. Shouldn't there be 'return gFalse' here ?
+ case adbe_pkcs7_detached:
+ vObj.dictAdd(copyString("SubFilter"),
obj1.initName("adbe.pkcs7.detached"));
+ break;
+ case ETSI_CAdES_detached:
+ vObj.dictAdd(copyString("SubFilter"),
obj1.initName("ETSI.CAdES.detached"));
+ break;
+ }
+ vObj.dictAdd(copyString("Name"), obj1.initString(name.copy()));
+ char buf[24];
+ size_t size = strftime(buf, 24, "D:%Y%m%d%H%M%S%z", localtime(sTime));
What does the following code block do?
+ if (size >= 2 && size < 22)
+ {
+ buf[size] = buf[size-1];
+ buf[size-1] = buf[size-2];
+ buf[size-2] = '\'';
+ buf[++size] = '\'';
+ buf[++size] = '\0';
+ GooString gTime(buf, size);
+ vObj.dictAdd(copyString("M"), obj1.initString(gTime.copy()));
+ }
[snip]
+GBool FormWidgetSignature::updateSignature(Object &vObj, const GooString*
signature)
+{
+ GBool ok = vObj.isDict();
Why not simply start here with
if (! vObj.isDict() || !signature)
return gFalse;
+ if (ok && signature != nullptr)
+ {
+ Object bObj, obj1;
+ vObj.dictSet("Contents", obj1.initString(signature->copy()));
+ bObj.initArray(xref);
+ bObj.arrayAdd(obj1.initInt64(0));
+ bObj.arrayAdd(obj1.initInt64(sig_start));
+ bObj.arrayAdd(obj1.initInt64(sig_end));
+ bObj.arrayAdd(obj1.initInt64(file_size-sig_end));
+ vObj.dictSet("ByteRange", &bObj);
+ obj1.free();
+ }
+ return ok;
+}
+
void FormWidgetSignature::updateWidgetAppearance()
{
// Unimplemented
@@ -1554,7 +1762,16 @@ void FormFieldSignature::parseInfo()
// check if subfilter is supported for signature validation, only detached
signatures work for now
sig_dict.dictLookup("SubFilter", &subfilterName);
- if (subfilterName.isName("adbe.pkcs7.detached") ||
subfilterName.isName("adbe.pkcs7.sha1")) {
+ if (subfilterName.isName("adbe.pkcs7.sha1")) {
+ signature_type = adbe_pkcs7_sha1;
+ signature_info->setSubFilterSupport(true);
+ }
+ if (subfilterName.isName("adbe.pkcs7.detached")) {
+ signature_type = adbe_pkcs7_detached;
+ signature_info->setSubFilterSupport(true);
+ }
+ else if (subfilterName.isName("ETSI.CAdES.detached")) {
+ signature_type = ETSI_CAdES_detached;
signature_info->setSubFilterSupport(true);
}
Does the code behave sanely if the subFilter name is something other
than these three possibilities?
--- a/poppler/Form.h
+++ b/poppler/Form.h
@@ -252,18 +258,37 @@ public:
FormWidgetSignature(PDFDoc *docA, Object *dict, unsigned num, Ref ref,
FormField *p);
void updateWidgetAppearance() override;
+ FormSignatureType signatureType();
+ void setFormSignatureType(FormSignatureType type);
SignatureInfo *validateSignature(bool doVerifyCert, bool forceRevalidation,
time_t validationTime = -1);
- Object* getByteRange();
+ Object *getByteRange();
Cosmetic change
// checks the length encoding of the signature and returns the hex encoded
signature
// if the check passed otherwise a nullptr is returned
- GooString* getCheckedSignature();
+ GooString *getCheckedSignature();
Cosmetic change
// this method only gives the correct file size if getCheckedSignature()
// has been called before
Goffset getCheckedFileSize() const { return file_size; }
+
Cosmetic change
--- a/poppler/PDFDoc.cc
+++ b/poppler/PDFDoc.cc
@@ -1215,6 +1215,14 @@ void PDFDoc::writeString (GooString* s, OutStream*
outStr, Guchar *fileKey,
outStr->printf("%c", unescaped);
}
outStr->printf(") ");
+ } else if (s->hasASN1Marker()) {
+ const char* c = s->getCString();
+ outStr->printf("<");
+ for(int i=0; i<s->getLength(); i++) {
+ unsigned char value = *(c+i)&0x000000ff;
+ outStr->printf("%2.2x", value);
+ }
+ outStr->printf("> ");
Add a brief comment explaining what the previous code block does.
--- a/poppler/SignatureHandler.cc
+++ b/poppler/SignatureHandler.cc
@@ -22,6 +22,18 @@
#include <dirent.h>
#include <Error.h>
+/* NSS headers */
+#include <secpkcs7.h>
+
+void SignatureHandler::outputCallback(void* arg, const char* buf, unsigned
long len)
+{
+ if (arg != nullptr && buf != nullptr)
if (arg && buf)
@@ -129,7 +167,7 @@ GooString
*SignatureHandler::getDefaultFirefoxCertDB_Linux()
/**
* Initialise NSS
*/
-void SignatureHandler::init_nss()
+void SignatureHandler::init_nss()
Where is the change here? Trailing whitespace?
@@ -145,11 +183,12 @@ void SignatureHandler::init_nss()
SignatureHandler::SignatureHandler(unsigned char *p7, int p7_length)
- : hash_context(NULL),
- CMSMessage(NULL),
- CMSSignedData(NULL),
- CMSSignerInfo(NULL),
- temp_certs(NULL)
+ : hash_context(nullptr),
+ CMSMessage(nullptr),
+ CMSSignedData(nullptr),
+ CMSSignerInfo(nullptr),
+ signing_cert(nullptr),
+ temp_certs(nullptr)
Use nullptr for new code, but don't replace existings NULLs.
That way it is easier to spot new code.
@@ -251,7 +316,6 @@ NSSCMSSignerInfo
*SignatureHandler::CMS_SignerInfoCreate(NSSCMSSignedData * cms_
{
NSSCMSSignerInfo *signerInfo = NSS_CMSSignedData_GetSignerInfo(cms_sig_data,
0);
if (!signerInfo) {
- printf("Error in NSS_CMSSignedData_GetSignerInfo()\n");
Removing this line is unrelated to your main objective.
If you don't like it please propose its removal in a separate patch.
@@ -342,6 +406,54 @@ SECErrorCodes SignatureHandler::validateCertificate(time_t
validation_time)
return retVal;
}
+GooString* SignatureHandler::signDetached(const char* password, time_t
signing_time)
+{
+ if (hash_context == nullptr)
if (!hash_context)
--- a/qt5/src/poppler-form.cc
+++ b/qt5/src/poppler-form.cc
@@ -182,7 +182,6 @@ Link *FormField::additionalAction(AdditionalActionType
type) const
return action;
}
-
Cosmetic change: one line removed
@@ -564,6 +563,73 @@ FormField::FormType FormFieldSignature::type() const
return FormField::FormSignature;
}
+FormFieldSignature::SignatureType FormFieldSignature::signatureType()
+{
+ SignatureType sigType = AdbePkcs7detached;
+ FormWidgetSignature* fws =
static_cast<FormWidgetSignature*>(m_formData->fm);
+ switch (fws->signatureType())
+ {
+ case adbe_pkcs7_sha1:
+ sigType = AdbePkcs7sha1;
+ break;
+ case adbe_pkcs7_detached:
+ sigType = AdbePkcs7detached;
+ break;
+ case ETSI_CAdES_detached:
+ sigType = EtsiCAdESdetached;
+ break;
+ }
Do we need to guard against the possibility that fws->signaturType() is
not one of these three types?
+ return sigType;
+}
+
+void FormFieldSignature::setSignatureType(SignatureType type)
+{
+ FormWidgetSignature* fws =
static_cast<FormWidgetSignature*>(m_formData->fm);
+ switch (type)
+ {
+ case AdbePkcs7sha1:
+ fws->setFormSignatureType(adbe_pkcs7_sha1);
+ break;
+ case AdbePkcs7detached:
+ fws->setFormSignatureType(adbe_pkcs7_detached);
+ break;
+ case EtsiCAdESdetached:
+ fws->setFormSignatureType(ETSI_CAdES_detached);
+ break;
+ }
Do we need to guard against the possibility that 'type' is
not one of these three types?
@@ -642,9 +708,10 @@ SignatureValidationInfo FormFieldSignature::validate(int
opt, const QDateTime& v
}
}
}
- if (priv->range_bounds.size() == 4)
+ GooString* checkedSignature = fws->getCheckedSignature();
+ if (priv->range_bounds.size() == 4 && checkedSignature != nullptr)
... && checkedSignature)
--- a/qt5/src/poppler-form.h
+++ b/qt5/src/poppler-form.h
@@ -487,7 +504,21 @@ namespace Poppler {
FormType type() const override;
- /**
[snip]
+
+ /**
--
You are receiving this mail because:
You are the assignee for the bug.
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/poppler-bugs/attachments/20170615/93ee29a3/attachment-0001.html>
More information about the Poppler-bugs
mailing list