Skip to content

Skip empty/invalid records/certs in MacOS keychain files #22927

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

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

squeek502
Copy link
Collaborator

@squeek502 squeek502 commented Feb 18, 2025

In the original PR that implemented rescanMac (#14325), it included a list of references for the keychain format. Multiple of those references include the checks that are added in this commit, and empirically this fixes the loading of a real keychain file that was previously failing (it had both a record with offset 0 and a record with cert_size 0).

To give more detail: when not skipping a record with offset zero, it would attempt to read the base of the table as a X509CertHeader, but the base of the table is actually a TableHeader. It would then take this misinterpreted X509CertHeader and read N bytes that follow it (where N is equal to the misinterpreted cert_size), and then try to parse those bytes as a DER cert. These bytes are just some arbitrary bytes in the keychain file, though, not the actual start of a cert, so it would fail at some arbitrary point in Certificate.parse.

What this means is that #22701 didn't directly cause a regression, but instead exposed a latent bug, and that these zero-record-offset/zero-cert-size records are seemingly much more common in /Library/Keychains/System.keychain.

Fixes #22870


Specifically, this has fixed the regression for @nurpax detailed in https://ziggit.dev/t/zig-fetch-failing-with-discover-remote-git-server-capabilities-certificatebundleloadfailure/8608

However, this fix does not explain/account for the 'swap the ordering' workaround from #22870 (comment), so this may not be a full solution. Would appreciate @jedisct1 checking to see that this fixes the regression for them.

In the original PR that implemented this (ziglang#14325), it included a list of references for the keychain format. Multiple of those references include the checks that are added in this commit, and empirically this fixes the loading of a real keychain file that was previously failing (it had both a record with offset 0 and a record with cert_size 0).

Fixes ziglang#22870
@squeek502 squeek502 mentioned this pull request Feb 18, 2025
@alexrp alexrp added this to the 0.14.0 milestone Feb 18, 2025
@nurpax
Copy link
Contributor

nurpax commented Feb 18, 2025

Dropped the macos.zig from this PR into my Zig 0.14.0-dev.3187 tree and confirm that this change makes this test case pass:

const std = @import("std");

test "scan for OS-provided certificates" {
    var bundle: std.crypto.Certificate.Bundle = .{};
    defer bundle.deinit(std.testing.allocator);

    try bundle.rescan(std.testing.allocator);
}

@alexrp alexrp merged commit 0779e84 into ziglang:master Feb 18, 2025
9 checks passed
@alexrp
Copy link
Member

alexrp commented Feb 18, 2025

@jedisct1 can you confirm that this fix works for you?

@jedisct1
Copy link
Contributor

@alexrp Yep, looks like it does!

@alexrp alexrp removed this from the 0.14.0 milestone Feb 21, 2025
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.

MacOS CA Bundle regression after b3a11018ae1fe99190fb6fb7ae82a486c40f6f15
4 participants