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

X509->_extractBER() can fail subtly #1542

Closed
LeonMelis opened this issue Oct 21, 2020 · 4 comments
Closed

X509->_extractBER() can fail subtly #1542

LeonMelis opened this issue Oct 21, 2020 · 4 comments

Comments

@LeonMelis
Copy link
Contributor

LeonMelis commented Oct 21, 2020

I discovered a subtle inconsistency bug in the way phpseclib parses a PEM file that contains multiple certificates (like a common certificate chain). From the source I can see that this is not supposed to be supported by phpseclib, but under certain conditions it will actually parse such file, and for other chains it may not.

Example:

Say, you have the following input (shortened for readability, see attachments for full PEM):

-----BEGIN CERTIFICATE-----
MIIL/DCCCuSgAwIBAgIQNC96aabFWNAAAAAAXBAmjjANBgkqhkiG9w0BAQsFADCB
GkO+J90tl9B7UQVtTO1bgGq98HIRdfusGzK5IG2MMT1CtPYR8S01mkf7KMiHK1zb
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
MIIFJzCCBA+gAwIBAgIRALPSJ9szCF4XAAAAAFHTkx8wDQYJKoZIhvcNAQELBQAw
mDaPrsUl15evEah6amsBfpQiWRbKpDLKs1kF
-----END CERTIFICATE-----

example1.txt

Then this will actually pass validation, and X509->loadX509() will actually return the first cert.

But now we have a slightly different certificate chain:

-----BEGIN CERTIFICATE-----
MIILODCCCSCgAwIBAgIQDh0LGipJ++wxFLj8X5MXKDANBgkqhkiG9w0BAQsFADCB
Z2OXFTCG5ORjPUOyMGjxzjEPZWmTOG+gqNOc0HKXbuBAsGZbK4dind+YdZE=
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
MIIF3jCCA8agAwIBAgIQBsFnz+v0jTXWJBAYXhHF6zANBgkqhkiG9w0BAQsFADCB
0wberd/tg3N0dJsSSznZjwYB
-----END CERTIFICATE-----

example2.txt

And now suddenly loadX509() returns false, due to an ASN.1 type 13 (TYPE_RELATIVE_OID), which is not supported and (afaik) deprecated. So how comes that this CA issued certificate is having a type 13 ASN1 structure?

// Note that both example1.pem and example2.pem are completely valid
// certificate chains.

$x509 = new X509();
$pem1 = file_get_contents('example1.pem');
$cert1 = $x509->loadX509($pem1);
// $cert1 contains certificate

$x509 = new X509();
$pem2 = file_get_contents('example2.pem');
$cert2 = $x509->loadX509($pem2);
// $cert2 is false

Root cause:

I traced it down to a subtle issue with X509->_extractBER(). It uses this regex to determine if the input is valid BASE64:

$temp = preg_match('#^[a-zA-Z\d\/+]*={0,2}$#', $temp) ? base64_decode($temp) : false;

Notice the requirement of the = sign only being allowed at the end of the input.

By the time this regex is executed, the -----BEGIN CERTIFICATE----- and -----END CERTIFICATE----- and whitespace is already stripped, so only the BASE64 encoded data remains.

The first example will actually pass the regex, just because the first certificate does not contain base64 padding (=). The second example, where there is padding in the first cert, will fail this regex.

The failure mode here is that phpseclib will not BASE64 decode, and feed the raw ASCII into ASN1._decode_ber(). The first character (-, ASCII value 45) will result (after masking) in opcode 13.

Conclusion

The first example will be parsed as one big ASN.1 structure. Due to the way the ASN.1 parser works, it'll stop at the end of the first certificate and ignore the trailing data. The second example (although being a completely valid PEM chain) will fail to parse.

I understand that multiple ASN.1 objects in a PEM encoded input are not supported. But the behaviour is inconsistent at this moment. I'm not a cryptogrpaher, so I can't oversee the implications here, but maybe this behaviour can be exploited.

@terrafrost
Copy link
Member

I'll try to take a look at this in the next few days - I've been without internet in Phantom Ranch (bottom of the Grand Canyon) and have one more days of hiking to do before I return to my hometown!

@LeonMelis
Copy link
Contributor Author

No worries @terrafrost enjoy your hike!

@terrafrost
Copy link
Member

00c9edc should fix this - thanks!

@LeonMelis
Copy link
Contributor Author

I do not think that the fix is correct. See my comment under commit 00c9edc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants