Skip to content

Commit

Permalink
crypto: check keyBlockDER for nil
Browse files Browse the repository at this point in the history
I got following panic while Caddy was running:

2021/10/26 08:06:34 panic: certificate worker: runtime error: invalid memory address or nil pointer dereference
goroutine 43 [running]:
github.com/caddyserver/certmagic.(*jobManager).worker.func1()
	github.com/caddyserver/certmagic@v0.14.5/async.go:58 +0x65
panic({0x145d400, 0x23d6c50})
	runtime/panic.go:1038 +0x215
github.com/caddyserver/certmagic.decodePrivateKey({0xc000738c00, 0x0, 0x0})
	github.com/caddyserver/certmagic@v0.14.5/crypto.go:75 +0x2a
github.com/caddyserver/certmagic.(*Config).reusePrivateKey(0xc0003b77c0, {0xc0003b1640, 0x32})
	github.com/caddyserver/certmagic@v0.14.5/config.go:602 +0x2b9
github.com/caddyserver/certmagic.(*Config).obtainCert.func2({0x190d3b8, 0xc000655920})
	github.com/caddyserver/certmagic@v0.14.5/config.go:487 +0x1d6
github.com/caddyserver/certmagic.doWithRetry({0x190d310, 0xc0000b0440}, 0xc00003bd40, 0xc0007afba8)
	github.com/caddyserver/certmagic@v0.14.5/async.go:106 +0x1cc
github.com/caddyserver/certmagic.(*Config).obtainCert(0xc0003b77c0, {0x190d310, 0xc0000b0440}, {0xc0003b1640, 0x32}, 0x0)
	github.com/caddyserver/certmagic@v0.14.5/config.go:572 +0x58e
github.com/caddyserver/certmagic.(*Config).ObtainCertAsync(...)
	github.com/caddyserver/certmagic@v0.14.5/config.go:427
github.com/caddyserver/certmagic.(*Config).manageOne.func1()
	github.com/caddyserver/certmagic@v0.14.5/config.go:332 +0x6f
github.com/caddyserver/certmagic.(*jobManager).worker(0x23e0c60)
	github.com/caddyserver/certmagic@v0.14.5/async.go:73 +0x112
created by github.com/caddyserver/certmagic.(*jobManager).Submit
	github.com/caddyserver/certmagic@v0.14.5/async.go:50 +0x288

According to Go documentation: https://pkg.go.dev/encoding/pem#Decode
p can be nil (first parameter returned) and so it should be checked
before continuing as per this example:
https://pkg.go.dev/encoding/pem#example-Decode

I also added a test to verify that the fix works. Running the test
without the fix causes a panic.

Test: go test -count=1 './...'
  • Loading branch information
simaotwx committed Oct 26, 2021
1 parent c17cc71 commit 0335715
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 0 deletions.
4 changes: 4 additions & 0 deletions crypto.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ func encodePrivateKey(key crypto.PrivateKey) ([]byte, error) {
func decodePrivateKey(keyPEMBytes []byte) (crypto.Signer, error) {
keyBlockDER, _ := pem.Decode(keyPEMBytes)

if keyBlockDER == nil {
return nil, fmt.Errorf("failed to decode PEM block containing private key")
}

if keyBlockDER.Type != "PRIVATE KEY" && !strings.HasSuffix(keyBlockDER.Type, " PRIVATE KEY") {
return nil, fmt.Errorf("unknown PEM header %q", keyBlockDER.Type)
}
Expand Down
6 changes: 6 additions & 0 deletions crypto_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ func TestEncodeDecodeRSAPrivateKey(t *testing.T) {
t.Error("error loading private key:", err)
}

// test load (should fail)
_, err = decodePrivateKey(savedBytes[2:])
if err == nil {
t.Error("loading private key should have failed")
}

// verify loaded key is correct
if !privateKeysSame(privateKey, loadedKey) {
t.Error("Expected key bytes to be the same, but they weren't")
Expand Down

0 comments on commit 0335715

Please sign in to comment.