Skip to content
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

crypto/x509: Certificate.Verify method seemingly ignoring EKU requirements on Windows #39360

Closed
niallnsec opened this issue Jun 2, 2020 · 3 comments
Assignees
Milestone

Comments

@niallnsec
Copy link

@niallnsec niallnsec commented Jun 2, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.2 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\User\AppData\Local\go-build
set GOENV=C:\Users\User\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GONOPROXY=none
set GONOSUMDB=
set GOOS=windows
set GOPATH=K:\Go
set GOPRIVATE=
set GOPROXY=
set GOROOT=C:\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set GO386=sse2
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m32 -fmessage-length=0 -fdebug-prefix-map=C:\Users\User\AppData\Local\Temp\go-build664969334=/tmp/go-build -gno-record-gcc-switches

What did you do?

When validating an x509 certificate, the KeyUsages value appears to be ignored on Windows.

What did you expect to see?

Certificate validation fail if the certificate chain does not meet the EKU requirements specified in VerifyOptions.

What did you see instead?

The certificate validates successfully despite not meeting the requirements.

I am not sure if this is intended behaviour though, due to the code here: https://golang.org/src/crypto/x509/verify.go?#L749
If the OS is Windows, validation is passed off to the system APIs and the result of that call is returned, meaning the EKU checks at the bottom of the Verify function are never hit.
Following the Windows specific code path, the KeyUsages value of VerifyOptions is not referenced as far as I can see.

I may be missing something here but it seems odd that this field would be ignored only on Windows without there being a note in the documentation.

@dmitshur dmitshur changed the title crypto/x509.Certificate: Verify function seemingly ignoring EKU requirements on Windows crypto/x509: Certificate.Verify method seemingly ignoring EKU requirements on Windows Jun 2, 2020
@dmitshur dmitshur added this to the Backlog milestone Jun 2, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 2, 2020

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 14, 2020

Change https://golang.org/cl/242597 mentions this issue: crypto/x509: respect VerifyOptions.KeyUsages on Windows

@dmitshur dmitshur modified the milestones: Backlog, Go1.15 Jul 14, 2020
@gopherbot gopherbot closed this in 82175e6 Jul 14, 2020
gopherbot pushed a commit that referenced this issue Jul 14, 2020
…eyUsages on Windows

When using the platform verifier on Windows (because Roots is nil) we
were always enforcing server auth EKUs if DNSName was set, and none
otherwise. If an application was setting KeyUsages, they were not being
respected.

Started correctly surfacing IncompatibleUsage errors from the system
verifier, as those are the ones applications will see if they are
affected by this change.

Also refactored verify_test.go to make it easier to add tests for this,
and replaced the EKULeaf chain with a new one that doesn't have a SHA-1
signature.

Thanks to Niall Newman for reporting this.

Fixes #39360
Fixes CVE-2020-14039

Change-Id: If5c00d615f2944f7d57007891aae1307f9571c32
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/774414
Reviewed-by: Katie Hockman <katiehockman@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/793509
Reviewed-by: Filippo Valsorda <valsorda@google.com>
bflad added a commit to terraform-providers/terraform-provider-aws that referenced this issue Jul 14, 2020
Reference: golang/go#39360
gopherbot pushed a commit that referenced this issue Jul 14, 2020
…eyUsages on Windows

When using the platform verifier on Windows (because Roots is nil) we
were always enforcing server auth EKUs if DNSName was set, and none
otherwise. If an application was setting KeyUsages, they were not being
respected.

Started correctly surfacing IncompatibleUsage errors from the system
verifier, as those are the ones applications will see if they are
affected by this change.

Also refactored verify_test.go to make it easier to add tests for this,
and replaced the EKULeaf chain with a new one that doesn't have a SHA-1
signature.

Thanks to Niall Newman for reporting this.

Fixes #39360
Fixes CVE-2020-14039

Change-Id: If5c00d615f2944f7d57007891aae1307f9571c32
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/774414
Reviewed-by: Katie Hockman <katiehockman@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/793511
Reviewed-by: Filippo Valsorda <valsorda@google.com>
bflad added a commit to terraform-providers/terraform-provider-aws that referenced this issue Jul 14, 2020
Reference: golang/go#39360
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jul 16, 2020

For future reference, here is the giant crt.sh query I used to find a test case that had EKUs on the leaf but not on the chain.

WITH cas AS (
  SELECT DISTINCT
    ca_c.CA_ID as ID
  FROM
    ca_certificate AS ca_c
    INNER JOIN ca_trust_purpose AS ca_t_p
      ON ca_t_p.CA_ID = ca_c.CA_ID
    INNER JOIN certificate AS c
      ON ca_c.CERTIFICATE_ID = c.ID
  WHERE
    -- https://github.com/crtsh/certwatch_db/blob/5e310cb/sql/create_schema.sql#L482
    ca_t_p.TRUST_PURPOSE_ID = 1
    -- https://github.com/crtsh/certwatch_db/blob/5e310cb/sql/create_schema.sql#L458
    AND ca_t_p.TRUST_CONTEXT_ID = 1
    -- is not revoked
    AND NOT ca_t_p.ALL_CHAINS_REVOKED_VIA_ONECRL
    -- with a certificate that omits EKUs
    AND NOT x509_hasextension(c.CERTIFICATE, '2.5.29.37')
    -- that is not expired
    AND coalesce(x509_notAfter(c.CERTIFICATE), 'infinity'::timestamp) > NOW()
)
SELECT DISTINCT
  c.ID
FROM
  certificate AS c
  -- signed by an unconstrained CA
  INNER JOIN cas ON c.ISSUER_CA_ID = cas.ID
WHERE
  -- is not expired
  coalesce(x509_notAfter(c.CERTIFICATE), 'infinity'::timestamp) > NOW()
  -- has an email EKU
  AND EXISTS(
    SELECT 1
    FROM x509_extkeyusages(c.CERTIFICATE)
    WHERE x509_extkeyusages = '1.3.6.1.5.5.7.3.4'
  )
  -- doesn't have a serverAuth EKU
  AND NOT EXISTS(
    SELECT 1
    FROM x509_extkeyusages(c.CERTIFICATE)
    WHERE x509_extkeyusages = '1.3.6.1.5.5.7.3.1'
  )
  -- is not a CA certificate itself
  AND NOT EXISTS(
    SELECT 1
    FROM ca_certificate AS ca_c
    WHERE ca_c.CERTIFICATE_ID = c.ID
  )
  -- is not revoked
  AND NOT EXISTS(
    SELECT 1
    FROM crl_revoked AS crl_r
    WHERE
      crl_r.CA_ID = c.ISSUER_CA_ID
      AND crl_r.SERIAL_NUMBER = x509_serialNumber(c.CERTIFICATE)
  )
LIMIT 10;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.