Bug 246614
| Summary: | certctl(8) silently overwrites certs with same subjects | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Michael Osipov <michael.osipov> | ||||
| Component: | bin | Assignee: | Kyle Evans <kevans> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | bugs, kevans, michael.osipov, pi | ||||
| Priority: | --- | Flags: | kevans:
mfc-stable12+
kevans: mfc-stable11+ |
||||
| Version: | 12.1-STABLE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| URL: | https://reviews.freebsd.org/D26167 | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 228913, 274019 | ||||||
| Attachments: |
|
||||||
|
Description
Michael Osipov
2020-05-20 19:51:58 UTC
Created attachment 214734 [details]
git(1) diff against base
Here's a tentative diff; there's some other WIP that I've already created one merge conflict with, I'm holding off on committing this until that work is complete (I wouldn't expect this to take long).
There are several issues with the patch: * The term "serial" is already taken: by the serial number embedded in the cert as well as serialNumber as part of the DN. c_rehash talks about decimal digit. Maybe "get_decimal" is maybe better? * While links are created correctly as it seems: > Reading siemens-cert-14.crt > Adding 8dc03e53.0 to trust store > Reading siemens-cert-15.crt > Adding 8dc03e53.1 to trust store * 'certctl list' does not show any of them because of: > for CFILE in *.0; do You likely will need to add *.1, *.2, ..., *.9 * There is another conceptional issue: *.n is only for the hashed links, not fo scanning, see https://www.openssl.org/docs/man1.1.1/man1/c_rehash.html. * Please also note that the hashed links for CRLs need to be in <hash>.r<D> (In reply to Michael Osipov from comment #2) Acknowledged- I'll fix the patch after WIP lands and wrangle you into a formal review on Phabricator. Thanks for the review. =-) (In reply to Kyle Evans from comment #3) I've pushed this diff into https://reviews.freebsd.org/D26167 and tagged a user whose name is listed as "Michael Osipov" as a reviewer -- the e-mail address doesn't match, but I assumed this was a personal address. (In reply to Kyle Evans from comment #4) Looking through. Note that I do not have access to Phabricator anymore because login via Google is rejected. Already notified the Phabricator admin. Went through the given diff, there are still issues with it:
* serial should be turned into decimal to avoid confusion
* create_blacklisted() is completely ill-designed for several reasons:
** When processing all links must be purged first
** Blacklisted certs should not be linked at all
** using <hash>.r<digit> is wrong because the r suffix is solely reserved for CRLs. Look into c_rehash: elsif($hdr eq "X509 CRL") {$is_crl = 1;..}
BUT the rehashing does work:
> root@deblndw011x:/etc/ssl/certs
> # ll | grep 8dc03e53
> lrwxr-xr-x 1 root wheel 52 2020-08-24 09:54 8dc03e53.0@ -> ../../../usr/local/etc/ssl/certs/siemens-cert-14.crt
> lrwxr-xr-x 1 root wheel 52 2020-08-24 09:54 8dc03e53.1@ -> ../../../usr/local/etc/ssl/certs/siemens-cert-15.crt
> root@deblndw011x:/etc/ssl/certs
> # certctl list | grep "Siemens Internet CA V1.0"
> 8dc03e53.0 Siemens Internet CA V1.0
(In reply to Michael Osipov from comment #6 D'oh! Thanks, will address some of this tonight. (In reply to Michael Osipov from comment #6) > * create_blacklisted() is completely ill-designed for several reasons: > ** When processing all links must be purged first > ** Blacklisted certs should not be linked at all > ** using <hash>.r<digit> is wrong because the r suffix is solely reserved for CRLs. Look into c_rehash: elsif($hdr eq "X509 CRL") {$is_crl = 1;..} They shouldn't be linked, so they should probably just retain their original name and get copied in rather than messing with <hash>.<digit> notation, right? (In reply to Kyle Evans from comment #8) Not even that. The origin c_rehash is not aware of blacklists, only CRLs. So the idea behind a blacklist is that you exclude specific certs from installing. This means that if a cert exists in the blackist and in the source, the hash link is never created (or removed) is /etc/ssl/certs. They must be skipped. When the link is established, OpenSSL assumes that you trust that entity. If you don't want to work with blacklists, use CRL. I don't see a reason to copy with the original name. What benefit should that give? (In reply to Michael Osipov from comment #9) Ah, OK, I see what you mean. So really, `certctl blacklist` should probably just be adding to /usr/share/certs/blacklisted and pulling any newly-blacklisted certs from /etc/ssl/certs. create_blacklisted goes away and create_trusted_link needs to be revised to figure out from /usr/share/certs/blacklisted if the link should be installed or not, if I understand correctly. (In reply to Kyle Evans from comment #10) Correct. I would start populate blacklist first and then generate links. Care must be taken if <digit> diverge, say you have n certs in blacklist with the same subject hash, great care must be taken when creating links that the correct certs is excluded from linking (https://www.openssl.org/docs/man1.1.0/man3/SSL_CTX_load_verify_locations.html). I think the general approach is to calculate a hash on the ASN.1 DER-encoded form of the ticket which will be truly unique. No ticket, certificate. Typo. I've updated the review to more thoroughly remove the 'serial' nomenclature and fix the problem with list and a couple other spots. I'm punting on the blacklist revamp for now, but I've slapped a band-aid on the blacklist functionality so that it least kind of works. For checking if a cert is blacklisted, we now grab all /etc/ssl/blacklisted/$hash.* and do a hard diff -q to see if it's the cert we care about. Future work will likely completely rewrite certctl in (f)lua so that we can optimize and fix this. (In reply to Kyle Evans from comment #13) Finally was able to leave a few comments. A commit references this bug: Author: kevans Date: Wed Sep 9 09:08:09 UTC 2020 New revision: 365500 URL: https://svnweb.freebsd.org/changeset/base/365500 Log: certctl: fix hashed link generation with duplicate subjects Currently, certctl rehash will just keep clobbering .0 rather than incrementing the suffix upon encountering a duplicate. Do this, and do it for blacklisted certs as well. This also improves the situation with the blacklist to be a little less flakey, comparing cert fingerprints for all certs with a matching subject hash in the blacklist to determine if the cert we're looking at can be installed. Future work needs to completely revamp the blacklist to align more with how it's described in PR 246614. In particular, /etc/ssl/blacklisted should go away to avoid potential confusion -- OpenSSL will not read it, it's basically certctl internal. PR: 246614 Reviewed by: Michael Osipov <michael.osipov siemens com> Tested by: Michael Osipov With suggestions from: Michael Osipov MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D26167 Changes: head/usr.sbin/certctl/certctl.sh A commit references this bug: Author: kevans Date: Sun Sep 13 01:09:23 UTC 2020 New revision: 365681 URL: https://svnweb.freebsd.org/changeset/base/365681 Log: MFC r365500: certctl: fix hashed link generation with duplicate subjects Currently, certctl rehash will just keep clobbering .0 rather than incrementing the suffix upon encountering a duplicate. Do this, and do it for blacklisted certs as well. This also improves the situation with the blacklist to be a little less flakey, comparing cert fingerprints for all certs with a matching subject hash in the blacklist to determine if the cert we're looking at can be installed. Future work needs to completely revamp the blacklist to align more with how it's described in PR 246614. In particular, /etc/ssl/blacklisted should go away to avoid potential confusion -- OpenSSL will not read it, it's basically certctl internal. PR: 246614 Changes: _U stable/11/ stable/11/usr.sbin/certctl/certctl.sh _U stable/12/ stable/12/usr.sbin/certctl/certctl.sh A commit references this bug: Author: kevans Date: Sun Sep 13 02:17:18 UTC 2020 New revision: 365683 URL: https://svnweb.freebsd.org/changeset/base/365683 Log: MFS r365681: certctl: fix hashed link generation with duplicate subjects Currently, certctl rehash will just keep clobbering .0 rather than incrementing the suffix upon encountering a duplicate. Do this, and do it for blacklisted certs as well. This also improves the situation with the blacklist to be a little less flakey, comparing cert fingerprints for all certs with a matching subject hash in the blacklist to determine if the cert we're looking at can be installed. Future work needs to completely revamp the blacklist to align more with how it's described in PR 246614. In particular, /etc/ssl/blacklisted should go away to avoid potential confusion -- OpenSSL will not read it, it's basically certctl internal. PR: 246614 Approved by: re (gjb) Changes: _U releng/12.2/ releng/12.2/usr.sbin/certctl/certctl.sh Merged all the way to releng/12.2; thanks! Will follow up later with a review when we're ready to fix the blacklist situation. (In reply to Kyle Evans from comment #18) Awesome, thanks! |
