Bug 94376

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 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.

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

Any progress on this? I'm eager to see https://bugzilla.gnome.org/show_bug.cgi?id=614929 fixed 😊

-- 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.