Skip to content

Fix incorrect key_strength for Windows certificates - #8989

Merged
zwass merged 2 commits into
osquery:masterfrom
munzzyy:fix-windows-cert-key-strength
Jul 29, 2026
Merged

Fix incorrect key_strength for Windows certificates#8989
zwass merged 2 commits into
osquery:masterfrom
munzzyy:fix-windows-cert-key-strength

Conversation

@munzzyy

@munzzyy munzzyy commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #8968

certificates.cpp computes key_strength on Windows as:

r["key_strength"] = INTEGER(
    (certContext->pCertInfo->SubjectPublicKeyInfo.PublicKey.cbData) * 8);

SubjectPublicKeyInfo.PublicKey.cbData is the byte length of the raw encoded public key BLOB (the CRYPT_BIT_BLOB from the certificate's DER), not the key's bit strength. For RSA that BLOB is the DER SEQUENCE { modulus INTEGER, publicExponent INTEGER }, so cbData * 8 also counts the ASN.1 tag/length bytes, the modulus's leading zero-padding byte (added whenever the high bit of the modulus is set, which is most of the time), and the exponent bytes. None of that belongs in a bit-strength number. That's exactly the 2048 -> 2160 and P-256 -> 776 the issue reports.

Switched to CertGetPublicKeyLength, which takes the encoding type and a CERT_PUBLIC_KEY_INFO* and decodes the BLOB itself to return the real bit length, or 0 with GetLastError() set if it can't. That matches what the docs say the function is for, and it's also the fix the issue itself points at. On a failure it logs at VLOG(1) and falls through to 0, the same pattern the neighboring getKeyUsage/getCertCtxProp calls in this file already use rather than throwing or leaving the field unset.

One thing I want to flag rather than quietly decide myself: specs/certificates.table documents key_strength as "Key size used for RSA/DSA, or curve name" - on macOS/Linux, ECC certs report the curve name (prime256v1, etc.), not a bit count, going back to #1751. Windows never implemented that branch; it's always returned some kind of number for ECC, just a wrong one. CertGetPublicKeyLength keeps that a number, it just makes it the correct one (256 instead of 776 for P-256). I didn't try to add curve-name detection to bring Windows in line with the other platforms, since that's a bigger change than what's broken here and I have no way to verify curve-name output against a real EC cert without a Windows box. Happy to follow up on that separately if it's wanted.

Also didn't pull in the wincert_utils.cpp helpers referenced in the issue (#8967) - that PR hasn't merged, so I kept this self-contained instead of depending on unmerged code.

I don't have a Windows build environment here, so I can't run this or the existing table tests. Verified by reading certificates.cpp, the CertGetPublicKeyLength docs above, and cross-checking against openssl_utils.cpp's RSA/DSA branch (EVP_PKEY_size(...) * 8) for how the other platforms already do this correctly. wincrypt.h is already included in this file so no new headers are needed. Relying on CI to confirm it builds and passes the existing certificates table tests on Windows.

key_strength was computed as
SubjectPublicKeyInfo.PublicKey.cbData * 8 - the byte length of the
raw encoded public key BLOB, times 8. That BLOB is the DER encoding
of the key (for RSA, a SEQUENCE of the modulus and exponent
INTEGERs), so the count includes ASN.1 tag/length bytes, the
modulus's leading zero padding byte, and the exponent bytes, none of
which are part of the key's actual bit strength. A 2048-bit RSA key
comes out as 2160, and a P-256 ECC key comes out as 776.

Switched to CertGetPublicKeyLength, which decodes the BLOB and
returns the real bit length of the key, matching what the column is
documented to hold. Logs and falls back to 0 on failure the same way
the neighboring key_usage/property lookups in this file already do.

Kept this self-contained rather than pulling in the wincert_utils
helpers from osquery#8967, since that PR hasn't merged yet.

Fixes osquery#8968
@munzzyy
munzzyy requested review from a team as code owners July 9, 2026 20:20
@zwass
zwass requested a review from Copilot July 10, 2026 07:52
Comment on lines +483 to 485
}
r["key_strength"] = INTEGER(keyStrength);

@zwass zwass Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It might be nice to only set it if there's a valid value. That way the table would return an empty column rather than explicit 0.

Suggested change
}
r["key_strength"] = INTEGER(keyStrength);
} else {
r["key_strength"] = INTEGER(keyStrength);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, that's cleaner. Pushed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes the Windows implementation of the certificates table so the key_strength column reports the actual public key bit length rather than deriving it from the encoded key blob size (which included ASN.1 overhead and produced incorrect values like 2160 for RSA-2048 and 776 for P-256).

Changes:

  • Replace the cbData * 8 computation with CertGetPublicKeyLength(...) to obtain the correct key size in bits.
  • Add a low-verbosity log message when CertGetPublicKeyLength fails and returns 0, while still populating the field with 0 (consistent with the file’s existing “log and continue” pattern).

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

@zwass zwass left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@zwass
zwass merged commit d798ab3 into osquery:master Jul 29, 2026
24 checks passed
pascalrobert pushed a commit to CollabInfra/osquery that referenced this pull request Jul 29, 2026
Fixes osquery#8968

`certificates.cpp` computes `key_strength` on Windows as:

```cpp
r["key_strength"] = INTEGER(
    (certContext->pCertInfo->SubjectPublicKeyInfo.PublicKey.cbData) * 8);
```

`SubjectPublicKeyInfo.PublicKey.cbData` is the byte length of the raw
encoded public key BLOB (the `CRYPT_BIT_BLOB` from the certificate's
DER), not the key's bit strength. For RSA that BLOB is the DER `SEQUENCE
{ modulus INTEGER, publicExponent INTEGER }`, so `cbData * 8` also
counts the ASN.1 tag/length bytes, the modulus's leading zero-padding
byte (added whenever the high bit of the modulus is set, which is most
of the time), and the exponent bytes. None of that belongs in a
bit-strength number. That's exactly the 2048 -> 2160 and P-256 -> 776
the issue reports.

Switched to
[`CertGetPublicKeyLength`](https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certgetpublickeylength),
which takes the encoding type and a `CERT_PUBLIC_KEY_INFO*` and decodes
the BLOB itself to return the real bit length, or 0 with
`GetLastError()` set if it can't. That matches what the docs say the
function is for, and it's also the fix the issue itself points at. On a
failure it logs at `VLOG(1)` and falls through to 0, the same pattern
the neighboring `getKeyUsage`/`getCertCtxProp` calls in this file
already use rather than throwing or leaving the field unset.

One thing I want to flag rather than quietly decide myself:
`specs/certificates.table` documents `key_strength` as "Key size used
for RSA/DSA, or curve name" - on macOS/Linux, ECC certs report the curve
name (`prime256v1`, etc.), not a bit count, going back to osquery#1751. Windows
never implemented that branch; it's always returned some kind of number
for ECC, just a wrong one. `CertGetPublicKeyLength` keeps that a number,
it just makes it the correct one (256 instead of 776 for P-256). I
didn't try to add curve-name detection to bring Windows in line with the
other platforms, since that's a bigger change than what's broken here
and I have no way to verify curve-name output against a real EC cert
without a Windows box. Happy to follow up on that separately if it's
wanted.

Also didn't pull in the `wincert_utils.cpp` helpers referenced in the
issue (osquery#8967) - that PR hasn't merged, so I kept this self-contained
instead of depending on unmerged code.

I don't have a Windows build environment here, so I can't run this or
the existing table tests. Verified by reading `certificates.cpp`, the
`CertGetPublicKeyLength` docs above, and cross-checking against
`openssl_utils.cpp`'s RSA/DSA branch (`EVP_PKEY_size(...) * 8`) for how
the other platforms already do this correctly. `wincrypt.h` is already
included in this file so no new headers are needed. Relying on CI to
confirm it builds and passes the existing `certificates` table tests on
Windows.
pascalrobert pushed a commit to CollabInfra/osquery that referenced this pull request Aug 4, 2026
Fixes osquery#8968

`certificates.cpp` computes `key_strength` on Windows as:

```cpp
r["key_strength"] = INTEGER(
    (certContext->pCertInfo->SubjectPublicKeyInfo.PublicKey.cbData) * 8);
```

`SubjectPublicKeyInfo.PublicKey.cbData` is the byte length of the raw
encoded public key BLOB (the `CRYPT_BIT_BLOB` from the certificate's
DER), not the key's bit strength. For RSA that BLOB is the DER `SEQUENCE
{ modulus INTEGER, publicExponent INTEGER }`, so `cbData * 8` also
counts the ASN.1 tag/length bytes, the modulus's leading zero-padding
byte (added whenever the high bit of the modulus is set, which is most
of the time), and the exponent bytes. None of that belongs in a
bit-strength number. That's exactly the 2048 -> 2160 and P-256 -> 776
the issue reports.

Switched to
[`CertGetPublicKeyLength`](https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certgetpublickeylength),
which takes the encoding type and a `CERT_PUBLIC_KEY_INFO*` and decodes
the BLOB itself to return the real bit length, or 0 with
`GetLastError()` set if it can't. That matches what the docs say the
function is for, and it's also the fix the issue itself points at. On a
failure it logs at `VLOG(1)` and falls through to 0, the same pattern
the neighboring `getKeyUsage`/`getCertCtxProp` calls in this file
already use rather than throwing or leaving the field unset.

One thing I want to flag rather than quietly decide myself:
`specs/certificates.table` documents `key_strength` as "Key size used
for RSA/DSA, or curve name" - on macOS/Linux, ECC certs report the curve
name (`prime256v1`, etc.), not a bit count, going back to osquery#1751. Windows
never implemented that branch; it's always returned some kind of number
for ECC, just a wrong one. `CertGetPublicKeyLength` keeps that a number,
it just makes it the correct one (256 instead of 776 for P-256). I
didn't try to add curve-name detection to bring Windows in line with the
other platforms, since that's a bigger change than what's broken here
and I have no way to verify curve-name output against a real EC cert
without a Windows box. Happy to follow up on that separately if it's
wanted.

Also didn't pull in the `wincert_utils.cpp` helpers referenced in the
issue (osquery#8967) - that PR hasn't merged, so I kept this self-contained
instead of depending on unmerged code.

I don't have a Windows build environment here, so I can't run this or
the existing table tests. Verified by reading `certificates.cpp`, the
`CertGetPublicKeyLength` docs above, and cross-checking against
`openssl_utils.cpp`'s RSA/DSA branch (`EVP_PKEY_size(...) * 8`) for how
the other platforms already do this correctly. `wincrypt.h` is already
included in this file so no new headers are needed. Relying on CI to
confirm it builds and passes the existing `certificates` table tests on
Windows.
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.

Incorrect key strength reported for certificates on Windows

3 participants