[ILASM] Add wrappers for platform-specific SHA-256 implementations by amanasifkhalid · Pull Request #109559 · dotnet/runtime

amanasifkhalid · 2024-11-05T18:10:13Z

Part of #108627. I'm splitting this out of #109091 to expedite review, as the crypto part of the ILAsm determinism work seems to be in decent shape. The intent is to consume this API surface in ILAsm to produce checksums, though I imagine these wrappers could be useful elsewhere.

dotnet-policy-service · 2024-11-05T18:12:01Z

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

src/coreclr/md/enc/CMakeLists.txt

amanasifkhalid · 2024-11-05T18:21:31Z

@jkotas could you PTAL? Thanks!

src/coreclr/md/enc/CMakeLists.txt

vcsjones · 2024-11-05T19:28:27Z

src/coreclr/md/enc/sha256.cpp

+{
+    CC_SHA256_CTX ctx = {{ 0 }};
+
+    if (!CC_SHA256_Init(&ctx))


Apple has a one-shot you can use too. It would look something like this:

HRESULT Sha256Hash(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize) { if (dstSize < CC_SHA256_DIGEST_LENGTH) { return E_FAIL; } CC_SHA256(pSrc, (CC_LONG)srcSize, pDst); return S_OK; }

If there is an actual need for it, you can copy to a temp buffer like Windows and Linux do.

vcsjones · 2024-11-05T19:31:16Z

src/coreclr/md/enc/sha256.cpp

+        goto cleanup;
+    }
+
+    memcpy(pDst, hash, min(hashLength, dstSize));


Why do we use min here? Is there a real scenario where dstSize is not big enough to hold the whole SHA-256 and it needs to be truncated?

In #109091, it's convenient to be able to get a truncated hash that can be used as a checksum (example). We could move the temp buffer logic to the caller, and require that pDst in Sha256Hash is big enough for a whole SHA-256 digest, if you'd prefer.

We could move the temp buffer logic to the caller

I think it would make more sense.

jkotas · 2024-11-05T20:58:33Z

cc @bartonjs This change is borrowing distro agnostic OpenSSL loading logic from System.Security.Cryptography.Native for ildasm

am11 · 2024-11-06T16:27:27Z

src/coreclr/ilasm/sha256.h

+
+    return S_OK;
+}
+#elif defined(__linux__)


This can be just #else as CryptoNative_EvpDigestOneShot is available on any Unix.

am11 · 2024-11-06T16:28:26Z

src/coreclr/ilasm/CMakeLists.txt

+  sha256.h
 )

+if(CLR_CMAKE_TARGET_LINUX)


Suggested change

if(CLR_CMAKE_TARGET_LINUX)

if(CLR_CMAKE_TARGET_UNIX AND NOT CLR_CMAKE_TARGET_APPLE)

am11 · 2024-11-06T16:28:32Z

src/coreclr/ilasm/CMakeLists.txt

    )
 endif(CLR_CMAKE_TARGET_WIN32)

+if(CLR_CMAKE_TARGET_LINUX)


Suggested change

if(CLR_CMAKE_TARGET_LINUX)

if(CLR_CMAKE_TARGET_UNIX AND NOT CLR_CMAKE_TARGET_APPLE)

src/coreclr/ilasm/sha256.h

jkotas · 2024-11-06T16:41:08Z

src/coreclr/ilasm/sha256.h

+    {
+         BCryptDestroyHash(hashHandle);
+    }
+    if(NULL != algHandle)


Suggested change

if(NULL != algHandle)

if (NULL != algHandle)

jkotas · 2024-11-06T16:43:25Z

src/coreclr/ilasm/sha256.h

+    {
+        goto cleanup;
+    }
+    if (hashLength != 32)


I am not sure why we need to check this - we do not have the same check for other OSes. SHA256 is defined as returning 32 bytes. If the crypto provider returns something else here, something is seriously broken.

Co-authored-by: Jan Kotas <[email protected]>

am11 · 2024-11-06T17:29:17Z

src/coreclr/ilasm/sha256.h

+inline bool IsOpenSslAvailable()
+{
+    return CryptoNative_OpenSslAvailable() != 0;
+}
+
+inline HRESULT Sha256Hash(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize)
+{
+    if (!IsOpenSslAvailable() || CryptoNative_EnsureOpenSslInitialized() || (dstSize < 32))


nit: perhaps a bit simpler this way:

Suggested change

inline bool IsOpenSslAvailable()

{

return CryptoNative_OpenSslAvailable() != 0;

}

inline HRESULT Sha256Hash(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize)

{

if (!IsOpenSslAvailable() || CryptoNative_EnsureOpenSslInitialized() || (dstSize < 32))

inline HRESULT Sha256Hash(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize)

{

if (CryptoNative_OpenSslAvailable() == 0 || CryptoNative_EnsureOpenSslInitialized() != 0 || (dstSize < 32))

I agree it looks a bit redundant in its current form. The intent behind IsOpenSslAvailable is to allow ILAsm to disable features based on OpenSSL availability, without leaking the CryptoNative API surface to it. Including it in this PR is probably premature, though I will want something like it when ILAsm starts using these wrappers.

I thought I saw a comment from Jan K that said that we should treat OpenSSL here as required as it is for the platform... which is to say "it's required for 'Linux'" (which includes OpenBSD/NetBSD/etc).

If you're taking it as required, you can drop the call to CryptoNative_OpenSslAvailable; CryptoNative_EnsureOpenSslInitialized will do the same dlopen that Available does, it just fails (via assert) if the library isn't found.

If you're taking it as optional, then using OpenSslAvailable is correct.

I suppose it makes sense to delegate the responsibility of checking for OpenSSL availability to the caller, if we're going to provide IsOpenSslAvailable anyway.

If you just call CryptoNative_EnsureOpenSslInitialized and nothing else, CryptoNative_EnsureOpenSslInitialized is going to print "No usable version of libssl was found" and abort if there is not openssl available on the machine.

I think it is good enough. Explicitly calling OpenSslAvailable makes sense only if you would like to provide a different error message or handle the error situation differently.

src/coreclr/ilasm/CMakeLists.txt

jkotas · 2024-11-06T20:55:20Z

src/coreclr/ilasm/sha256.h

+        return E_FAIL;
+    }
+
+    DWORD hashLength = 0;


Suggested change

DWORD hashLength = 0;

uint32_t hashLength = 0;

jkotas · 2024-11-06T20:58:31Z

src/coreclr/ilasm/sha256.h

+    DWORD hashLength   = 0;
+    DWORD resultLength = 0;
+
+    NTSTATUS status = BCryptOpenAlgorithmProvider(&algHandle, BCRYPT_SHA256_ALGORITHM, NULL, 0);
+
+    if (!NT_SUCCESS(status))
+    {
+        goto cleanup;
+    }
+
+    status = BCryptGetProperty(algHandle, BCRYPT_HASH_LENGTH, (PBYTE)&hashLength, sizeof(hashLength), &resultLength, 0);
+
+    if (!NT_SUCCESS(status))
+    {
+        goto cleanup;
+    }
+
+    assert(hashLength == 32);
+    status = BCryptCreateHash(algHandle, &hashHandle, NULL, 0, NULL, 0, 0);
+
+    if (!NT_SUCCESS(status))
+    {
+        goto cleanup;
+    }
+
+    status = BCryptHashData(hashHandle, pSrc, srcSize, 0);
+
+    if (!NT_SUCCESS(status))
+    {
+        goto cleanup;
+    }
+
+    status = BCryptFinishHash(hashHandle, pDst, hashLength, 0);


Suggested change

DWORD hashLength = 0;

DWORD resultLength = 0;

NTSTATUS status = BCryptOpenAlgorithmProvider(&algHandle, BCRYPT_SHA256_ALGORITHM, NULL, 0);

if (!NT_SUCCESS(status))

{

goto cleanup;

}

status = BCryptGetProperty(algHandle, BCRYPT_HASH_LENGTH, (PBYTE)&hashLength, sizeof(hashLength), &resultLength, 0);

if (!NT_SUCCESS(status))

{

goto cleanup;

}

assert(hashLength == 32);

status = BCryptCreateHash(algHandle, &hashHandle, NULL, 0, NULL, 0, 0);

if (!NT_SUCCESS(status))

{

goto cleanup;

}

status = BCryptHashData(hashHandle, pSrc, srcSize, 0);

if (!NT_SUCCESS(status))

{

goto cleanup;

}

status = BCryptFinishHash(hashHandle, pDst, hashLength, 0);

DWORD resultLength = 0;

NTSTATUS status = BCryptOpenAlgorithmProvider(&algHandle, BCRYPT_SHA256_ALGORITHM, NULL, 0);

if (!NT_SUCCESS(status))

{

goto cleanup;

}

status = BCryptCreateHash(algHandle, &hashHandle, NULL, 0, NULL, 0, 0);

if (!NT_SUCCESS(status))

{

goto cleanup;

}

status = BCryptHashData(hashHandle, pSrc, srcSize, 0);

if (!NT_SUCCESS(status))

{

goto cleanup;

}

status = BCryptFinishHash(hashHandle, pDst, dstSize, 0);

jkotas · 2024-11-06T21:01:30Z

src/coreclr/ilasm/sha256.h

+#ifdef _WIN32
+inline HRESULT Sha256Hash(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize)
+{
+    if (dstSize < 32)


Should this check for exact match?

Taking output buffer, filling part of it, and not telling the caller how much of it was filled is odd. The typical patterns for output buffers are:

Take output buffer and fill it completely, fail if it is not possible to fill it completely

Take output buffer and return how much of it was filled

jkotas · 2024-11-06T21:05:55Z

src/coreclr/ilasm/sha256.h

+
+inline HRESULT Sha256Hash(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize)
+{
+    if (!IsOpenSslAvailable() || CryptoNative_EnsureOpenSslInitialized() || (dstSize < 32))


Suggested change

if (!IsOpenSslAvailable() || CryptoNative_EnsureOpenSslInitialized() || (dstSize < 32))

if (CryptoNative_EnsureOpenSslInitialized() || (dstSize < 32))

CryptoNative_OpenSslAvailable call is unnecessary here. It is done by CryptoNative_EnsureOpenSslInitialized

jkotas

LGTM!

src/coreclr/ilasm/sha256.h

Co-authored-by: Adeel Mujahid <[email protected]>

amanasifkhalid · 2024-11-07T05:46:50Z

Thanks for the reviews!

amanasifkhalid added 2 commits November 5, 2024 13:07

Add SHA-256 impls

954d649

Add space

e605218

ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 5, 2024

dotnet-policy-service bot assigned amanasifkhalid Nov 5, 2024

amanasifkhalid added area-ILTools-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 5, 2024

amanasifkhalid commented Nov 5, 2024

View reviewed changes

src/coreclr/md/enc/CMakeLists.txt Outdated Show resolved Hide resolved

jkoritzinsky reviewed Nov 5, 2024

View reviewed changes

src/coreclr/md/enc/CMakeLists.txt Outdated Show resolved Hide resolved

vcsjones reviewed Nov 5, 2024

View reviewed changes

build-analysis bot mentioned this pull request Nov 5, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

amanasifkhalid added 2 commits November 5, 2024 16:24

One-shot CommonCrypto SHA-256

58b85ab

Move SHA-256 wrappers to ilasm

6a077a6

amanasifkhalid changed the title ~~Add wrappers for platform-specific SHA-256 implementations~~ [ILASM] Add wrappers for platform-specific SHA-256 implementations Nov 5, 2024

am11 reviewed Nov 6, 2024

View reviewed changes

Feedback

d67fe75

jkotas reviewed Nov 6, 2024

View reviewed changes

src/coreclr/ilasm/sha256.h Outdated Show resolved Hide resolved

jkotas reviewed Nov 6, 2024

View reviewed changes

amanasifkhalid and others added 3 commits November 6, 2024 11:44

Remove reuse flag

5b7468c

Co-authored-by: Jan Kotas <[email protected]>

Style; remove hashLength check

10254e7

Remove temp buffer

0b0063d

am11 reviewed Nov 6, 2024

View reviewed changes

bartonjs reviewed Nov 6, 2024

View reviewed changes

src/coreclr/ilasm/CMakeLists.txt Show resolved Hide resolved

jkotas reviewed Nov 6, 2024

View reviewed changes

amanasifkhalid added 2 commits November 6, 2024 16:20

Remove unnecessary OpenSSL check

c3c90e8

Check for exact dstSize match

61aa71e

Simplify Win32 impl

49f782b

jkotas approved these changes Nov 6, 2024

View reviewed changes

build-analysis bot mentioned this pull request Nov 6, 2024

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

am11 reviewed Nov 6, 2024

View reviewed changes

src/coreclr/ilasm/sha256.h Outdated Show resolved Hide resolved

am11 reviewed Nov 6, 2024

View reviewed changes

src/coreclr/ilasm/sha256.h Outdated Show resolved Hide resolved

amanasifkhalid and others added 2 commits November 7, 2024 00:23

Rename macro

2951f66

Co-authored-by: Adeel Mujahid <[email protected]>

Update comment

8b7d4fb

Co-authored-by: Adeel Mujahid <[email protected]>

amanasifkhalid merged commit 36bcc2c into dotnet:main Nov 7, 2024

amanasifkhalid deleted the sha256-impl branch November 7, 2024 05:47

amanasifkhalid mentioned this pull request Nov 8, 2024

[ILASM] Add support for deterministic builds and PDB checksums #109091

Merged

github-actions bot locked and limited conversation to collaborators Dec 7, 2024

	if(CLR_CMAKE_TARGET_LINUX)
	if(CLR_CMAKE_TARGET_UNIX AND NOT CLR_CMAKE_TARGET_APPLE)

	if (!IsOpenSslAvailable() \|\| CryptoNative_EnsureOpenSslInitialized() \|\| (dstSize < 32))
	if (CryptoNative_EnsureOpenSslInitialized() \|\| (dstSize < 32))

Conversation

amanasifkhalid commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Nov 5, 2024

Uh oh!

Uh oh!

amanasifkhalid commented Nov 5, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Nov 5, 2024

Uh oh!

am11 Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

am11 Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

am11 Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amanasifkhalid commented Nov 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

amanasifkhalid commented Nov 5, 2024 •

edited

Loading

am11 Nov 6, 2024 •

edited

Loading

am11 Nov 6, 2024 •

edited

Loading

am11 Nov 6, 2024 •

edited

Loading