re: PKCS#7: Verify internal certificate chain

re: PKCS#7: Verify internal certificate chain

[Dan Carpenter]

Hello David Howells,

This is a semi-automatic email about new static checker warnings.

The patch 8c76d79393cc: "PKCS#7: Verify internal certificate chain" 
from Jul 1, 2014, leads to the following Smatch complaint:

crypto/asymmetric_keys/pkcs7_verify.c:200 pkcs7_verify_sig_chain()
	 error: we previously assumed 'x509->issuer' could be null (see line 193)

crypto/asymmetric_keys/pkcs7_verify.c
   192	
   193			if (x509->issuer)
                            ^^^^^^^^^^^^
Check.

   194				pr_debug("- issuer %s\n", x509->issuer);
   195			if (x509->authority)
   196				pr_debug("- authkeyid %s\n", x509->authority);
   197	
   198			if (!x509->authority ||
   199			    (x509->subject &&
   200			     strcmp(x509->subject, x509->issuer) == 0)) {
                                                   ^^^^^^^^^^^^
Unchecked dereference.  But maybe a non-NULL ->subject implies ->issuer
is non-NULL?

   201				/* If there's no authority certificate specified, then
   202				 * the certificate must be self-signed and is the root
   203                           * of the chain.  Likewise if the cert is its own
   204                           * authority.
   205                           */
   206                          pr_debug("- no auth?\n");
   207                          if (x509->raw_subject_size != x509->raw_issuer_size ||
   208                              memcmp(x509->raw_subject, x509->raw_issuer,
   209                                     x509->raw_issuer_size) != 0)
   210                                  return 0;
   211  
   212                          ret = x509_check_signature(x509->pub, x509);
   213                          if (ret < 0)
   214                                  return ret;
   215                          x509->signer = x509;
   216                          pr_debug("- self-signed\n");
   217                          return 0;
   218                  }
   219  
   220                  /* Look through the X.509 certificates in the PKCS#7 message's
   221                   * list to see if the next one is there.
   222                   */
   223                  pr_debug("- want %s\n", x509->authority);
   224                  for (p = pkcs7->certs; p; p = p->next) {
   225                          pr_debug("- cmp [%u] %s\n", p->index, p->fingerprint);
   226                          if (p->raw_subject_size == x509->raw_issuer_size &&
   227                              strcmp(p->fingerprint, x509->authority) == 0 &&
   228                              memcmp(p->raw_subject, x509->raw_issuer,
   229                                     x509->raw_issuer_size) == 0)
   230                                  goto found_issuer;
   231                  }
   232  
   233                  /* We didn't find the root of this chain */
   234                  pr_debug("- top\n");
   235                  return 0;
   236  
   237          found_issuer:
   238                  pr_debug("- issuer %s\n", p->subject);
                                    ^^^^^^
Also this looks like it should be "subject" instead of "issuer" but
maybe they are the same?

   239                  if (p->seen) {
   240                          pr_warn("Sig %u: X.509 chain contains loop\n",
   241                                  sinfo->index);


regards,
dan carpenter

Re: PKCS#7: Verify internal certificate chain

[David Howells]

Dan Carpenter <dan.carpenter@oracle.com> wrote:

>    193			if (x509->issuer)
>                             ^^^^^^^^^^^^
> Check.
> 
>    194				pr_debug("- issuer %s\n", x509->issuer);

Actually, x509->subject and x509->issuer are mandatory in the ASN.1 definition
and point to empty strings if there's no name material we can use, so the
check is superfluous.

>    199			    (x509->subject &&

This check too.

However, I probably should check that x509->subject is not an empty string.

>    238                  pr_debug("- issuer %s\n", p->subject);
>                                     ^^^^^^
> Also this looks like it should be "subject" instead of "issuer" but
> maybe they are the same?

This is the subject attribute of issuing certificate (that p points to), not
the issuing attribute of the subject certificate (that x509 points to).

Note this is a debugging line.  x509->issuer was printed by a pr_debug()
several lines above.

David