Skip to content

schannel: check schannel_sha256sum() success, and more#21739

Closed
vszakats wants to merge 9 commits into
curl:masterfrom
vszakats:schmaybe
Closed

schannel: check schannel_sha256sum() success, and more#21739
vszakats wants to merge 9 commits into
curl:masterfrom
vszakats:schmaybe

Conversation

@vszakats

@vszakats vszakats commented May 24, 2026

Copy link
Copy Markdown
Member

Also:

  • support 4GiB+ SHA-256 digest inputs.
  • check CryptGetHashParam() output size.
  • avoid overwriting existing digest when new digest calculation fails.
  • avoid adding digest hash element on failure.

https://github.com/curl/curl/pull/21739/files?w=1

@vszakats vszakats marked this pull request as draft May 24, 2026 13:02
@github-actions github-actions Bot added TLS Windows Windows-specific labels May 24, 2026
@vszakats vszakats requested a review from Copilot May 24, 2026 13:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Schannel SHA-256 helper path to propagate failures from the underlying WinCrypt hashing routines, and uses that status when computing digests for CA cache validation.

Changes:

  • Change schannel_checksum() to return a CURLcode and report success/failure.
  • Make schannel_sha256sum() return the checksum result instead of always CURLE_OK.
  • Check schannel_sha256sum() return value when comparing/storing CAinfo blob digests for the Schannel cert-store cache.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/vtls/schannel.c Outdated
Comment thread lib/vtls/schannel.c Outdated
Comment thread lib/vtls/schannel.c Outdated
@vszakats vszakats force-pushed the schmaybe branch 2 times, most recently from f313879 to 1cbdb37 Compare May 26, 2026 23:06
@vszakats vszakats requested a review from Copilot May 26, 2026 23:10
@vszakats

Copy link
Copy Markdown
Member Author

augment review
@aisle-analyzer

@aisle-research-bot

aisle-research-bot Bot commented May 26, 2026

Copy link
Copy Markdown

🔒 Aisle Security Analysis

We found 1 low security issue(s) in this PR:

See details in the comment below.

Analyzed PR: #21739 at commit 1cbdb37

Last updated on: 2026-05-26T23:13:50Z

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comment thread lib/vtls/schannel.c Outdated
@augmentcode

augmentcode Bot commented May 26, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR tightens SChannel SHA-256 digest handling so failures are detected and don’t silently produce/propagate incorrect hashes.

Changes:

  • Make schannel_checksum() return a CURLcode and propagate success/failure to callers
  • Validate the returned CryptGetHashParam(HP_HASHVAL) output length matches the algorithm hash size
  • Update schannel_sha256sum() to return the actual status instead of always CURLE_OK
  • In cert-store caching, compute the CA-blob digest into a temporary buffer so an existing cached digest isn’t overwritten on failure
  • Avoid adding/updating cached state (or leaking ownership assumptions) when digest calculation fails

Technical Notes: The cache validation path now fails closed if hashing fails, preventing reuse of a store keyed by an invalid/zeroed digest.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@vszakats

Copy link
Copy Markdown
Member Author

@aisle-analyzer

@aisle-research-bot

aisle-research-bot Bot commented May 26, 2026

Copy link
Copy Markdown

🔒 Aisle Security Analysis

✅ Aisle scan complete — no security issues detected.

Previously reported findings
🔵 Size truncation in SChannel CAinfo blob hashing can cause incorrect certificate-store cache hits

Low severity CWE-197 at lib/vtls/schannel.c:2668-2672

Description

In schannel_checksum(), size_t inputlen is cast to a 32-bit DWORD when passed to CryptHashData(). On 64-bit builds, if inputlen > 0xFFFFFFFF, the cast truncates the length and the hash is computed over only the low 32 bits (or the call fails), so the resulting digest does not uniquely represent the full input.

This digest is used to key the cached certificate store for CURLOPT_CAINFO_BLOB (share->CAinfo_blob_digest). If an application supplies two different CA blobs with the same >4GiB length and the same first (DWORD) bytes, the cache comparison can incorrectly treat them as identical and reuse a certificate store built from different trust material.

Vulnerable code:

if(!CryptHashData(hHash, input, (DWORD)inputlen, 0))
  goto out;

Notes:

  • The blob is application-provided, but this is still a correctness/security footgun in core TLS code; other backends explicitly guard against oversized blobs.

Recommendation

Reject or safely handle inputs larger than DWORD_MAX.

Option A (simplest/safest): fail if inputlen > DWORD_MAX.

if(inputlen > (size_t)DWORD_MAX)
  return CURLE_BAD_FUNCTION_ARGUMENT;
if(!CryptHashData(hHash, input, (DWORD)inputlen, 0))
  goto out;

Option B: hash in chunks to support arbitrary sizes.

size_t off = 0;
while(off < inputlen) {
  DWORD chunk = (DWORD)CURLMIN(inputlen - off, (size_t)DWORD_MAX);
  if(!CryptHashData(hHash, input + off, chunk, 0))
    goto out;
  off += chunk;
}

Also consider adding a similar size guard at the call site using ca_info_blob->len (like other TLS backends do).

Aisle supplements but does not replace security review.

Last updated on: 2026-05-26T23:52:28Z

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@vszakats vszakats marked this pull request as ready for review May 26, 2026 23:56
@vszakats vszakats changed the title schannel: check schannel_sha256sum() success schannel: check schannel_sha256sum() success, and more May 26, 2026
@vszakats vszakats requested a review from Copilot May 27, 2026 08:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@vszakats vszakats closed this in a1baacc May 27, 2026
@vszakats vszakats deleted the schmaybe branch May 27, 2026 14:59
outcast36 pushed a commit to greearb/curl that referenced this pull request Jun 3, 2026
Also:
- support 4GiB+ SHA-256 digest inputs.
- check `CryptGetHashParam()` output size.
- avoid overwriting existing digest when new digest calculation fails.
- avoid adding digest hash element on failure.

Closes curl#21739
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

TLS Windows Windows-specific

Development

Successfully merging this pull request may close these issues.

2 participants