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.

Sign up for GitHub

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

Envoy crashes when processing a TLS certificate with no SANs or a CN #9182

Closed
erwbgy opened this issue Dec 2, 2019 · 3 comments · Fixed by #9844
Closed

Envoy crashes when processing a TLS certificate with no SANs or a CN #9182

erwbgy opened this issue Dec 2, 2019 · 3 comments · Fixed by #9844
Labels
enhancement Feature requests. Not bugs or questions.

Comments

@erwbgy
Copy link

erwbgy commented Dec 2, 2019

Envoy crashes when processing a TLS certificate that does not have SubjectAltNames or a CN field in the Subject.

This looks to be intentional: https://github.com/envoyproxy/envoy/blob/v1.11.2/source/extensions/transport_sockets/tls/context_impl.cc#L838

While there is no valid use case for having a certificate with no SANs or CN it should not be possible to cause a crash by providing a bad certificate. Perhaps this could be an exception instead like when there are no certificates provided:
https://github.com/envoyproxy/envoy/blob/v1.11.2/source/extensions/transport_sockets/tls/context_impl.cc#L744
?
@erwbgy erwbgy mentioned this issue Dec 2, 2019
Closed
@zuercher zuercher added the enhancement Feature requests. Not bugs or questions. label Dec 2, 2019
@jpeach
Copy link
Contributor

jpeach commented Dec 6, 2019

I'm looking at this. I think the right approach is to throw, and ServerContextImpl already does this in some cases. There's also some refactoring needed for other release assertions that should not assert.

Please assign to me?

jpeach added a commit to jpeach/envoy that referenced this issue Dec 9, 2019
@jpeach
tls: reject certificate without either a subject CN or altnames
a01a8ec 
A server certificate must have either a subject CN or supported subject
alternative names. Certificates that have neither are a configuration
error, not an assertion, so check for this condition and throw an
exception rather than asserting.

Fix up the TLS certificate assertions to log the last libcrypto error
when we do actually assert, so that we can have get hint as to the
underlying problem. Add release assertion messages to describe other
assertion cases.

This fixes envoyproxy#9182.

Signed-off-by: James Peach <[email protected]>
@jpeach jpeach mentioned this issue Dec 9, 2019
Closed
jpeach added a commit to jpeach/envoy that referenced this issue Dec 11, 2019
@jpeach
tls: add release assert messages
9d2e7c6 
Update `RELEASE_ASSERT` calls to log the last TLS error, or a
description of the assert condition where it makes more sense.

This updates envoyproxy#9182.

Signed-off-by: James Peach <[email protected]>
@jpeach jpeach mentioned this issue Dec 11, 2019
Merged
jpeach added a commit to jpeach/envoy that referenced this issue Jan 4, 2020
@jpeach
tls: refactor to return the session context ID
9d6bfb8 
Refactor the TLS session context ID generation to return the ID by
value. This separates the buffers that are used for intermediate
hashes from the buffer that stores the final session context ID hash.

This updates envoyproxy#9182.

Signed-off-by: James Peach <[email protected]>
@jpeach jpeach mentioned this issue Jan 4, 2020
Merged
@stale
Copy link

stale bot commented Jan 5, 2020

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 5, 2020
@jpeach
Copy link
Contributor

jpeach commented Jan 5, 2020

Not stale, see related PRs.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 5, 2020
lizan pushed a commit that referenced this issue Jan 27, 2020
@jpeach @lizan
tls: refactor to return the session context ID (#9558)
6799eec 
Refactor the TLS session context ID generation to return the ID by
value. This separates the buffers that are used for intermediate
hashes from the buffer that stores the final session context ID hash.

This updates #9182.

Signed-off-by: James Peach <[email protected]>
jpeach added a commit to jpeach/envoy that referenced this issue Jan 27, 2020
@jpeach
tls: reject certificate without either a subject CN or altnames
357ca59 
A server certificate must have either a subject CN or supported subject
alternative names. Certificates that have neither are a configuration
error, not an assertion, so check for this condition and throw an
exception rather than asserting.

This fixes envoyproxy#9182.

Signed-off-by: James Peach <[email protected]>
@jpeach jpeach mentioned this issue Jan 27, 2020
Merged
lizan pushed a commit that referenced this issue Jan 31, 2020
@jpeach
tls: reject certificate without either a subject CN or altnames (#9844)
46eb084 
A server certificate must have either a subject CN or supported subject
alternative names. Certificates that have neither are a configuration
error, not an assertion, so check for this condition and throw an
exception rather than asserting.

Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes:
Fixes #9182 

Signed-off-by: James Peach <[email protected]>
@youngnick youngnick mentioned this issue Nov 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Feature requests. Not bugs or questions.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

tls: reject certificate without either a subject CN or altnames jpeach/envoy
3 participants
@jpeach @erwbgy @zuercher