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

make CryptQueryObject call compliant to interop guidelines #60702

Merged
merged 2 commits into from
Oct 28, 2021

Conversation

pedrobsaila
Copy link
Contributor

@pedrobsaila pedrobsaila commented Oct 20, 2021

Fixes partially issue #51564
(this issue is still under progress, more PRs are to come)

This PR makes the following calls compliant with Interop gudielines :

  • Crypt32.CryptQueryObject
  • Crypt32.CertGetCertificateContextProperty
  • Crypt32.CertDuplicateCertificateContext
    I know the PR should be small, but it's barely the minimum to move CryptQueryObject : this call uses many struct/enum/class and safe handles, that should either also be moved to Common folder.or deleted because they already exists there (so we need to reuse the later)

There's a change to CERT_INFO class inside Common folder, so it uses the created struct FILENAME (like CERT_INFO inside System.Security.Cryptography.X509Certificates\src\Internal\Cryptography\Pal.Windows\Native\Primitives.cs), using CERT_INFO with COM FILENAME causes overflows in tests since it uses int instead of uint.

I create this file Common/src/Interop/Windows/Crypt32/Interop.CertGetCertificateContextProperty_NO_NULLABLE.cs instead of reusing Common/src/Interop/Windows/Crypt32/Interop.CertGetCertificateContextProperty.cs because it's referenced by System.Windows.Extensions which does not support nullable yet

Sorry for the PR #60218 I was trying to solve a conflict in interop.crypt32.cs by rebasing my branch on master and I did something wrong.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 20, 2021
@ghost
Copy link

ghost commented Oct 20, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes partially issue #51564
(this issue is still under progress, more PRs are to come)

This PR makes the following calls compliant with Interop gudielines :

  • Crypt32.CryptQueryObject
  • Crypt32.CertGetCertificateContextProperty
  • Crypt32.CertDuplicateCertificateContext
    I know the PR should be small, but it's barely the minimum to move CryptQueryObject : this call uses many struct/enum/class and safe handles, that should either also be moved to Common folder.or deleted because they already exists there (so we need to reuse the later)

There's a change to CERT_INFO class inside Common folder, so it uses the created struct FILENAME (like CERT_INFO inside System.Security.Cryptography.X509Certificates\src\Internal\Cryptography\Pal.Windows\Native\Primitives.cs), using CERT_INFO with COM FILENAME causes overflows in tests since it uses int instead of uint.

I create this file Common/src/Interop/Windows/Crypt32/Interop.CertGetCertificateContextProperty_NO_NULLABLE.cs instead of reusing Common/src/Interop/Windows/Crypt32/Interop.CertGetCertificateContextProperty.cs because it's referenced by System.Windows.Extensions which does not support nullable yet

Sorry for the PR #60218 I was trying to solve a conflict by rebasing my branch on master and I did something wrong.

Author: pedrobsaila
Assignees: -
Labels:

area-System.Security, community-contribution

Milestone: -

@@ -99,7 +99,7 @@ static delegate (void* pvDecoded, int cbDecoded)
{
Debug.Assert(cbDecoded >= sizeof(CERT_BASIC_CONSTRAINTS_INFO));
CERT_BASIC_CONSTRAINTS_INFO* pBasicConstraints = (CERT_BASIC_CONSTRAINTS_INFO*)pvDecoded;
return ((pBasicConstraints->SubjectType.pbData[0] & CERT_BASIC_CONSTRAINTS_INFO.CERT_CA_SUBJECT_FLAG) != 0,
return ((pBasicConstraints->SubjectType.ToByteArray()[0] & CERT_BASIC_CONSTRAINTS_INFO.CERT_CA_SUBJECT_FLAG) != 0,
Copy link
Member

Choose a reason for hiding this comment

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

Surely we can do better than making a byte array here...

Copy link
Member

Choose a reason for hiding this comment

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

Something like

Suggested change
return ((pBasicConstraints->SubjectType.ToByteArray()[0] & CERT_BASIC_CONSTRAINTS_INFO.CERT_CA_SUBJECT_FLAG) != 0,
return ((Marshal.ReadByte(pBasicConstraints->SubjectType.pbData) & CERT_BASIC_CONSTRAINTS_INFO.CERT_CA_SUBJECT_FLAG) != 0,

@bartonjs bartonjs merged commit a858050 into dotnet:main Oct 28, 2021
@pedrobsaila pedrobsaila deleted the 51564/part2 branch October 31, 2021 15:11
@ghost ghost locked as resolved and limited conversation to collaborators Nov 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants