Navigation Menu

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

Jump to bottom

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

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

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

niallnsec opened this issue Jun 2, 2020 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone
Go1.15

Comments

@niallnsec
Copy link

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 NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Jun 2, 2020
@dmitshur dmitshur added this to the Backlog milestone Jun 2, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Jun 2, 2020

/cc @FiloSottile @katiehockman

@FiloSottile FiloSottile self-assigned this Jun 2, 2020
This was referenced Jul 14, 2020
Closed
Closed
@gopherbot
Copy link

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

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 14, 2020
@dmitshur dmitshur modified the milestones: Backlog, Go1.15 Jul 14, 2020
gopherbot pushed a commit that referenced this issue Jul 14, 2020
@FiloSottile @katiehockman
[release-branch.go1.13-security] crypto/x509: respect VerifyOptions.K…
4a4c8d3 
…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 hashicorp/terraform-provider-aws that referenced this issue Jul 14, 2020
@bflad
provider: Bump Go version to 1.13.13
c23b5d2 
Reference: golang/go#39360
@bflad bflad mentioned this issue Jul 14, 2020
Merged
gopherbot pushed a commit that referenced this issue Jul 14, 2020
@FiloSottile @katiehockman
[release-branch.go1.14-security] crypto/x509: respect VerifyOptions.K…
9c0a6ce 
…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 hashicorp/terraform-provider-aws that referenced this issue Jul 14, 2020
@bflad
provider: Bump Go version to 1.13.13 (#14176)
d274cec 
Reference: golang/go#39360
@FiloSottile
Copy link
Contributor

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;

@FiloSottile FiloSottile mentioned this issue Mar 10, 2021
Closed
@golang golang locked and limited conversation to collaborators Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
4 participants
@FiloSottile @dmitshur @gopherbot @niallnsec