Re: [PATCH v6 11/28] crypto/x509-utils: Add helper functions for DIAG 508 subcode 1

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Oct 07, 2025 at 11:58:03AM +0200, Thomas Huth wrote:
> On 18/09/2025 01.21, Zhuoying Cai wrote:
> > Introduce helper functions to support signature verification required by
> > DIAG 508 subcode 1:
> > 
> > qcrypto_pkcs7_convert_sig_pem() – converts a signature from DER to PEM format
> > qcrypto_x509_verify_sig() – verifies the provided data against the given signature
> > 
> > These functions enable basic signature verification support.
> > 
> > Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> > ---
> >   crypto/x509-utils.c         | 109 ++++++++++++++++++++++++++++++++++++
> >   include/crypto/x509-utils.h |  39 +++++++++++++
> >   2 files changed, 148 insertions(+)
> > 
> > diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
> > index 763eccb190..8f3c895d7c 100644
> > --- a/crypto/x509-utils.c
> > +++ b/crypto/x509-utils.c


> > +int qcrypto_x509_verify_sig(uint8_t *cert, size_t cert_size,
> > +                            uint8_t *comp, size_t comp_size,
> > +                            uint8_t *sig, size_t sig_size, Error **errp)
> > +{
> > +    int rc;
> > +    int ret = -1;
> > +    gnutls_x509_crt_t crt = NULL;
> > +    gnutls_pkcs7_t signature = NULL;
> > +    gnutls_datum_t cert_datum = {.data = cert, .size = cert_size};
> > +    gnutls_datum_t data_datum = {.data = comp, .size = comp_size};
> > +    gnutls_datum_t sig_datum = {.data = sig, .size = sig_size};
> > +
> > +    rc = gnutls_x509_crt_init(&crt);
> > +    if (rc < 0) {
> > +        error_setg(errp, "Failed to initialize certificate: %s", gnutls_strerror(rc));
> > +        goto cleanup;
> 
> So if you go to cleanup here, signature is still NULL ...
> 
> > +    }
> > +
> > +    rc = gnutls_x509_crt_import(crt, &cert_datum, GNUTLS_X509_FMT_PEM);
> > +    if (rc != 0) {
> > +        error_setg(errp, "Failed to import certificate: %s", gnutls_strerror(rc));
> > +        goto cleanup;
> > +    }
> > +
> > +    rc = gnutls_pkcs7_init(&signature);
> > +    if (rc < 0) {
> > +        error_setg(errp, "Failed to initalize pkcs7 data: %s", gnutls_strerror(rc));
> > +        goto cleanup;
> > +     }
> > +
> > +    rc = gnutls_pkcs7_import(signature, &sig_datum , GNUTLS_X509_FMT_PEM);
> > +    if (rc != 0) {
> > +        error_setg(errp, "Failed to import signature: %s", gnutls_strerror(rc));
> > +        goto cleanup;
> > +    }
> > +
> > +    rc = gnutls_pkcs7_verify_direct(signature, crt, 0, &data_datum, 0);
> > +    if (rc != 0) {
> > +        error_setg(errp, "Failed to verify signature: %s", gnutls_strerror(rc));
> > +        goto cleanup;
> > +    }
> > +
> > +    ret = 0;
> > +
> > +cleanup:
> > +    gnutls_x509_crt_deinit(crt);
> > +    gnutls_pkcs7_deinit(signature);
> 
> ... did you check whether gnutls_pkcs7_deinit() is able to deal with NULL
> pointers? The man-page does not mention it ... so just to be on the save
> side, maybe it would be better to have to cleanup labels, and only do the
> gnutls_pkcs7_deinit() if it has been initialized before?

All gnutls APIs for free'ing objects accept NULL and act as a no-op,
so we're ok.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|