search

aws / s2n-tls

An implementation of the TLS/SSL protocols
https://aws.github.io/s2n-tls/usage-guide/
Apache License 2.0
4.49k stars 704 forks source link

s2n's PEM Parser should handle Comments in PEM Files #1136

Open alexw91 opened 5 years ago

alexw91 commented 5 years ago

Problem:

CentOS Certificate Authority PEM files include comments which causes s2n's PEM Parser to reject them. See: https://github.com/awslabs/s2n/issues/700#issuecomment-520629683

PEM File: https://gist.github.com/alexw91/502f439535bf9886851d0ba9ce27043a

Example:

# CA name with spaces-and-dashes
-----BEGIN CERTIFICATE-----
...valid PEM contents here...
-----END CERTIFICATE-----

# CA with ----BEGIN CERTIFICATE----? 
-----BEGIN CERTIFICATE-----
...valid PEM contents here...
-----END CERTIFICATE-----

Proposed Solution:

Upgrade s2n's PEM Parser to handle this case.

alexw91 commented 5 years ago

Related to: https://github.com/awslabs/s2n/issues/700

alexw91 commented 5 years ago

If we do treate # as the beginning of a comment, then this case will regress and stop working. 

#-----BEGIN CERTIFICATE-----
...valid PEM contents here...
-----END CERTIFICATE-----
justinboswell commented 5 years ago

If we do treate # as the beginning of a comment, then this case will regress and stop working.

#-----BEGIN CERTIFICATE-----
...valid PEM contents here...
-----END CERTIFICATE-----

Is this case valid in the PEM format? I'd expect that to be garbage.

alexw91 commented 5 years ago

Is this case valid in the PEM format?

It's ambiguous. There's an RFC defining how to encode each individual PEM, but no spec for what is or isn't allowed for a list of PEM's, so people end up doing all sorts of weird things that OpenSSL's parser allows. Right now OpenSSL's parser would silently ignore that cert, but s2n's parser would accept it.

I'd expect that to be garbage.

Agreed that I'd expect it to be treated as garbage too. I'm just calling this change out as a potentially breaking change in s2n's parser. If anyone out in the wild has a PEM with a single character typo (which has happened and has caused an outage before), this change could break them.