Page MenuHome GnuPG

Function ksba_cert_get_digest_algo returns an uninitialized pointer in some error cases
Closed, ResolvedPublic

Description

The function ksba_cert_get_digest_algo looks as follows:

const char *
ksba_cert_get_digest_algo (ksba_cert_t cert)
{

gpg_error_t err;
AsnNode n;
char *algo;

...

  // 1
  n = _ksba_asn_find_node (cert->root, "Certificate.signatureAlgorithm");
  if (!n || n->off == -1)
    err = gpg_error (GPG_ERR_UNKNOWN_ALGORITHM);
  else
    err = _ksba_parse_algorithm_identifier (cert->image + n->off,
                                            n->nhdr + n->len, &nread, &algo);
  if (err)
    cert->last_error = err;
  else
    cert->cache.digest_algo = algo;

  return algo;

}

At point // 1, the local variable algo has not been written to yet. When the "then" branch of the "if (!n || n->off == -1)" is taken, it continues to be
uninitialized. Neither branch of the second "if" initialize it. And then it is returned.

Returning an uninitialized pointer is very poor form even if it is this function's official API that cert->last_error should be checked. That makes it
very easy to misuse, and returning a null pointer instead would at least mean that the caller can test the pointer as a more usual check, and that the
program will crash cleanly if the pointer is dereferenced without a check.

Case in point: if that is the API, the program test/cert-basic uses the function ksba_cert_get_digest_algo incorrectly:

  oid = ksba_cert_get_digest_algo (cert);
  s = get_oid_desc (oid);

The function get_oid_desc has no way to know that it has been passed an uninitialized pointer. This uninitialized pointer is not guaranteed to be null.
When used, it may crash the cert-basic program, or not.

Probably the intention here is that a null pointer should be returned when the then branch of the "if (!n || n->off == -1)" has been taken.

Details

Version
1.3.3

Event Timeline

werner added a subscriber: werner.

Fixed with commit 3f74c2c. Thanks.

The use in cert-basic is correct because get_oid_desc accepst a NULL pointer.
However, some libc versions bail out on a NULL for "%s"; I fixed that too.

werner claimed this task.
werner removed a project: Restricted Project.