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

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

Closed
boaks opened this issue Jul 7, 2022 · 30 comments
Closed

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

boaks opened this issue Jul 7, 2022 · 30 comments

Comments

@boaks
Copy link
Contributor

boaks commented Jul 7, 2022

For details, please see 3.6.0

@boaks boaks pinned this issue Jul 7, 2022
@boaks
Copy link
Contributor Author

boaks commented Jul 14, 2022

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.

@boaks boaks changed the title Minor Release 3.6.0 - Scheduled for 14. July 2022 Minor Release 3.6.0 - Available Jul 14, 2022
@sbernard31
Copy link
Contributor

sbernard31 commented Jul 19, 2022

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
Copy link
Contributor Author

boaks commented Jul 20, 2022

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
Copy link
Contributor Author

boaks commented Jul 25, 2022

@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
Copy link
Contributor

sbernard31 commented Jul 26, 2022

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 :

  • new minor : you add new/better behavior without changing default behavior
  • new major : you change default behavior.

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
Copy link
Contributor Author

boaks commented Jul 26, 2022

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
Copy link
Contributor Author

boaks commented Aug 1, 2022

❗❗❗ Important Note: ❗❗❗

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

@boaks boaks changed the title Minor Release 3.6.0 - Available Minor Release 3.6.0 - Available / Fixes CVE-2022-2576 Aug 1, 2022
@jvermillard
Copy link
Contributor

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
Copy link
Contributor

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

@boaks
Copy link
Contributor Author

boaks commented Aug 25, 2022

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
Copy link
Contributor

sbernard31 commented Aug 25, 2022

@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 eclipse-leshan/leshan#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
Copy link
Contributor Author

boaks commented Aug 25, 2022

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

@sbernard31
Copy link
Contributor

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
Copy link
Contributor Author

boaks commented Aug 25, 2022

Just to mention:

CertificateConfigurationHelper

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

@boaks
Copy link
Contributor Author

boaks commented Aug 25, 2022

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

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

@sbernard31
Copy link
Contributor

Yep 🤷

@sbernard31
Copy link
Contributor

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 🤔

@boaks
Copy link
Contributor Author

boaks commented Aug 25, 2022

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
Copy link
Contributor

I checked the pair with :

And both seems to work with this key pair. 🤔

@boaks
Copy link
Contributor Author

boaks commented Aug 25, 2022

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
Copy link
Contributor Author

boaks commented Aug 25, 2022

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
Copy link
Contributor Author

boaks commented Aug 25, 2022

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
Copy link
Contributor

sbernard31 commented Aug 25, 2022

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 😬, I guess it should not, or ?
(I mean the handshake succeed)

@boaks
Copy link
Contributor Author

boaks commented Aug 25, 2022

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
Copy link
Contributor Author

boaks commented Aug 25, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

boaks commented Aug 25, 2022

See PR #2058

@boaks
Copy link
Contributor Author

boaks commented Aug 25, 2022

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
Copy link
Contributor

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

@boaks
Copy link
Contributor Author

boaks commented Aug 25, 2022

You're welcome.

@boaks boaks unpinned this issue Sep 19, 2022
@boaks boaks closed this as completed Sep 23, 2022
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

3 participants