Bug 94376 - Implement digital signature support (glib frontend)
Summary: Implement digital signature support (glib frontend)
Status: RESOLVED MOVED
Alias: None
Product: poppler
Classification: Unclassified
Component: glib frontend (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-02 22:43 UTC by Albert Astals Cid
Modified: 2018-10-11 09:08 UTC (History)
8 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Expose signature verification in the glib frontend (12.25 KB, patch)
2016-06-20 16:08 UTC, Andre Guerreiro
Details | Splinter Review
Fixed issues with previous patch (13.67 KB, patch)
2016-06-28 23:49 UTC, Andre Guerreiro
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Albert Astals Cid 2016-03-02 22:43:23 UTC
Expose the core code from https://bugs.freedesktop.org/show_bug.cgi?id=16770 for the glib frontend
Comment 1 Andre Guerreiro 2016-06-20 16:08:36 UTC
Created attachment 124622 [details] [review]
Expose signature verification in the glib frontend

Here's my attempt at exposing the signature verification feature at the glib level.
I've added a property to the document: signature count. I'm not really sure about this part because it depends on the loading of form fields which is done "on-demand" AFAICT so it can result in inconsistent behaviour.
Comment 2 Carlos Garcia Campos 2016-06-26 07:16:33 UTC
Comment on attachment 124622 [details] [review]
Expose signature verification in the glib frontend

Review of attachment 124622 [details] [review]:
-----------------------------------------------------------------

Thanks for the patch and sorry for the delay to review it. I haven't followed the core implementation, so I'm not sure how this API is expected to be used by applications. I guess the general idea is that the document might contain one or more signatures (defined as form fields), and applications can verify the document by validating every form field, right?

::: poppler-tmp/poppler-0.45.0/glib/poppler-document.cc
@@ +1105,4 @@
>  }
>  
>  /**
> + * poppler_document_count_signatures:

In glib API we normally use n_signatures instead of count

@@ +1119,5 @@
> +poppler_document_count_signatures(PopplerDocument *document)
> +{
> +  g_return_val_if_fail (POPPLER_IS_DOCUMENT (document), 0);
> +  return document->doc->getSignatureWidgets().size();
> +}

What's the point of providing the number of signatures, how is the user expected to use this? Just to know whether the document needs to be verified?

@@ +1478,5 @@
>  							 FALSE,
>  							 G_PARAM_READABLE));
> +  /**
> +   * PopplerDocument::signed:
> +   * Whether the document is signed.

This doesn't match the property definition. I'm not sure we need a property for this, though.

::: poppler-tmp/poppler-0.45.0/glib/poppler-form-field.cc
@@ +703,5 @@
> +
> +  new_info = g_new0(PopplerSignatureInfo, 1);
> +  new_info->sig_status = sig_info->sig_status;
> +  new_info->cert_status = sig_info->cert_status;
> +  new_info->signer_name = sig_info->signer_name;

This is owned by the other PopplerSignatureInfo, if you don't duplicate the string, freeing one structure would leave the other one with an invalid pointer.

@@ +704,5 @@
> +  new_info = g_new0(PopplerSignatureInfo, 1);
> +  new_info->sig_status = sig_info->sig_status;
> +  new_info->cert_status = sig_info->cert_status;
> +  new_info->signer_name = sig_info->signer_name;
> +  new_info->signing_time = sig_info->signing_time;

You can do *new_info = *sig_info and then copy manually only the fields needed, the signer name in this case.

@@ +712,5 @@
> +
> +void
> +poppler_signature_info_free (PopplerSignatureInfo *siginfo)
> +{
> +  g_free(siginfo);

And here you should also free the signer name

@@ +716,5 @@
> +  g_free(siginfo);
> +}
> +
> +PopplerSignatureInfo *
> +poppler_form_field_signature_validate (PopplerFormField *sigField, gboolean doVerifyCert, gboolean forceRevalidation)

This method also need to be documented. What do the parameters mean? I prefer to use flags oinstead of boolean parameters if possible.

@@ +725,5 @@
> +  FormFieldSignature * sig_field = static_cast<FormFieldSignature*>(sigField->widget->getField());
> +  SignatureInfo * sig_info = sig_field->validateSignature(doVerifyCert, forceRevalidation);
> +
> +  PopplerSignatureInfo * poppler_sig_info;
> +  poppler_sig_info = g_new0(PopplerSignatureInfo, 1);

Maybe this could be an out parameter, so that it can be stack allocated by the caller. And the method could return the validation result.

@@ +772,5 @@
> +    case CERTIFICATE_GENERIC_ERROR:
> +      poppler_sig_info->cert_status = POPPLER_CERTIFICATE_GENERIC_ERROR;
> +      break;
> +    case CERTIFICATE_NOT_VERIFIED:
> +      poppler_sig_info->cert_status = POPPLER_CERTIFICATE_NOT_VERIFIED;

So we are validating two things in the same function? Are really sig and cert status part of the signature information?

@@ +776,5 @@
> +      poppler_sig_info->cert_status = POPPLER_CERTIFICATE_NOT_VERIFIED;
> +      break;
> +  }
> +
> +  poppler_sig_info->signer_name   = sig_info->getSignerName();

The return value is owned by SignatureInfo, we should copy it, and SignatureInfo should return a const char* instead.
Comment 3 Andre Guerreiro 2016-06-28 23:49:53 UTC
Created attachment 124771 [details] [review]
Fixed issues with previous patch

This patch addresses the issues Carlos mentioned, with the exception of the return value of poppler_form_field_signature_validate() because I think it makes sense to return the output of the whole operation in a single structure and I should add that signature and cert status are different steps of the same high-level validation so I don't think certificate validation should have a seperate entry point in the API.

Actually thinking of the goal of providing trust to the end-user, certificate validation should be always performed and we only offer a way to disable it because in practice there are cases when we can't accomplish it because e.g. we are offline and so we can't check revocation status via OCSP
Comment 4 Yajo 2017-04-06 15:58:12 UTC
Any progress on this? I'm eager to see https://bugzilla.gnome.org/show_bug.cgi?id=614929 fixed 😊
Comment 5 GitLab Migration User 2018-08-21 11:00:05 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/464.


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.