Skip to content
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

Fix cert finding logic and ensure context cleanup for windows #76

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

gehhilfe
Copy link
Contributor

  • Ensure the context are cleaned up when not returned
  • Iterate all certificates for each issuer

I noticed during some testing with multiple certificates contained in a Windows store that only the first certificate is iterated instead of all.
When changing that all certs are iterated for each issuer, it also needs to free last context if is not returned to the caller.
At least, following the win32 wincrypt documentation:

[in] pPrevCertContext

A pointer to the last CERT_CONTEXT structure returned by this function. This parameter must be NULL on the first call of the function. To find successive certificates meeting the search criteria, set pPrevCertContext to the pointer returned by the previous call to the function. This function frees the CERT_CONTEXT referenced by non-NULL values of this parameter.

A non-NULL CERT_CONTEXT that CertFindCertificateInStore returns must be freed by CertFreeCertificateContext or by being passed as the pPrevCertContext parameter on a subsequent call to CertFindCertificateInStore

I marked this as draft because I want to test this more and maybe clean up the code a bit more the nested for loops are quite complex

Copy link

google-cla bot commented Sep 16, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mit-da
Copy link
Contributor

mit-da commented Sep 23, 2024

Thanks for the PR, will be reviewing in a bit.

@mit-da
Copy link
Contributor

mit-da commented Sep 26, 2024

Give us a shout when you think its ready for review and I'll take a look.

@gehhilfe gehhilfe marked this pull request as ready for review September 27, 2024 13:09
@gehhilfe
Copy link
Contributor Author

Ready to review.
I removed additional free, because the loop for cert != nil is executed one last time after the last certificate is found for a issuer and frees the prev context.

Copy link
Contributor

@mit-da mit-da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, lgtm

@mit-da mit-da merged commit 10f8401 into google:master Oct 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants