Comment 1 Kyle Evans 2020-05-21 18:37:03 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>

Comment 3 Kyle Evans 2020-05-21 19:49:42 UTC

(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. =-)

Comment 4 Kyle Evans 2020-08-24 01:47:23 UTC

(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

Comment 7 Kyle Evans 2020-08-24 11:47:12 UTC

(In reply to Michael Osipov from comment #6

D'oh! Thanks, will address some of this tonight.

Comment 8 Kyle Evans 2020-08-24 14:27:47 UTC

(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?

Comment 10 Kyle Evans 2020-08-24 15:11:47 UTC

(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.

Comment 13 Kyle Evans 2020-09-02 02:15:49 UTC

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.

Comment 15 commit-hook 2020-09-09 09:08:41 UTC

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

Comment 16 commit-hook 2020-09-13 01:10:02 UTC

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

Comment 17 commit-hook 2020-09-13 02:18:17 UTC

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

Comment 18 Kyle Evans 2020-09-13 02:19:30 UTC

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!