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

CBOR serialisation crashes #65

Open
marius-se opened this issue Feb 11, 2024 · 4 comments
Open

CBOR serialisation crashes #65

marius-se opened this issue Feb 11, 2024 · 4 comments

Comments

@marius-se
Copy link

Similar to SwiftCBOR trying to serialise a lot of invalid CBOR data crashes the entire app. Here is a small reproducible project:

potent_cbor_crash.zip

I saw that the Go CBOR library has different serialisation modes (one specifically for WebAuthn, which is the target application I'm working on) and a "wellformed" check (which IIRC is optional), maybe we could implement something similar for PotentCBOR to work around this crash?

@kdubb
Copy link
Collaborator

kdubb commented Feb 11, 2024

I'm very much in support of doing all we can to ensure ill-formed data is reported but doesn't crash the app.

I assume throwing an error is fine?

Thanks for reproducer!

@marius-se
Copy link
Author

Cool! Yeah throwing an error should be fine! Adding the wellformed checks will likely decrease performance, so maybe we should make it optional?

@kdubb
Copy link
Collaborator

kdubb commented Feb 13, 2024

I'm not sure all the checks that Go CBOR does but I would say safety is a concern and CBOR is generally used for the externally sourced data (e.g. network, etc.)... disabling safety for a small performance gain is not really something that should be done.

I ran your reproducer and for that case it seems to simply need a loop counter and to check it against a maximum on every nested call. I can't imagine that disabling that would show any performance advantage.

@marius-se
Copy link
Author

marius-se commented Feb 13, 2024

Oh okay! I don't really know how CBOR or the safety checks work under the hood, but if it's really just a nested level check you're right and it shouldn't kill performance!

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