GCC -fanalyzer finds an issue

[maxgerhardt]

Version

5.6.0-stable

Description

Turning on the, since GCC 10 new, -fanalyzer static analyzer switch for WolfSSL compilation makes it output a single issue it found with WolfSSL code:

https://github.com/wolfSSL/wolfssl/blob/158c0362e7b12d2455d739814aa23fa565825e2d/src/internal.c#L11552-L11570

it thinks that a dereference of dCert can happen through a certain codepath.

lib/wolfssl-5.6.0-stable/src/internal.c: In function 'CheckHostName':
lib/wolfssl-5.6.0-stable/src/internal.c:11556:34: warning: dereference of NULL 'dCert' [CWE-690] [-Wanalyzer-null-dereference]
11556 |         if (MatchDomainName(dCert->subjectCN, dCert->subjectCNLen,
      |                             ~~~~~^~~~~~~~~~~
  'CheckHostName': events 1-2
    |
    |11539 | int CheckHostName(DecodedCert* dCert, const char *domainName, size_t domainNameLen)
    |      |     ^~~~~~~~~~~~~
    |      |     |
    |      |     (1) entry to 'CheckHostName'
    |......
    |11547 |     if (CheckForAltNames(dCert, domainName, &checkCN) != 1) {
    |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (2) calling 'CheckForAltNames' from 'CheckHostName'
    |
    +--> 'CheckForAltNames': events 3-9
           |
           |11479 | int CheckForAltNames(DecodedCert* dCert, const char* domain, int* checkCN)
           |      |     ^~~~~~~~~~~~~~~~
           |      |     |
           |      |     (3) entry to 'CheckForAltNames'
           |......
           |11488 |     if (dCert)
           |      |        ~
           |      |        |
           |      |        (4) following 'false' branch (when 'dCert' is NULL)...
           |......
           |11491 |     if (checkCN != NULL) {
           |      |        ~
           |      |        |
           |      |        (5) ...to here
           |      |        (6) following 'true' branch (when 'checkCN' is non-NULL)...
           |11492 |         *checkCN = (altName == NULL) ? 1 : 0;
           |      |                    ~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                                          |
           |      |                                          (7) ...to here
           |......
           |11495 |     while (altName) {
           |      |           ~
           |      |           |
           |      |           (8) following 'false' branch (when 'altName' is NULL)...
           |......
           |11527 |     return match;
           |      |            ~~~~~
           |      |            |
           |      |            (9) ...to here
           |
    <------+
    |
  'CheckHostName': events 10-15
    |
    |11547 |     if (CheckForAltNames(dCert, domainName, &checkCN) != 1) {
    |      |        ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |        ||
    |      |        |(10) returning to 'CheckHostName' from 'CheckForAltNames'
    |      |        (11) following 'true' branch...
    |......
    |11555 |     if (checkCN == 1) {
    |      |        ~~~~~~~~~~~~~
    |      |        |        |
    |      |        |        (12) ...to here
    |      |        (13) following 'true' branch...
    |11556 |         if (MatchDomainName(dCert->subjectCN, dCert->subjectCNLen,
    |      |                             ~~~~~~~~~~~~~~~~
    |      |                                  |
    |      |                                  (14) ...to here
    |      |                                  (15) dereference of NULL 'dCert'
    |

Even though the only usage of that function is in x509.c where it errors out if the certificate can be not decoded, it would still be nice to satisfy the static analyzer.

wolfssl/src/x509.c

Lines 12785 to 12791 in 158c036

	InitDecodedCert(dCert, x->derCert->buffer, x->derCert->length, NULL);
	ret = ParseCertRelative(dCert, CERT_TYPE, 0, NULL);
	if (ret != 0) {
	goto out;
	}
	ret = CheckHostName(dCert, (char *)chk, chklen);

[lealem47]

Hi @maxgerhardt,

This code path looks impossible to me. If dCert was NULL, ParseCertRelative() would have returned an error code and CheckHostName() would have never been called because of the goto.

[maxgerhardt]

Yes, the usage of the function in x509.c cannot lead to a nullptr dereference, but the analyzer is looking at the function as being callable on its own (where it can freely chose the input arguments), and it found a path per above.

[lealem47]

Hi @maxgerhardt,

My sincere apologies for losing track of this. I've added a commit to my PR here to add a check that addresses this #9449

Thanks

[lealem47]

Closing this now as the fix has been merged