Skip to content

False positives in cpp/user-after-free #19387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ajohnston9 opened this issue Apr 25, 2025 · 2 comments
Open

False positives in cpp/user-after-free #19387

ajohnston9 opened this issue Apr 25, 2025 · 2 comments
Labels
acknowledged GitHub staff acknowledges this issue false-positive

Comments

@ajohnston9
Copy link

cpp-user-after-free seems to have a number of false positives, particular when a pointer is freed, re-allocated, and then reused correctly.

Consider the following code snippet from this part of OpenSC:

free(priv->aid_der.value);  /* free previous value if any */
if ((priv->aid_der.value = malloc(resplen)) == NULL) {
	LOG_FUNC_RETURN(card->ctx, SC_ERROR_OUT_OF_MEMORY);
}
memcpy(priv->aid_der.value, rbuf, resplen);
priv->aid_der.len = resplen;
LOG_FUNC_RETURN(card->ctx,i);

Analysis of the code shows that although free(priv->aid_der.value) is called at line 2939, the pointer priv->aid_der.value is immediately reassigned by a malloc call at line 2940. If the malloc fails, the function returns before the potentially dangerous memcpy at line 2943 is reached. If malloc succeeds, the memcpy operates on the newly allocated buffer, preventing a use-after-free. This finding appears to be a false positive.

Similarly, consider this snippet from this part of Assimp:

        delete[] pcScene->mRootNode->mChildren;
        for (std::vector<const BaseNode *>::/*const_*/ iterator i = aiList.begin(); i != aiList.end(); ++i) {
            const ASE::BaseNode *src = *i;

            // The parent is not known, so we can assume that we must add
            // this node to the root node of the whole scene
            aiNode *pcNode = new aiNode();
            pcNode->mParent = pcScene->mRootNode;
            pcNode->mName.Set(src->mName);
            AddMeshes(src, pcNode);
            AddNodes(nodes, pcNode, pcNode->mName.data);
            apcNodes.push_back(pcNode);
        }

        // Regenerate our output array
        pcScene->mRootNode->mChildren = new aiNode *[apcNodes.size()];
        for (unsigned int i = 0; i < apcNodes.size(); ++i)
            pcScene->mRootNode->mChildren[i] = apcNodes[i];

        pcScene->mRootNode->mNumChildren = (unsigned int)apcNodes.size();
    }

Again, the CodeQL finding indicates a potential use-after-free vulnerability where pcScene->mRootNode->mChildren is deleted at line 661 and potentially used later at line 678. Analysis of the code shows that pcScene->mRootNode->mChildren is reallocated with new aiNode*[apcNodes.size()] on line 678 before being accessed in the loop starting on line 679. Therefore, this finding appears to be a false positive, as the memory is valid when accessed.

These findings were generated when running codeql/cpp-queries against the given codebases.

Recommendation

Revise the query to look for use of a freed pointer where the the use of the pointer is done before any malloc, new or similar memory-allocating function is performed. Validate the query against test cases such as these to ensure that proper pointer re-use isn't flagged as a potential UAF.

@coadaflorin
Copy link
Contributor

Hey @ajohnston9 thanks for raising this!
Also, thank you for the detailed description here. Did you attempt any QL modifications to sort this issue? If you have anything that could help us kick-start the work on our side, we'll be very happy to look at it :)

@jketema
Copy link
Contributor

jketema commented Apr 28, 2025

Hi @ajohnston9,

Thanks for your report. We are aware of this problem, and unfortunately there's no easy fix for this. I've added a link to this issue to our internal issue tracking this.

@jketema jketema added the acknowledged GitHub staff acknowledges this issue label Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged GitHub staff acknowledges this issue false-positive
Projects
None yet
Development

No branches or pull requests

3 participants