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

Add Z85 decoding #243

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add Z85 decoding #243

wants to merge 18 commits into from

Conversation

gregorni
Copy link
Contributor

@gregorni gregorni commented May 28, 2023

I'm still a newbie at Rust, so feel free to criticize.

Due to how the z85 crate works, the decoding fails whenever the input strings length is not a multiple of 5. Not sure if that's supposed to happen 🤷 Probably a bug on their part

Related to #61

@gregorni
Copy link
Contributor Author

gregorni commented May 28, 2023

Due to how the z85 crate works, the decoding fails whenever the input strings length is not a multiple of 5

This is why the test are failing right now

Edit: I fixed it, now the tests are failing because of something else

Edit no. 2: all tests pass now

@gregorni gregorni changed the title Implemented Z85 decoder Add Z85 decoding May 28, 2023
@gregorni gregorni marked this pull request as draft May 28, 2023 12:57
@bee-san
Copy link
Owner

bee-san commented May 28, 2023

@SkeletalDemise is the crypto person, I just build the framework

can you review this skeletal? 🙏🏻 We might need some more successful tests, purely so we know it can decrypt some more advanced Z85 strings :D

@gregorni
Copy link
Contributor Author

Due to how the z85 crate works, the decoding fails whenever the input strings length is not a multiple of 5

This is really the most limiting factor right now, it's why I've marked this PR as draft. I'm thinking about forking the crate and fixing the problem if I can, since the original developer doesn't seem to be very active on GitHub anymore.

It's really stupid, cause it renders the crate basically useless for trying to decode unknown strings.

@SkeletalDemise
Copy link
Collaborator

Due to how the z85 crate works, the decoding fails whenever the input strings length is not a multiple of 5.

That's how it's supposed to work. Z85 is a base85 variant created by a company called ZeroMQ.
Their official spec says that the input string should be divisible by 5.

https://rfc.zeromq.org/spec/32/

The binary frame SHALL have a length that is divisible by 4 with no remainder. The string frame SHALL have a length that is divisible by 5 with no remainder. It is up to the application to ensure that frames and strings are padded if necessary.

Their example C implementation even has the same check:
https://github.com/zeromq/rfc/blob/master/src/spec_32.c

//  Accepts only strings bounded to 5 bytes
if (strlen (string) % 5)
   return NULL;

The problem is that the majority of online encoders/decoders like CyberChef and dcode.fr completely ignore the spec and simply swap out the base85 alphabet for ZeroMQ's one.

https://www.dcode.fr/ascii-85-encoding

ZeroMQ's conversion function is different from ASCII85, this page only uses the alphabet.

The question is whether we should follow ZeroMQ's spec and have our decoder be more rigid or do what online decoders do and ignore the RFC so that we can decode more Z85 strings.

@bee-san
Copy link
Owner

bee-san commented May 28, 2023

Due to how the z85 crate works, the decoding fails whenever the input strings length is not a multiple of 5.

That's how it's supposed to work. Z85 is a base85 variant created by a company called ZeroMQ. Their official spec says that the input string should be divisible by 5.

https://rfc.zeromq.org/spec/32/

The binary frame SHALL have a length that is divisible by 4 with no remainder. The string frame SHALL have a length that is divisible by 5 with no remainder. It is up to the application to ensure that frames and strings are padded if necessary.

Their example C implementation even has the same check: https://github.com/zeromq/rfc/blob/master/src/spec_32.c

//  Accepts only strings bounded to 5 bytes
if (strlen (string) % 5)
   return NULL;

The problem is that the majority of online encoders/decoders like CyberChef and dcode.fr completely ignore the spec and simply swap out the base85 alphabet for ZeroMQ's one.

https://www.dcode.fr/ascii-85-encoding

ZeroMQ's conversion function is different from ASCII85, this page only uses the alphabet.

The question is whether we should follow ZeroMQ's spec and have our decoder be more rigid or do what online decoders do and ignore the RFC so that we can decode more Z85 strings.

Future me says "why not both" and make it so the more rigid one runs first, but we don't support that currently :(

I think we should go less rigid, sadly for our target audience most people will use online encoders to generate the texts, and even if they are wrong they will expect us to support it :(

@SkeletalDemise
Copy link
Collaborator

I agree, 99% of people are not using ZeroMQ's implementation sadly.
Perhaps in the future we could check the format and print Successfully matched Z85's spec and This Z85 string didn't match Z85's spec but Ares decoded it for you

Since there are no Z85 crates that match CyberChef/dcode.fr/etc. we need to make our own.

@gregorni
Copy link
Contributor Author

Should I then close this PR until we have our own, more flexible crate for z85 decoding?

@SkeletalDemise
Copy link
Collaborator

No, leave it open.

@gregorni
Copy link
Contributor Author

Okay. Should I rename it to Z85Strict or something like that, so we can call the non-strict one in the future Z85Lax or so?

Or should both decode modes be in the same decoder?

@SkeletalDemise
Copy link
Collaborator

Don't worry about that. I don't even know if we'll add two decoders.

@gregorni
Copy link
Contributor Author

Okay, I'll just continue to work on this PR, then.

@gregorni gregorni marked this pull request as ready for review May 29, 2023 21:13
@gregorni
Copy link
Contributor Author

This PR already works for input strings of length 5, so I'll mark it as ready.

@gregorni
Copy link
Contributor Author

gregorni commented Jun 9, 2023

Can someone review this?

Not sure why the tests are failing...

@bee-san
Copy link
Owner

bee-san commented Aug 21, 2023

cargo lint and cargo clippy are failing

@gregorni
Copy link
Contributor Author

Cargo-deny is failing because there are duplicate entries of the same crate in Cargo.lock. I tried to remove them manually, but cargo run puts them back in there again. What do I do?

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

Successfully merging this pull request may close these issues.

3 participants