eclipse-californium / californium

CoAP/DTLS Java Implementation
https://www.eclipse.org/californium/
Other
724 stars 361 forks source link

Minor Release 3.6.0 - Available / Fixes CVE-2022-2576 #2040

Closed boaks closed 1 year ago

boaks commented 2 years ago

For details, please see 3.6.0

boaks commented 2 years ago

The 3.6.0 is released and available on Maven Central and the Eclipse Repository.

The tools and actinium are only available on the Eclipse Repository.

sbernard31 commented 2 years ago

Just to let know that, there is something which looks like a behavior break with this version.

The new verifyKeyPair feature in SingleCertificateProvider is activated by default. This could raise exception at unexpected moment for people which was using previous 3.x version.

Note: this is possible to deactivate this check with code but this will trigger a not so elegant ex.printStackTrace();.

boaks commented 2 years ago

Just to let know that, there is something which looks like a behavior break with this version.

Thanks! FMPOV, it breaks a "misbehavior" ;-).

This could raise exception at unexpected moment for people which was using previous 3.x version

I don't get this. This adds an additional check to DtlsConnectorConfig.builder.build() and works, as all the others there. AFAIK, the only affected use-cases are tests with broken key-pairs, and for them, you may disable it.

boaks commented 2 years ago

@sbernard31

Any news/feedback on the reported API break?

Did you find it on tests with mismatching public/private keys?

Or have I overseen use-cases?

sbernard31 commented 2 years ago

Short answer

I raise this just in case this was not done on purpose. From Leshan v2.0.0-Mx, not a big deal, I just go back to previous behavoir in Leshan Client.

In all case, I strongly advice to remove the ex.printStackTrace();.

Long answer

In a general way : I just said that changing this kind of behavior could create "unexpected problem". If the idea of semantic versioning is to ensure (as far as possible) backward compatibility, my opinion is that this kind of changes should be avoid for minor version :

Anyway, I understand why this could also be considered as a bug/ or miss behavior fix.

More concretely for Leshan integration :

  1. For leshan v2.0.0 (in development), don't care if there is API break in californium before the official v2.0.0 release. I have no problem integrating new major.
  2. mismatching public/private keys tests failed. (Not so easy to adapt the code but this is life)
  3. In Leshan client, credentials could change dynamically (bootstrap). Before bad key make handshake failed, then device try to bootstrap again. With new behavior, exception is raised and client crash/shutdown.

    So for now I just go back to previous behavior in Leshan Client. I guess in Leshan Client this kind of check should be done at bootstrap time in bootstrap config consistency checker. (not done for now)

    About 3., I guess we can consider this is a bad behavior of Leshan because this kind of exception should be handled correctly but this is what I called "unexpected problem".

I don't try to convince you, I just give more details as you asked.

boaks commented 2 years ago

I don't try to convince you, I just give more details as you asked.

Thanks! The ex.printStackTrace() is a overseen left over.

In Leshan client, credentials could change dynamically (bootstrap). Before bad key make handshake failed, then device try to bootstrap again. With new behavior, exception is raised and client crash/shutdown.

Generally, the checks in DtlsConnectorConfig.Builder.build() have been improved over the time. And the last is that detection of private/public key mismatches. I'm not sure, if "miss-configuration" is considered, not dealing with the RuntimeException is really a good solution. Maybe, for midterm, it's easier to "fallback" also on such an configuration exception.

(FMPOV, the API contains an Exception for miss-configuration. The API user should consider that. And if so, the "behavior changes" is just a "bugfix".)

boaks commented 2 years ago

:exclamation::exclamation::exclamation: Important Note: :exclamation::exclamation::exclamation:

This bugfix is required for all users of Californium 3.0.0 - 3.5.0, which are using DTLS resumption and DTLS_VERIFY_PEERS_ON_RESUMPTION_THRESHOLD values larger than 0! It provides the fix for

CVE-2022-2576

jvermillard commented 2 years ago

On my local machine (openjdk 17), leshan tests complains about the key (the exact issue pointed by Simon).

But I struggle to understand why, the key is generated like this:

https://github.com/eclipse/leshan/blob/master/leshan-server-cf/src/test/java/org/eclipse/leshan/server/californium/bootstrap/LeshanBootstrapServerBuilderTest.java#L61-L85

jvermillard commented 2 years ago

I guess the starting point is broken and we should generate one properly

boaks commented 2 years ago

Some weeks/months ago, I recognized, that people not common with x509 mix up the private and public key from the certificates. And then report mystic errors. e.g. server's private key, CA's public key. With that I started to analyze, what could be done best (at least from my view point). I decided to report broken key-pairs just at the builder, as many other misconfiguration is also reported there. That even caused some of my unit tests in Californium to fail, because they use such a invalid pair to verify, that some errors are detected. With that, I added also a function to disable that check. I accidentally left the "System.out", but in the meantime this is replaced by the LOGGER and should be gone with next minor release.

So, from my side the question is: If Californium generally checks for misconfiguration and reports the in the build by exceptions, is it then really a good practice to "do all well and so ignore the exception"? For me it's not. If leshan would catch the exception, it would be already clear at that point, that the bootstrap must be repeated. If it's really not want, then disable the key verification.

About the System.out, I would like to finish a first version of a coap-to-S3-proxy. Since yesterday the required CQs for the aws-sdk has been resolved, so I will prepare for a initial PR as work-in-progress and a feature branch. My forecast will be something short before the EclipseCon (Mid of October).

If you @sbernard31 or @jvermillard see any benefit in a 3.7 without the System.out earlier, let me know, then we can release the 3.7 without the S3-proxy and do that later in a 3.8.

sbernard31 commented 2 years ago

@boaks ,

Maybe @jvermillard was not clear but I guess he wanted to mention that he faces this issue but in a particular case : with openjdk 17 only (not previous one) and for which was considered until now as valid key pair (at least it works with previous version of JDK) See https://github.com/eclipse/leshan/issues/1298.

(this is not a complain about regression or something like this but more asking for help just in case you understand / face same kind of issue)

I feel that a dedicated issue should have been created because this 3.6.0 annonce issue is probably not the best place. If you want to discuss about it, I guess you can use the leshan issue.

If you @sbernard31 or @jvermillard see any benefit in a 3.7 without the System.out earlier, let me know

No urgency for me.

boaks commented 2 years ago

Is it possible for you to share the affected keys (demo-keys?)? Or at least the public-key and certificate?

sbernard31 commented 2 years ago

This is just RPK (no certificate) and it was created like this : https://github.com/eclipse/leshan/blob/master/leshan-server-cf/src/test/java/org/eclipse/leshan/server/californium/bootstrap/LeshanBootstrapServerBuilderTest.java#L61-L85

boaks commented 2 years ago

Just to mention:

CertificateConfigurationHelper

does the check. I signs with the private key and verifies with the public key.

boaks commented 2 years ago

This is just RPK (no certificate) and it was created like this

And running that with java 11 succeeds and fails with 17?

sbernard31 commented 2 years ago

Yep :shrug:

sbernard31 commented 2 years ago

Just to mention:

CertificateConfigurationHelper

does the check. I signs with the private key and verifies with the public key.

Yep I looked at this but don't get why JDK behave differently for this particular key pairs :thinking:

boaks commented 2 years ago

I can reproduce the behavior (I added a test for the leshan keys to CertificateConfigurationHelperTest). Works with java11, fails with java17 ... Yes, Oracle changed a lot in the EC implementation, you may remember ECDSA - CVE-2022-21449. I will check that. But it may be not too easy. And I'm not sure, if the key-pair works, but the verification fails. Or it doesn't work.

sbernard31 commented 2 years ago

I checked the pair with :

And both seems to work with this key pair. :thinking:

boaks commented 2 years ago

I also tried with java 15, which has also the new EC implementation and it fails with that. I added also a dump of the private and public key and I can't see a difference.

boaks commented 2 years ago

OK, my result so far: All your keys have the high-bit set and may be interpreted as negative numbers. But EC is based on positive numbers. If you add a "00" at the head, ... it works ...

(Or use new BigInteger(int signum, byte[] magnitude) with 1).

boaks commented 2 years ago

I hope, it works with the 00 for you as well. Just as cross check: If you now switch off the check and try a handshake with the original key, does that succeed?

sbernard31 commented 2 years ago

I hope, it works with the 00 for you as well.

It seems to work.

If you now switch off the check and try a handshake with the original key, does that succeed?

My first test seems to show it works :grimacing:, I guess it should not, or ? (I mean the handshake succeed)

boaks commented 2 years ago

My first test seems to show it works grimacing, I guess it should not, or ?

I would have assumed not. Anyway, in the real handshake the public key is sent and processed by the other peer. The other peer load that keys from ASN.1, which may fix that on the fly :-):

If I dump the encoded public key, I get the same bytes, signed or unsigned. But only the unsigned is working, if they are used directly.

(And the private key works even without the 00.)

boaks commented 2 years ago

From my side: I don't think, that the "verify" introduces a real issue. At least, if it's assumed, that the public key do not only hold the right bytes, but is also applicable as well.

sbernard31 commented 2 years ago

I also tried with java 15, which has also the new EC implementation and it fails with that.

I guess this confirms the idea : https://bugs.openjdk.org/browse/JDK-8183666

boaks commented 2 years ago

See PR #2058

boaks commented 2 years ago

Yes, once on the right trail, the information starts to give the right picture.

(I still think, that this verification is a good idea, even if I learned now, that there are some pitfalls.)

jvermillard commented 2 years ago

I was planning to dive deeper later, but you two already solved the mystery. Thanks :+1:

boaks commented 2 years ago

You're welcome.