Bug 246614

Summary: certctl(8) silently overwrites certs with same subjects
Product: Base System Reporter: Michael Osipov <michael.osipov>
Component: binAssignee: 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    
Attachments:
Description Flags
git(1) diff against base none

Description Michael Osipov 2020-05-20 19:51:58 UTC
When additional trusted certificates are added with the same subject instead of increasing n in <hash>.n it does overwrite <hash>.0.

Here are two certs with the same subject hash:

subject: CN=Siemens Internet CA V1.0,OU=Copyright (C) Siemens AG 2011 All Rights Reserved,serialNumber=ZZZZZZV0,O=Siemens,C=DE
issuer: CN=Baltimore CyberTrust Root,OU=CyberTrust,O=Baltimore,C=IE
source: Siemens
subject hash: 8dc03e53
fingerprint (SHA-1): 1C:7D:56:40:9E:CB:A4:96:8B:8F:FF:41:78:31:86:06:DE:DB:05:32
fingerprint (SHA-256): 3E:BF:5F:FE:C5:82:D2:7C:69:3D:1B:C3:01:04:A6:3B:BB:FC:36:52:C7:8A:95:02:7E:91:B7:F8:8D:AC:63:45
-----BEGIN CERTIFICATE-----
MIIEkTCCA3mgAwIBAgIQDL0FAAzqeadFvWvsl9xaiDANBgkqhkiG9w0BAQsFADBa
MQswCQYDVQQGEwJJRTESMBAGA1UEChMJQmFsdGltb3JlMRMwEQYDVQQLEwpDeWJl
clRydXN0MSIwIAYDVQQDExlCYWx0aW1vcmUgQ3liZXJUcnVzdCBSb290MB4XDTE2
MDMwMTEyNDMzOFoXDTIxMDMwMTEyNDMzOFowgZExCzAJBgNVBAYTAkRFMRAwDgYD
VQQKDAdTaWVtZW5zMREwDwYDVQQFEwhaWlpaWlpWMDE6MDgGA1UECwwxQ29weXJp
Z2h0IChDKSBTaWVtZW5zIEFHIDIwMTEgQWxsIFJpZ2h0cyBSZXNlcnZlZDEhMB8G
A1UEAwwYU2llbWVucyBJbnRlcm5ldCBDQSBWMS4wMIIBIjANBgkqhkiG9w0BAQEF
AAOCAQ8AMIIBCgKCAQEAtOKu51fv/rScjbH0r0Uol/O96u8qGrtBQ+thN0A0ryzw
sgcU4QAp8nPYt6cRSI+29ysibk4xevVNfWCKOTquYntPOSXmyhXaPOKgJ588zr+1
F1//yODojIn+yDIRDR9mix/Znwa1K6ECXismikPP4GtqHv7Pj9T6QVonMHfntCFB
6fO8NN0akVJBZoS9ejtueypkKrYTAtzrA7R102kcp30UDPtrwtDXCFIvjfVmJmp3
0e2QX2kVhIYx7PtX+qLCVPOMujT3wJQ8tnLCTnRAv6MIgz7Ufp7AFF0TdUvVlHQ0
w6XqQJMgvlY4libFBZgW4hZ146STgsD+uRO1YODa8QIDAQABo4IBGTCCARUwHQYD
VR0OBBYEFBGqTpwKTU4XVhuUVzgB8rc1pEllMB8GA1UdIwQYMBaAFOWdWTCCR1jM
rPoIVDaGezq1BE3wMBIGA1UdEwEB/wQIMAYBAf8CAQEwDgYDVR0PAQH/BAQDAgGG
MDQGCCsGAQUFBwEBBCgwJjAkBggrBgEFBQcwAYYYaHR0cDovL29jc3AuZGlnaWNl
cnQuY29tMDoGA1UdHwQzMDEwL6AtoCuGKWh0dHA6Ly9jcmwzLmRpZ2ljZXJ0LmNv
bS9PbW5pcm9vdDIwMjUuY3JsMD0GA1UdIAQ2MDQwMgYEVR0gADAqMCgGCCsGAQUF
BwIBFhxodHRwczovL3d3dy5kaWdpY2VydC5jb20vQ1BTMA0GCSqGSIb3DQEBCwUA
A4IBAQA2D2N0Cnpk+cXutX6rfjebXl9C/rmOpxjHBEOJcUf4In/wZ8KO2ZaHlKdO
l6Hr8Ui4pZe6N/6lItx3aPhTBEd8buo+ApGtwDJIwB5ExsfDmCq9w7xo6ICb/TRP
Z867M411fhkh0pPqecVLSeqx5To0HM1Pixl7q8BmL4kyN4Oz0J/Uuy/UGuFCL7BF
nIkzUPL8oMdlwnUrWMPeHSOqgVinx3DEc4ysZBQ8lSYcAQj2xGLH+8Bict24VGV3
RsdJ2yCtXbg2H6Vj4R2Gtm4GdyK/kFgjd1aLYSxWD82G9IJPv4EvZsQyOtJqfhPn
Wp3ujLiX4hL9XpsnfGjDqzoU4y1K
-----END CERTIFICATE-----

subject: CN=Siemens Internet CA V1.0,OU=Copyright (C) Siemens AG 2011 All Rights Reserved,serialNumber=ZZZZZZV0,O=Siemens,C=DE
issuer: CN=Baltimore CyberTrust Root,OU=CyberTrust,O=Baltimore,C=IE
source: Siemens
subject hash: 8dc03e53
fingerprint (SHA-1): 04:E8:21:81:E0:9E:4E:DC:46:3C:1F:E2:67:41:60:5C:80:E8:18:11
fingerprint (SHA-256): 24:E5:6F:48:60:44:46:D8:A8:37:3B:43:CA:29:D1:A1:C4:97:72:E5:AA:BA:8B:A7:C1:76:62:BD:60:DA:8D:F6
-----BEGIN CERTIFICATE-----
MIIE4TCCA8mgAwIBAgIEBydWSzANBgkqhkiG9w0BAQUFADBaMQswCQYDVQQGEwJJ
RTESMBAGA1UEChMJQmFsdGltb3JlMRMwEQYDVQQLEwpDeWJlclRydXN0MSIwIAYD
VQQDExlCYWx0aW1vcmUgQ3liZXJUcnVzdCBSb290MB4XDTExMDcxNDE2MTE0MVoX
DTIzMDcxNDE2MTExN1owgZExCzAJBgNVBAYTAkRFMRAwDgYDVQQKDAdTaWVtZW5z
MREwDwYDVQQFEwhaWlpaWlpWMDE6MDgGA1UECwwxQ29weXJpZ2h0IChDKSBTaWVt
ZW5zIEFHIDIwMTEgQWxsIFJpZ2h0cyBSZXNlcnZlZDEhMB8GA1UEAwwYU2llbWVu
cyBJbnRlcm5ldCBDQSBWMS4wMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC
AQEAtOKu51fv/rScjbH0r0Uol/O96u8qGrtBQ+thN0A0ryzwsgcU4QAp8nPYt6cR
SI+29ysibk4xevVNfWCKOTquYntPOSXmyhXaPOKgJ588zr+1F1//yODojIn+yDIR
DR9mix/Znwa1K6ECXismikPP4GtqHv7Pj9T6QVonMHfntCFB6fO8NN0akVJBZoS9
ejtueypkKrYTAtzrA7R102kcp30UDPtrwtDXCFIvjfVmJmp30e2QX2kVhIYx7PtX
+qLCVPOMujT3wJQ8tnLCTnRAv6MIgz7Ufp7AFF0TdUvVlHQ0w6XqQJMgvlY4libF
BZgW4hZ146STgsD+uRO1YODa8QIDAQABo4IBdTCCAXEwEgYDVR0TAQH/BAgwBgEB
/wIBATBbBgNVHSAEVDBSMEgGCSsGAQQBsT4BADA7MDkGCCsGAQUFBwIBFi1odHRw
Oi8vY3liZXJ0cnVzdC5vbW5pcm9vdC5jb20vcmVwb3NpdG9yeS5jZm0wBgYEVR0g
ADAOBgNVHQ8BAf8EBAMCAQYwgYUGA1UdIwR+MHyAFOWdWTCCR1jMrPoIVDaGezq1
BE3woV6kXDBaMQswCQYDVQQGEwJJRTESMBAGA1UEChMJQmFsdGltb3JlMRMwEQYD
VQQLEwpDeWJlclRydXN0MSIwIAYDVQQDExlCYWx0aW1vcmUgQ3liZXJUcnVzdCBS
b290ggQCAAC5MEcGA1UdHwRAMD4wPKA6oDiGNmh0dHA6Ly93d3cucHVibGljLXRy
dXN0LmNvbS9jZ2ktYmluL0NSTC8yMDI1MDEvY2RwLmNybDAdBgNVHQ4EFgQUEapO
nApNThdWG5RXOAHytzWkSWUwDQYJKoZIhvcNAQEFBQADggEBAAdvcLAkY0C51Hm6
hqgFctsCDXNy+CW0cuIwe2SV4apynjcMU7RB30cnOX7JGiL//o5VlpdGYeSIiI42
N3tZJQ6JURagL5mcBcCMAwSIE8cBa9RiSVW4KfMmun5ldN6q1FXJ28OnxxsJkZLZ
h2Y1J1R9WGxJbYRDJvrRH0icJKJmlD+k4h/EeyC75K2x2xQO5XIKJHUWH/+ChgpV
TcawNX3uH9XrPvaSRr+Troj6+iyE5nWXPolmnlyUrg1rJsMvY8iBtMUUxst10Y/V
nhIcGv8FWoPpaRJVVOgGRmwA7k4TsP3Yv4cQuANX0fQtCRrNq3BKMkUaznsHx90C
6sbFZEM=
-----END CERTIFICATE-----
Comment 1 Kyle Evans freebsd_committer 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).
Comment 2 Michael Osipov 2020-05-21 19:11:42 UTC
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 freebsd_committer 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 freebsd_committer 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.
Comment 5 Michael Osipov 2020-08-24 07:33:20 UTC
(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.
Comment 6 Michael Osipov 2020-08-24 07:57:56 UTC
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 freebsd_committer 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 freebsd_committer 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?
Comment 9 Michael Osipov 2020-08-24 15:01:08 UTC
(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 freebsd_committer 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.
Comment 11 Michael Osipov 2020-08-24 15:49:37 UTC
(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.
Comment 12 Michael Osipov 2020-08-24 16:09:20 UTC
No ticket, certificate. Typo.
Comment 13 Kyle Evans freebsd_committer 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.
Comment 14 Michael Osipov 2020-09-07 13:47:13 UTC
(In reply to Kyle Evans from comment #13)

Finally was able to leave a few comments.
Comment 15 commit-hook freebsd_committer 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 freebsd_committer 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 freebsd_committer 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 freebsd_committer 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.
Comment 19 Michael Osipov 2020-09-13 18:44:30 UTC
(In reply to Kyle Evans from comment #18)

Awesome, thanks!