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

SSLError(58, '[ASN1] nested asn1 error (_ssl.c:4174)') and SSLError(0, 'not enough data: cadata does not contain a certificate (_ssl.c:4159)') #25023

Open
levicki opened this issue Jul 29, 2024 · 22 comments
Labels
branch: master Merge to master branch help wanted triaged: feature The issue/pr requests/adds a feature

Comments

@levicki
Copy link

levicki commented Jul 29, 2024

If a CA encodes certificate serial number integer value 102 (0x66) as 02 04 00 00 00 66 instead of 02 01 66, which violates the DER standard the following error shows up when trying to load such a certificate:

SSLError(58, '[ASN1] nested asn1 error (_ssl.c:4174)') and SSLError(0, 'not enough data: cadata does not contain a certificate (_ssl.c:4159)')

There are some root certificates in circulation with such non-compliant DER encoding which apparently can't be replaced, and they are causing a host of issues in various Python based applications since Python uses OpenSSL.

I am reporting this as a bug even though I don't consider strict adherence to DER standard to be a bug, but something should be done to allow use of such certificates or there should be an option to relax validation so that this results in a warning instead of error.

Other implementations (Firefox, Chromium) accept those certificates.

Relevant Python issue can be found here.

Current workaround is to:

  • Process certificates from Windows certificate store one by one and
  • Skip certificates which raise an exception related to this error

@mattcaswell @t8m Please advise.

@levicki levicki added the issue: bug report The issue was opened to report a bug label Jul 29, 2024
hubot pushed a commit to blender/blender that referenced this issue Jul 30, 2024
Workaround: `[ASN1] nested asn1 error` error when making HTTPS
connections on systems with certificates that OpenSSL cannot parse
are installed.

This is a general issue with Python, resolve by applying a proposed
fix [0] to the extensions Python process at run-time.
(this doesn't impact Blender's Python run-time).

The down side is HTTPS connections will only work for extensions
on systems with this problem so this needs to be resolved by Python
long term.

While any changes to Python's SSL checks is worth avoiding,
this simply skips SSL certificates in the windows store that OpenSSL
can't parse instead of failing all SSL connections.

See related issues:

- python/cpython#79846
- openssl/openssl#25023

[0]: python/cpython#91740

Ref !124943.
@mattcaswell
Copy link
Member

Do we have some examples of such non-compliant CA certs?

@mattcaswell mattcaswell added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug and removed issue: bug report The issue was opened to report a bug labels Jul 31, 2024
@levicki
Copy link
Author

levicki commented Jul 31, 2024

@mattcaswell Original DER file can be found here:

http://crl.mup.gov.rs/MUPCARoot.crt

I converted it to .pem and attached as .txt here because OpenSSL refuses to convert it with the following error:

openssl x509 -inform DER -in MUPCARoot.cer -out MUPCARoot.pem
Could not find certificate from MUPCARoot.cer
10050000:error:1608010C:STORE routines:ossl_store_handle_load_result:unsupported:..\OpenSSL-source\crypto\store\store_result.c:151:

When we are at it, that's a rather cryptic and unhelpful error message.

Here's the problematic part of the DER encoding:

image

So apparently this issue affects command line tool as well.

Version used:

OpenSSL 3.3.0 9 Apr 2024 (Library: OpenSSL 3.3.0 9 Apr 2024)

Let me know if there's anything more I can help with.

mupcaroot.txt

@mattcaswell mattcaswell added help wanted triaged: feature The issue/pr requests/adds a feature and removed triaged: bug The issue/pr is/fixes a bug labels Aug 2, 2024
@mattcaswell
Copy link
Member

I converted it to .pem and attached as .txt here

Thanks. That was helpful.

I took a quick look to understand what would need to change in order to be able to parse these certificates. Here's a quick-and-dirty hack I did to make the certificate readable:

diff --git a/crypto/asn1/a_int.c b/crypto/asn1/a_int.c
index dc962290dd..0779fe8d9f 100644
--- a/crypto/asn1/a_int.c
+++ b/crypto/asn1/a_int.c
@@ -150,7 +150,7 @@ static size_t i2c_ibuf(const unsigned char *b, size_t blen, int neg,
  * of buffer or 0 on error: for malformed INTEGER. If output buffer is
  * NULL just return length.
  */
-
+#define HACK_ACCEPT_ILLEGAL_PADDING
 static size_t c2i_ibuf(unsigned char *b, int *pneg,
                        const unsigned char *p, size_t plen)
 {
@@ -176,7 +176,13 @@ static size_t c2i_ibuf(unsigned char *b, int *pneg,
 
     pad = 0;
     if (p[0] == 0) {
+#ifdef HACK_ACCEPT_ILLEGAL_PADDING
+        for (pad = 1; (size_t)pad < plen && p[pad] == 0; pad++);
+        if ((size_t)pad == plen)
+            pad--;
+#else
         pad = 1;
+#endif
     } else if (p[0] == 0xFF) {
         size_t i;
 
@@ -188,11 +194,13 @@ static size_t c2i_ibuf(unsigned char *b, int *pneg,
             pad |= p[i];
         pad = pad != 0 ? 1 : 0;
     }
+#ifndef HACK_ACCEPT_ILLEGAL_PADDING
     /* reject illegal padding: first two octets MSB can't match */
     if (pad && (neg == (p[1] & 0x80))) {
         ERR_raise(ERR_LIB_ASN1, ASN1_R_ILLEGAL_PADDING);
         return 0;
     }
+#endif
 
     /* skip over pad */
     p += pad;

Note that adding this hack causes various tests related to asn1 integer encoding/decoding to start failing (which is perhaps not surprising).

I agree, that I don't think we can characterise this as a bug. The code is operating as designed and in accordance with the standards. It might be reasonable to have some "relaxed" mode for DER parsing. OTOH this isn't without security consequences. Part of the point of DER is that there is only one possible encoding of the same thing. By being relaxed about such things we are allowing multiple encodings of the same certificate - although its difficult to see how this could be exploited in any meaningful way in this case.

This change is deep in the ASN1 parsing code. To make this kind of change acceptable I think we would need some mechanism for turning this on or off (default should be off, as now). That then throws up various questions as to how that would be achieved. It could be a compile time option - but then that means it would always be on if compiled that way. Which seems unwise. If we had some runtime option how would that work? Would it be global (probably the easiest to do - but again is it wise), or somehow local to a particular parsing attempt. If local how would you pass such an option down through the call stack without significant impact on the API?

This seems a somewhat niche feature so I think its unlikely that the dev team will prioritise this anytime soon. If someone from the community wanted to take this on then I think we would consider it - but I think getting the design right would be critical. Probably some outline design of how to turn it on/off would be required first.

Adding the "feature" and "help wanted" labels.

@levicki
Copy link
Author

levicki commented Aug 2, 2024

@mattcaswell I agree with the most of your assessment of the issue. Detailed reply below:

OTOH this isn't without security consequences.

If the user already has such a certificate in their trust store (which implies trust), then allowing them to override parsing strictness is a reasonable request even if it might not be wise thing to do.

I think we would need some mechanism for turning this on or off (default should be off, as now).

I agree it should be off by default, but I also think it should stay off by default forever.

My thoughts on the available options:

On one hand, this is something that a user having such a problem with their certificates will want to turn on for all instances of OpenSSL on their system, and on the other hand developers writing code in Python who are facing this issue could as well bundle custom OpenSSL build with compile-time option turned on. Finally, there's also an option of OpenSSL considering whether this DER enforcement in this particular case (serial number) makes sense, and either changing the default code to ignore the error like browsers and Microsoft certificate tools do, or adding a new API to load such certs with relaxed checking.

I am far from the security expert to tell which one of these 3 options is lower risk.

I'd prefer if @t8m also chimed in with his thoughts on the matter.

@mattcaswell
Copy link
Member

@vdukhovni may also have a view.

@mattcaswell
Copy link
Member

Another point to note about my quick-and-dirty hack, is that it modifies the way DER integer parsing is processed for all DER integers regardless of the context. To fix this problem we only really need to be relaxed for this one serial number integer. Perhaps we could declare that one integer as some special type of "relaxed" integer instead of a normal one which would reduce the scope of any change we are making here and perhaps make it much easier to justify having it on.

@levicki
Copy link
Author

levicki commented Aug 3, 2024

@mattcaswell I was always wondering why DER standard insists on tight packing of integers. What's the big deal except saving some zero bytes? Does it actually prevent some exploits?

Unless there is some endinanness ambiguity is there any actual difference between representing a value of 0x55 as 0x02 0x01 0x55, 0x02 0x02 0x00 0x55, and 0x02 0x04 0x00 0x00 0x00 0x55?

I mean, in order to compare 0x55 with some other integer value you will most likely end up zero-extending both values to the platform integer size under the hood. That means said 0x55 will on x86 get loaded with movzx eax, byte ptr [mem] and then compared using some form of cmp eax, [reg/mem].

Now if the DER standard says that 0x02 0x01 0x55 and 0x02 0x02 0x00 0x55 are not identical because of length field difference even though both would get zero-extended to 4 bytes before comparison and compare as equal, then that's another case of UTF8STRING "ABC" isn't the same as PRINTABLESTRING "ABC" nonsense in the standard (which by the way was an issue I reported here which got converted into discussion and nothing was ever done about it despite conclusion that PRINTABLESTRING can be converted to Unicode for comparison with UTF8STRING).

Perhaps we could declare that one integer as some special type of "relaxed" integer instead of a normal one which would reduce the scope of any change we are making here and perhaps make it much easier to justify having it on.

From RFC 5280 Section 4.1.2.2

4.1.2.2.  Serial Number

   The serial number MUST be a positive integer assigned by the CA to
   each certificate.  It MUST be unique for each certificate issued by a
   given CA (i.e., the issuer name and serial number identify a unique
   certificate).  CAs MUST force the serialNumber to be a non-negative
   integer.

   Given the uniqueness requirements above, serial numbers can be
   expected to contain long integers.  Certificate users MUST be able to
   handle serialNumber values up to 20 octets.  Conforming CAs MUST NOT
   use serialNumber values longer than 20 octets.

   Note: Non-conforming CAs may issue certificates with serial numbers
   that are negative or zero.  Certificate users SHOULD be prepared to
   gracefully handle such certificates.

My reading of this is that the handling of X.509 serial number field on the client side (so not certificate creation, but opening of an existing certificate) should definitely be relaxed as long as the DER parsing can be continued correctly after whatever integer length up to 20 octets provided in the integer field is consumed (that is, the field length fits into total length of the parent ASN.1 object) unless of course there's some chance for that to affect security.

I'd like to hear your thoughts on it.

hubot pushed a commit to blender/blender that referenced this issue Aug 6, 2024
Workaround: `[ASN1] nested asn1 error` error when making HTTPS
connections on systems with certificates that OpenSSL cannot parse
are installed.

This is a general issue with Python, resolve by applying a proposed
fix [0] to the extensions Python process at run-time.
(this doesn't impact Blender's Python run-time).

The down side is HTTPS connections will only work for extensions
on systems with this problem so this needs to be resolved by Python
long term.

While any changes to Python's SSL checks is worth avoiding,
this simply skips SSL certificates in the windows store that OpenSSL
can't parse instead of failing all SSL connections.

See related issues:

- python/cpython#79846
- openssl/openssl#25023

[0]: python/cpython#91740

Ref !124943.
@t8m
Copy link
Member

t8m commented Sep 6, 2024

@mattcaswell I was always wondering why DER standard insists on tight packing of integers. What's the big deal except saving some zero bytes? Does it actually prevent some exploits?

The primary (only?) difference of DER from BER is that there is only one possible encoding of the ASN.1 structure under the DER format. I.e. the same structure always encodes into the same binary bytes. That property is very much useful for various security and non-security reasons.

What you want is a BER encoding but that is not a conforming encoding for X.509.

@vdukhovni
Copy link

I don't think OpenSSL should change to accomodate BER (non-DER) encoded X.509 certificates. RFC5280 is quite clear on the DER requirement:

The signatureValue field contains a digital signature computed upon
the ASN.1 DER encoded tbsCertificate.

Does this particular trust anchor (self-signed root CA) have a signature computed over the DER or BER form of the "tbs" certificate?
If the signature is actually over the DER encoding as required, just reëncode the tbsCertificate in DER form, and add that to the trust store.

Note that (unless the application specifies otherwise), OpenSSL does not check the self-signatures self-signed certificates, so you can reëncode it even if that in principle "breaks" the signature.

OpenSSL 1.1.1 also refused to parse the example TA, so refusing to parse it is by no means a recent regression. The CA in question can/shoudl just reissue the certificate in question with the same key, but correctly encoded.

@levicki
Copy link
Author

levicki commented Sep 6, 2024

@vdukhovni

Note that (unless the application specifies otherwise), OpenSSL does not check the self-signatures self-signed certificates, so you can reëncode it even if that in principle "breaks" the signature.

Re-encoding it and breaking the signature is not an option because then other software (mail clients, browsers, Office suite, PDF readers, etc which aren't using OpenSSL) won't be able to use the chain to validate the signature — under that root CA there is an intermediate CA and quite a lot of individual document signing certificates under it.

Furthermore, OpenSSL itself fails to find and re-encode the certificate (unless there's some way of doing it using openssl.exe that I am not aware of).

OpenSSL 1.1.1 also refused to parse the example TA, so refusing to parse it is by no means a recent regression.

Nobody claimed it was a recent regression, certificate in question is quite dated but sadly still in use.

The CA in question can/should just reissue the certificate in question with the same key, but correctly encoded.

I agree they could/should, but I know they won't hence the request for OpenSSL to provide some off-by-default option to ignore this like others do because at the end of the day the user has chosen to trust that CA despite a slight encoding issue.

@vdukhovni
Copy link

Re-encoding it and breaking the signature is not an option because then other software (mail clients, browsers, Office suite, PDF readers, etc which aren't using OpenSSL) won't be able to use the chain to validate the signature — under that root CA there is an intermediate CA and quite a lot of individual document signing certificates under it.

Which applications check the self-signature of root CAs? This is not typically productive, so reëncoding is quite likely to be safe. Yes, you'd need some other tool to produce the DER form of the tbsCertificate.

If users have trouble with that root CA, perhaps that's the right signal to the relevant operator and any subsidiary (intermediate) CAs.

@vdukhovni
Copy link

At this point it would be useful to see a complete chain with the problem root CA as a trust anchor. Is this something that can be arranged?

@vdukhovni
Copy link

By the way, here's the reëncoded root CA cert. It may be sufficient to address the issue.

mup-root.pem.txt

@levicki
Copy link
Author

levicki commented Sep 7, 2024

Which applications check the self-signature of root CAs?

Windows crypto stack and tools? Here's how it shows modified root CA which you have provided:

image

Even after you import it into the Windows root store it shows as invalid:

image

If users have trouble with that root CA, perhaps that's the right signal to the relevant operator and any subsidiary (intermediate) CAs.

Users do not have trouble with that root CA — users have trouble with Python standard library importing that root CA from Windows root store into its own store using OpenSSL which chokes on it, thus crashing many Python based applications in the process. They are forced to choose between not using Python apps or not using their document signing certificates and e-Government portal.

As I pointed out, this is government (interior ministry) issued root CA, their intermediate CA below, and then quite a lot of citizens' personal signing certificates stored in their national ID cards. Said interior ministry doesn't even have a contact listed for CA which you could ask to re-issue the problematic root and even if you somehow got them to reissue it, most users wouldn't know how to replace it (as a matter of fact most of them didn't even know it's the cause of the python apps' issues).

I know it all sounds ass-backwards but such is real life sometimes.

Moreover, even if you imported this modified root CA and it:

  • didn't fail the import in Python apps causing crash in the same manner DER encoding does now
  • worked for intended purposes of verifying the chain for document signing

The interior ministry middleware would likely restore the unmodified chain upon reboot or next time you insert your national ID card.

Finally, I don't see how your suggested tampering with CA root signature is better from a security perspective than having an option to relax the DER check for certificate serial number like Chromium and Mozilla already do.

@levicki
Copy link
Author

levicki commented Sep 7, 2024

At this point it would be useful to see a complete chain with the problem root CA as a trust anchor. Is this something that can be arranged?

I have a newer national ID with personal certificate issued from a different root / intermediary so sadly I can't show you the chain for that one. I also fail to see how that's relevant? That part is working as intended, it's only OpenSSL refusing to load / parse such certificate that's the issue.

@vdukhovni
Copy link

When I look at the Serbian government CA website, the root CA in question is not listed. It appears to have been long retired. So I am looking for a chain of certificates (at least one unexpired actively used intermediate or unexpired EE certificate) that that ultimately terminates at this particular root.

Is it actually needed, or is it just vestigial. If the Python application "crashes", that's a bug in the Python application, it should simply skip CAs that fail to decode, and use the rest.

@vdukhovni
Copy link

Bottom line, if the CA in question is just hanging around various root CA stores, but no longer needed to verify any unexpired certificates, then the solution is to update the Python code in question to skip errors when decoding root CAs, and use what it can decode.

If, on the other hand, there are (despite no mention of this CA on the Serbian national CA website) still live certificates chaining to this and no other root CA, then we plausibly have a reason to support this (though not necessarily compelling if this is the only use-case).

This is why I am asking for evidence of current relevance (unexpired chains leading to EE certs) of this CA.

@levicki
Copy link
Author

levicki commented Sep 7, 2024

When I look at the Serbian government CA website, the root CA in question is not listed. It appears to have been long retired. So I am looking for a chain of certificates (at least one unexpired actively used intermediate or unexpired EE certificate) that that ultimately terminates at this particular root.

They have been removed from public facing listing. They can still be downloaded though:

1. Root CA in question: http://crl.mup.gov.rs/MUPCARoot.crt
2. Intermediate CA issued under it: http://crl.mup.gov.rs/MUPCAGradjani.crt

Both have validity until February 2030.

Is it actually needed, or is it just vestigial.

I can't tell for sure — it would depend on when they stopped issuing personal certificates under MUPCA Gradjani intermediate together with national IDs. Those personal certificates are valid for 5 years. From what I see they haven't really been following the best practices in the beginning of their operation. I do know that all newly issued cards and certificates use new root and intermediary.

If the Python application "crashes", that's a bug in the Python application, it should simply skip CAs that fail to decode, and use the rest.

No, that's the "bug" in the python standard library. Application just requests to import certs from Windows store so it can communicate using HTTPS, code for importing is in the library and that's what crashes. That has been reported by me and it has been fixed but only in Python 3.12 onwards. There are a lot of applications which can't update from Python 3.10 and 3.11 because of dependency chains so unfortunately that doesn't fully address the issue.

To be honest, I reported this here with pretty tame expectations because Pyhton team requested that someone does so.

I did say upfront that I don't consider this a bug as such, but I really think OpenSSL shouldn't be this nitpicky on things which are safe to ignore like this serial number encoding or comparisons between ANSI and UTF8 strings which fail even when strings are identical just because their DER labels aren't.

@vdukhovni
Copy link

No, that's the "bug" in the python standard library. Application just requests to import certs from Windows store so it can communicate using HTTPS, code for importing is in the library and that's what crashes. That has been reported by me and it has been fixed but only in Python 3.12 onwards. There are a lot of applications which can't update from Python 3.10 and 3.11 because of dependency chains so unfortunately that doesn't fully address the issue.

Good to know that Python (library, rather applications, ...) will be updated. Note that even if some OpenSSL releases are updated, it will be some time before users see any updates.

If there isn't clear evidence that unexpired chains leading to that root will still be around long enough by the time the fix reaches users, I don't see much value in implementing a workaround.

To be honest, I reported this here with pretty tame expectations because Pyhton team requested that someone does so.

Sure, you did the right thing, what remains to be determined is the right response...

I did say upfront that I don't consider this a bug as such, but I really think OpenSSL shouldn't be this nitpicky on things which are safe to ignore like this serial number encoding or comparisons between ANSI and UTF8 strings which fail even when strings are identical just because their DER labels aren't.

Well, there isn't any dedicated code for reading serial numbers, they're just ASN.1 integers, and the code for that is shared across all the structures that hold such values, so a workaround would have collateral side-effects.

@vdukhovni
Copy link

I should also mention that the intermediate CA in question also has a non-DER serial number encoding 00000070, but is signed with SHA1, so e.g. its signature from the root would fail to validate under the default RHEL/Fedora crypto policies...

These are looking more and more like vestigial artefacts we can ask libraries, applications and users to gracefully ignore.

@levicki
Copy link
Author

levicki commented Sep 8, 2024

These are looking more and more like vestigial artefacts we can ask libraries, applications and users to gracefully ignore.

On that we fully agree and if OpenSSL can also add an option to ignore such errors in old certificates it would be nice.

Please, if you have time also take a look at this issue which got converted into discussion and closed for some reason:
#18339

It's another situation where the library could in my humble opinion do better — no idea why it got moved and closed without any concrete decision being made. The reason I posed it as a question was to check if the behavior is expected or not and if not to see if it can be improved.

@vdukhovni
Copy link

Note, when I say ignore, I don't mean support. Refusing to parse it leaves ignoring the certificate (not the non-standard encoding) up to the caller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch help wanted triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

No branches or pull requests

4 participants