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

Building certigo with Go tip can cause panics with 'certigo connect' in certigo/lib.explainCipher #184

Closed
siebenmann opened this issue Jun 7, 2019 · 4 comments
Assignees
Labels
bug

Comments

@siebenmann
Copy link

@siebenmann siebenmann commented Jun 7, 2019

The illustration and reproduction:

$ certigo connect www.google.com:https
panic: runtime error: index out of range [1] with length 1

goroutine 1 [running]:
github.com/square/certigo/lib.explainCipher(0xc00035e3e0, 0xc, 0xc00035e3e0, 0xc, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
        /homes/hawklords/cks/go/src/github.com/square/certigo/lib/tls.go:182 +0x1e3
github.com/square/certigo/lib.EncodeTLSInfoToText(0xc0001222c0, 0x0, 0x2, 0x0)
        /homes/hawklords/cks/go/src/github.com/square/certigo/lib/tls.go:62 +0x148
main.main()
        /homes/hawklords/cks/go/src/github.com/square/certigo/main.go:162 +0x667

The problem is that explainCipher() has a hard-coded list of ciphers and it assumes that this list covers all ciphers that Go's TLS code can ever use. In Go tip, the list of supported TLS ciphers has expanded (to include TLS 1.3 ciphers, such as TLS_AES_128_GCM_SHA256, which is what I believe is being negociated here), but the cipher is not in the list and so explainCipher winds up trying to slice up a null string and creating a bad index.

One fix would be to add the new Go supported ciphers to the list. This is a bit tricky, since their tls. values are not defined in current released Go versions, so they would have to be added by explicit hex value with comments. Another would be to make explainCipher cope (somehow) with ciphers not in the cipherSuites map.

(I believe that there is probably a similar issue with the tlsVersions map, which currently doesn't include TLS 1.3.)

The build issue might be dealt with by pulling both tlsVersions and cipherSuites out into multiple separate files, each built only on Go versions with the necessary constants defined.

(It is a real pity that crypto/tls does not export any name mapping information in its public API, because it sticks programs and code like this with a tough job and a tough set of tradeoffs. But I assume that the Go authors have their reasons.)

@csstaub csstaub self-assigned this Jun 7, 2019
@csstaub csstaub added the bug label Jun 7, 2019
@csstaub

This comment has been minimized.

Copy link
Contributor

@csstaub csstaub commented Jun 7, 2019

Thank you for the detailed info. I will take a look at this.

@csstaub

This comment has been minimized.

Copy link
Contributor

@csstaub csstaub commented Jun 7, 2019

This should address the panic: #185

It will simply print UNKNOWN_{HEX} for unknown ciphers. We can add those into the ciphers list in the future to provide better output.

@mcpherrinm

This comment has been minimized.

Copy link
Collaborator

@mcpherrinm mcpherrinm commented Sep 3, 2019

@csstaub this is fixed, right?

@csstaub

This comment has been minimized.

Copy link
Contributor

@csstaub csstaub commented Sep 3, 2019

Yep, I believe so.

@mcpherrinm mcpherrinm closed this Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.