Add support for AWS-LC in the openssl plugin about strongswan HOT 25 CLOSED

So how do you want to proceed? Are you going to prepare a PR?

from strongswan.

Sorry, I missed your last comment here. We're currently working on implementing the missing symbols/ciphers in the AWS-LC library. Once that is done, we'll put together a PR of what needs to be contributed upstream to strongSwan. I'm hopeful that there will be minimal changes to strongSwan.

As of today, we've added compatibility for these missing symbols in AWS-LC:

BN_CTX_secure_new
BN_secure_new
RAND_priv_bytes
EVP_shake128
EVP_shake256

from strongswan.

Hi there, we recently finished adding all of the missing symbols in AWS-LC and are ready to look into making this all build successfully. In addition to the symbols listed above, we've since added support for ChaCha20-Poly1305 and AES-CCM via the EVP_CIPHER API.

The only remaining incompatibility is the difference in how hash data within SHA_CTX is exposed. Our library uses an array, whereas OpenSSL uses fields. BoringSSL is able to support both accessors because of C11's anonymous unions, but our library needs to build against C99 and we can't apply the same workaround. I'm thinking the best way around this is to add a conditional macro around OPENSSL_IS_AWSLC to handle either interface. After that, I am able to get strongSwan to build and run with AWS-LC as the openssl. I will submit a PR for this shortly and you can provide direct feedback there.

In an earlier thread, you suggested defining the analagous OPENSSL_IS_BORINGSSL macro I looked into all places where this macro was applied and concluded that none of those apply to AWS-LC so it makes sense to me to keep these two separate. The two categories of issues this macro handled were: 1) Handling older versions of BoringSSL 2) Avoiding symbols missing from BoringSSL. The first is mitigated because AWS-LC is forked from a much later version of BoringSSL. The second is mitigated because AWS-LC has all of these missing symbols properly stubbed and don't need the conditional handling.

For testing, I'm looking for guidance on what else I can do. I have been able to get this to compile/link successfully. I was also able to setup a VPN with the AWS Site-To-Site VPN service as the other end of the tunnel and manually test this working end to end. The unit tests are largely passing. My next step is to get the integration testing working and see if there are any other defects that need to be addressed.

For the specific unit test failures, I found two issues. test_ed25519_sign fails because AWS-LC's version of i2d_PrivateKey does not support ED25519 keys which breaks the get_encoding implementation (this is also what led to this PR). If you hack around this, the rest of the ed25519 test suite does succeed. I'll look into the difficulty of adding this support into AWS-LC. The other issue is that the entire ed448 test suite fails because AWS-LC has no support for this algorithm. We will not be adding support for this as it is no longer recommended by our cryptographers. Any thoughts on how these should be addressed?

from strongswan.

The only remaining incompatibility is the difference in how hash data within SHA_CTX is exposed.

Well, that whole FIPS-SHA-1 PRF thing is problematic anyway because with OpenSSL 3 direct access to the SHA-1 internals is deprecated. There is an open issue regarding that (openssl/openssl#17688).

The FIPS-SHA-1 PRF is also only required because we currently don't support EAP-AKA' that uses better PRF constructs. Hopefully, at some point we can get rid of it.

I'm thinking the best way around this is to add a conditional macro around OPENSSL_IS_AWSLC to handle either interface.

I'm fine with that change.

The first is mitigated because AWS-LC is forked from a much later version of BoringSSL. The second is mitigated because AWS-LC has all of these missing symbols properly stubbed and don't need the conditional handling.

Makes sense.

test_ed25519_sign fails because AWS-LC's version of i2d_PrivateKey does not support ED25519 keys which breaks the get_encoding implementation (this is also what led to this PR). ... I'll look into the difficulty of adding this support into AWS-LC.

Is there some alternative way to encode the private key besides i2d_PrivateKey? Of course, support via the existing function would be preferable.

The other issue is that the entire ed448 test suite fails because AWS-LC has no support for this algorithm. We will not be adding support for this as it is no longer recommended by our cryptographers. Any thoughts on how these should be addressed?

Is there some conditional that allows us to detect this (some OPENSSL_NO... variable)? If not, we could possible use OPENSSL_IS_AWSLC and then add #ifndefs around the PLUGIN_PROVIDE(..., ..._ED448) statements in openssl_plugin.c.

Also, could we maybe add a test on GitHub Actions that uses AWS-LC?

from strongswan.

I'll look into those ed448 defines and the i2d alternatives and get back to you.

Also, could we maybe add a test on GitHub Actions that uses AWS-LC?

Do you have any pointers on where to start looking into this? Really just need a good example to work off of. At a minimum, I'm planning to add a CI action to AWS-LC to validate against strongSwan.

from strongswan.

I'll look into those ed448 defines and the i2d alternatives and get back to you.

Great, thanks.

Do you have any pointers on where to start looking into this? Really just need a good example to work off of. At a minimum, I'm planning to add a CI action to AWS-LC to validate against strongSwan.

If we can build AWS-LC from sources like we build OpenSSL 3.x, we should be fine. We could even use the same target in test.sh (maybe call the test openssl-aws or something and then add some functions that build the library and configure the flags, then this can be added to linux.yml).

from strongswan.

We picked up these OPENSSL_NO_x defines from BoringSSL. ed448 is not on the list. Our preference is to stick to using the OPENSSL_IS_AWSLC macro for now to avoid adding to our public interface. I'm thinking I can add some ifdefs to openssl_plugin.c like you described to condition out both ed25519 and ed448 and we can scope it down to just ed448 in the future if we can get that i2d functionality implemented. I did a quick look for any alternatives within OpenSSL to do this and came up empty so our only option is to implement it.

If we can build AWS-LC from sources like we build OpenSSL 3.x, we should be fine...

Thanks for the pointers. This looks very straightforward and overlaps a lot with the work we'd need to do to add a CI task to AWS-LC anyway.

In terms of priorities I'm thinking along these lines:

1. Condition out the edwards curves.
2. Add CI tasks to both repositories.
3. Triage any new failures flagged by strongSwan's CI.
4. Implement ed25519 support in i2d_PrivateKey.

from strongswan.

Sounds good. Please let me know if you need any assistance.

from strongswan.

The other issue is that the entire ed448 test suite fails because AWS-LC has no support for this algorithm. We will not be adding support for this as it is no longer recommended by our cryptographers.

By the way, is there any documentation on the reasoning behind this recommendation?

from strongswan.

I posed this question to my colleagues. There's no security concern with these curves and they are considered acceptable for use in applications. The reason they are no longer recommended is that we simply prefer other curves with equivalent/better security, performance, and compliance characteristics. As for why it doesn't exist in AWS-LC, I speculate that it's an artifact of BoringSSL removing it from the library and a lack of impetus to add it back in.

from strongswan.

I've implemented the CI job for AWS-LC and it flagged one new failure which I've traced to an idiosyncrasy of our ASN.1 parser. One of your unit tests passes in an ASN.1 DER encoded X.509 certificate with an expected serial number of [0x00] (it's unclear to me what the ASN.1 DER blob of data intends for it to be) which is illegal according to RFC 5280 but implementers "SHOULD be prepared to gracefully handle such certificates". It turns out, AWS-LC interprets this as parsing the certificate as having [] for the serial number rather than what OpenSSL does which is parsing it with [0x00]. One approach is to update the unit test asserts to accept either convention, but I thought I'd ask if there are other considerations. I'm still trying to figure out if this is a bug in AWS-LC or if it's just an implementation detail which we ended up not aligning on.

from strongswan.

(it's unclear to me what the ASN.1 DER blob of data intends for it to be

What do you mean?

which is illegal according to RFC 5280 but implementers "SHOULD be prepared to gracefully handle such certificates"

The RFC says "MUST be a positive integer" and also "CAs MUST force the serialNumber to be a non-negative integer". So what's zero? It's clearly non-negative, but is it positive? There is actually an errata about this, which changes the text explicitly to "MUST be a positive non-zero integer" because of the note that points out zero. So yes, I guess zero is technically invalid.

The serial tests were added after 18082ce. Because that change removes leading zeroes in the serials, I guess, this particular test was to ensure that a zero serial is not actually shortened. So this...

It turns out, AWS-LC interprets this as parsing the certificate as having [] for the serial number rather than what OpenSSL does which is parsing it with [0x00].

...sounds incorrect.

from strongswan.

What do you mean?

I'm trying to distinguish between whether this is an OpenSSL specific decision to handle non-conforming certificates in this way or if we were to decode these X.509/ASN.1/DER bytes according to the standards documents would we get the same result? Sorry if I wasn't being clear there.

Thanks for the context and good find on that errata document. I've seen a few other things as well that suggest that there's something wrong in our ASN.1 parser with this case, but I still have to keep looking into it to really understand what's going on here.

from strongswan.

BoringSSL settled on the convention (ref) of representing the value of an ASN1_INTEGER as an empty array. This is (roughly) because they wanted a newly allocated ASN1_INTEGER to represent 0 and this change in convention was preferable to changing the default construction. As of today, this is still the case for BoringSSL and building against that library should surface the same test failures in strongSwan. Internally, AWS-LC is consistent with this convention (e.g. compare, parse, encode... implementations) thanks to BoringSSL. Changing this behavior means that we'd have to maintain that consistency without reintroducing the CVEs that BoringSSL wanted to avoid. That said, I don't think AWS-LC should be changed to support a 0-valued ASN1_INTEGER as [0x00] since the risk of it going wrong seems high.

strongSwan has a unit test which parses an ASN.1 DER encoded certificate (and CRL with the CRL number extension) of 0. The original intent of this test seems to be to assert that leading 0's are trimmed from the serial numbers to achieve a canonical form (code ref). It's unclear if there was any intent to explicitly test the 0-valued certificate serial number which is illegal according to RFC 5280 (However, a 0-valued CRL number is legal). A simple workaround we can add is to update the unit test to expect either representation of 0. Properly handling a 0-valued serial number within strongSwan (if we believe there is even an issue there) should probably be tracked separately from the AWS-LC work in the openssl plugin. If there were an issue there, I'd expect it would surface around checking equality of 0-value serials which should probably just be rejected outright.

I also noticed that get_serial returns different interpretations of 0 for x509_t and crl_t. This is due to x509_t returning a chunk backed by an ASN1_INTEGER parsed natively by AWS-LC which will create the [] version of 0 (ref). In crl_t, the CRL number is parsed from the ASN1_OCTET by strongSwan and results in the [0x00] encoded version of 0. If there's a desire for crl_t to be consistent with the underlying libcrypto's parsing, the d2i_ASN1_INTEGER function can be used instead of the one coded into openssl_crl.c.

We are unable to change the representation of 0 within the library. I can make the openssl_crl code's ASN.1 parsing more consistent, but that could break other 0-related assertions within strongSwan and doesn't address the unit test failures. I can see some potential issues around handling 0's in the OpenSSL convention within strongSwan, but I'm probably not the right person to investigate/address them. Any thoughts?

from strongswan.

of representing the value of an ASN1_INTEGER as an empty array.

Is there maybe an "invalid" missing here?

That said, I don't think AWS-LC should be changed to support a 0-valued ASN1_INTEGER as [0x00]

Hm, zero is a valid value for ASN.1 integers, though. Are you just referring to serials in X.509 certificates? Or what do you mean?

A simple workaround we can add is to update the unit test to expect either representation of 0.

What does that mean? What's the other representation? The implementations of get_serial() are expected to return the serials in canonical form (that was the whole point of the commit you also pointed out), so there should be only one form at that point. But also see below.

I also noticed that get_serial returns different interpretations of 0 for x509_t and crl_t. This is due to x509_t returning a chunk backed by an ASN1_INTEGER parsed natively by AWS-LC which will create the [] version of 0 (ref). In crl_t, the CRL number is parsed from the ASN1_OCTET by strongSwan and results in the [0x00] encoded version of 0. If there's a desire for crl_t to be consistent with the underlying libcrypto's parsing, the d2i_ASN1_INTEGER function can be used instead of the one coded into openssl_crl.c.

The implementation of these interfaces (i.e. the openssl plugin in this case) has to provide the serial in the way strongSwan expects it. So this has to be handled inside the plugin, for OpenSSL it seems to work correctly (just going by the test results), so if there is something we have to do differently for AWS-LC, we should do that inside the plugin and hide it from the rest of strongSwan.

from strongswan.

Is there maybe an "invalid" missing here?

No, see the following...

Hm, zero is a valid value for ASN.1 integers, though. Are you just referring to serials in X.509 certificates? Or what do you mean?

To clarify, yes, zero is a valid value for ASN.1 integers. It's just that Google chose to represent it as an empty array within the ASN1_INTEGER C type. Maybe it'll be clearer with some source code to look at.

// Definition of an ASN1_INTEGER in the include files looks like this
typedef struct asn1_string_st ASN1_INTEGER;
struct asn1_string_st {
  int length;
  int type;
  unsigned char *data;
  long flags;
};

// Here's what a 0 looks like within AWS-LC/BoringSSL
(gdb) p *this->x509->cert_info->serialNumber
$12 = {
  length = 0,
  type = 2,
  data = 0x602000012958 "",
  flags = 0
}

// If you do the same thing in OpenSSL, it'll probably look like this (n.b. I didn't actually run this one)
(gdb) p *this->x509->cert_info->serialNumber
$12 = {
  length = 1,
  type = 2,
  data = 0x602000012958 "\0",
  flags = 0
}

// Semantically, both refer to an ASN.1 Integer with value of 0 within the library. They just look different in memory.

The implementations of get_serial() are expected to return the serials in canonical form (that was the whole point of the commit you also pointed out), so there should be only one form at that point. But also see below.

The implementation of these interfaces (i.e. the openssl plugin in this case) has to provide the serial in the way strongSwan expects it. So this has to be handled inside the plugin, for OpenSSL it seems to work correctly (just going by the test results), so if there is something we have to do differently for AWS-LC, we should do that inside the plugin and hide it from the rest of strongSwan.

Thanks for these insights around the expectations of what should come out of the openssl plugin and what strongSwan expects for the serial's canonical form. I agree that that should be handled within the openssl plugin and I'll take a pass at it to get these tests passing again.

from strongswan.

To clarify, yes, zero is a valid value for ASN.1 integers. It's just that Google chose to represent it as an empty array within the ASN1_INTEGER C type.

Might be for different reasons, but a long time ago our code also assumed zero was represented by a zero length encoding in ASN.1. That's not the case (82e526c, see 8.3.1 in X.690).

Also, does ASN1_INTEGER [0x00] exist in BoringSSL/AWS-LC?

I agree that that should be handled within the openssl plugin and I'll take a pass at it to get these tests passing again.

OK, great, looking forward to what you come up with.

from strongswan.

Might be for different reasons, but a long time ago our code also assumed zero was represented by a zero length encoding in ASN.1. That's not the case (82e526c, see 8.3.1 in X.690).

Ahh interesting, that's pretty much the logic I've got in my workspace right now. I think the disconnect is that ASN.1, in the abstract sense, doesn't have any encoding and we're mixing the implementation of the C-type ASN1_INTEGER with the DER encoding. It's only when you get to BER/DER in X.690 where they start defining things like [0x02, 0x01, 0x00] is an Integer of value 0.

Also, does ASN1_INTEGER [0x00] exist in BoringSSL/AWS-LC?

No, they make sure that all ASN1_INTEGERs produced by the library are "minimal" down to an empty array. I checked the ASN.1 parsing code, it very explicitly define minimal encoding as being either empty or the MSB is not 0. The only exception to this (documented in the comments) is if you forcefully constructed an ASN1_INTEGER from an ASN1_STRING.

from strongswan.

With that serial parsing fix, here's the last CI failure that needs to be addressed. When run with --enable-leak-detector I've got these two memory leaks from internal RAND_bytes related state. This suggests that the destructors (which frees the related memory) for the threads are not being called by the test runner for some reason. I'll post a CR shortly that appears to fix the memory leak, but would appreciate some insight on why this might be happening. I'm unsure how all the threading works in the test runner and the plugin.

     Failure in 'test_vectors': Leak detected: 1 allocations using 40 bytes (i = 1)
 dumping 19 stack frame addresses:
  /home/gcr/git-workplace/aws-lc/build/install/lib/libcrypto.so @ 0x7fe857a00000 (CRYPTO_set_thread_local+0x73) [0x7fe857ad0ecb]
    -> /home/gcr/git-workplace/aws-lc/crypto/thread_pthread.c:157
  /home/gcr/git-workplace/aws-lc/build/install/lib/libcrypto.so @ 0x7fe857a00000 [0x7fe857b95b19]
    -> /home/gcr/git-workplace/aws-lc/crypto/fipsmodule/rand/rand.c:423 (discriminator 1)
  /home/gcr/git-workplace/aws-lc/build/install/lib/libcrypto.so @ 0x7fe857a00000 (RAND_bytes+0x2e) [0x7fe857b95e3a]
    -> /home/gcr/git-workplace/aws-lc/crypto/fipsmodule/rand/rand.c:549
  /home/gcr/git-workplace/aws-lc/build/install/lib/libcrypto.so @ 0x7fe857a00000 (RAND_priv_bytes+0x27) [0x7fe857b95e68]
    -> /home/gcr/git-workplace/aws-lc/crypto/fipsmodule/rand/rand.c:554
  /home/gcr/git-workplace/strongswan/src/libstrongswan/plugins/openssl/.libs/libstrongswan-openssl.so @ 0x7fe858133000 [0x7fe85814ab03]
    -> /home/gcr/git-workplace/strongswan/src/libstrongswan/plugins/openssl/openssl_rng.c:56
    ...
      Failure in 'test_vectors': Leak detected: 1 allocations using 304 bytes (i = 1)
 dumping 18 stack frame addresses:
  /home/gcr/git-workplace/aws-lc/build/install/lib/libcrypto.so @ 0x7fe857a00000 [0x7fe857b95aca]
    -> /home/gcr/git-workplace/aws-lc/crypto/fipsmodule/rand/rand.c:419
  /home/gcr/git-workplace/aws-lc/build/install/lib/libcrypto.so @ 0x7fe857a00000 (RAND_bytes+0x2e) [0x7fe857b95e3a]
    -> /home/gcr/git-workplace/aws-lc/crypto/fipsmodule/rand/rand.c:549
  /home/gcr/git-workplace/aws-lc/build/install/lib/libcrypto.so @ 0x7fe857a00000 (RAND_priv_bytes+0x27) [0x7fe857b95e68]
    -> /home/gcr/git-workplace/aws-lc/crypto/fipsmodule/rand/rand.c:554
  /home/gcr/git-workplace/strongswan/src/libstrongswan/plugins/openssl/.libs/libstrongswan-openssl.so @ 0x7fe858133000 [0x7fe85814ab03]
    -> /home/gcr/git-workplace/strongswan/src/libstrongswan/plugins/openssl/openssl_rng.c:56
    ...

from strongswan.

This suggests that the destructors (which frees the related memory) for the threads are not being called by the test runner for some reason. I'll post a CR shortly that appears to fix the memory leak, but would appreciate some insight on why this might be happening. I'm unsure how all the threading works in the test runner and the plugin.

Only worker threads are terminated during test runs. The main thread is not, because all the tests are run in the same process. Whereby each test case fully initializes/deinitializes the strongSwan libraries and loads/unloads the plugins. The leak detective evaluation then also happens in the main thread (when libstrongswan is deinitialized), so if something is freed only when that thread is terminated, leak detective will flag it.

The above has some implications on third-party library cleanup. For instance, with OpenSSL 3 we don't explicitly cleanup at all anymore when the openssl plugin is unloaded/destroyed because the library can't be initialized again within the same process. That's why there are quite a lot of whitelisted OpenSSL functions in

Even some RAND_* functions are already there. So we could add RAND_bytes() to the list (since it's not meant to allocate anything that would be fine because if it "leaks" it's not because we've forgotten to free something), or we could explicitly call AWSLC_thread_local_clear() from the main thread (I guess calling it twice is not a problem?). I think I'd prefer the whitelist approach in this case.

from strongswan.

so if something is freed only when that thread is terminated, leak detective will flag it.

Yes, that's exactly what's happening here. RAND_bytes allocates some thread specific data and registers a destructor to free it when the thread terminates. No explicit cleanup is normally necessary so I agree with the whitelist approach you suggested. Expect another PR for this and also the CI check that brings it all together.

from strongswan.

In PR #2151 you asked about adding a function to ID EC_KEY's from explicit curves. It turns out BoringSSL and AWS-LC don't support explicit curves at all. Here's a comment from davidben (BoringSSL maintainer) which describes the behavior succinctly. Looking through the strongSwan code, it looks like you always go through the ASN.1 parser and explicit EC curve parameters will either return a NULL EC_KEY or (in the case of this test) checked that it matches a known curve. We provide this function, but it simply just returns OPENSSL_EC_NAMED_CURVE so it may not be worthwhile to code that up.

from strongswan.

Ah, I see. So public keys with explicit parameters are always rejected, but private keys not if they use known curves (which the test case actually shows, it only warns about the private key). I guess that's OK.

from strongswan.

If there's anything you'd like to change in the unit tests or plugin regarding this explicit EC parameter behavior let me know and I can put that together.

Otherwise, I just wanted to thank you for your time working with me on this. I'm extremely grateful for all the prompt and helpful feedback you've provided as I merged these changes in. I've added CI checks for strongSwan to AWS-LC, but if any issues come up that we don't notice definitely open an issue against the AWS-LC repository so we can get it sorted out. With that said, I think we can close out this issue if you agree.

from strongswan.

If there's anything you'd like to change in the unit tests or plugin regarding this explicit EC parameter behavior let me know and I can put that together.

I don't think we have to change anything in the plugin. Rejecting the public key (e.g. in a received certificate) is what's important. So I've changed the warning message in the test case a bit if the private key was parsed after the public key was rejected. I guess we could theoretically extend the test with a non-NIST curve and distinguish that case as well, but I don't think that's necessary at the moment.

With that said, I think we can close out this issue if you agree.

Agreed 😄 🎉 Thank you for your efforts on this (also internally in extending AWS-LC to avoid #ifdefs). It was a really enjoyable experience working with you.

from strongswan.