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

Fix Monero address validation (#1652) #1683

Merged
merged 1 commit into from Sep 23, 2018

Conversation

Projects
None yet
3 participants
@Technohacker
Contributor

Technohacker commented Sep 13, 2018

Add a RegexAddressValidator and tests

@matt2134711

matt2134711 suggested changes Sep 13, 2018 edited

Monero addresses are encoded in base58, so using [0-9A-Za-z] would still let some bad characters through (0, I, O, and l are never in an address).

Also the last few bytes are a checksum so it would be nice if that was validated too.

@Technohacker

This comment has been minimized.

Contributor

Technohacker commented Sep 14, 2018

This might take me a while, but I'll try getting it done :)
If anyone gets it done, I'd highly suggest notifying all CryptoNote coins to follow suit as well

@matt2134711

This comment has been minimized.

matt2134711 commented Sep 14, 2018

We already have Bouncy Castle as a dependency, which provides all of the cryptographic primitives needed (just keccak-256 IIRC), but I'm not sure if Bitcoin's base58 decoding class can be used as it is slightly different.

I can have a go at implementing this today, if you don't mind.

@Technohacker

This comment has been minimized.

Contributor

Technohacker commented Sep 14, 2018

Sure, I'll see if I can get an implementation done as well 😄

@Technohacker

This comment has been minimized.

Contributor

Technohacker commented Sep 14, 2018

I've made changes to avoid the invalid characters (and made it a utility class for other CryptoNotes). What remains is the checksum. CryptoNote doesn't do decoding like Bitcoin (in blocks of bytes rather than as a whole), and I'm not sure if adding the decoder only for CryptoNote addresses is warrantable. Of course on the other hand there's a risk of putting a wrong (but valid) address.

@matt2134711

This comment has been minimized.

matt2134711 commented Sep 14, 2018

The decoder shouldn't be too complicated, the JavaScript on luigi1111's site is a good reference.

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 14, 2018

@matt2134711 @Technohacker We are in the process to remove Bouncy Castle as security provider as with Java 10 they removed support for the extension mechanism (which was used to add the BC signed jar) and we have not found a way yet to include it in the fat jar for the binary otherwise. So we will use the default provider which have less features but covers the features we use atm. I doubt that keccak-256 will be supported there.

@matt2134711

This comment has been minimized.

matt2134711 commented Sep 14, 2018

@ManfredKarrer Would it be okay to depend on something like keccakj instead?

@ManfredKarrer

This comment has been minimized.

Member

ManfredKarrer commented Sep 14, 2018

@matt2134711 Is there a jar on a public repo? If so, yes we add that dependency.

Fix Monero address validation (#1652)
Add a CryptonoteAddressValidator and tests
The current implementation accepts valid standard addresses and
subaddresses on the basis of valid Base58 characters and length.
Verification of the address checksum is pending

@Technohacker Technohacker force-pushed the Technohacker:fix-monero-validation branch from cd7bd2f to aebf283 Sep 15, 2018

@Technohacker

This comment has been minimized.

Contributor

Technohacker commented Sep 15, 2018

I've (force-)pushed my changes for the new CryptonoteAddressValidator. Checksum verification is still pending

@ridd84 ridd84 referenced this pull request Sep 17, 2018

Merged

List Croat (CROAT) #1691

@ManfredKarrer ManfredKarrer self-requested a review Sep 23, 2018

@ManfredKarrer

ACK for first part of validation.
Validation including the check sum will be delegated to a new PR.

@ManfredKarrer ManfredKarrer merged commit c2df7ce into bisq-network:master Sep 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@blur-network blur-network referenced this pull request Sep 25, 2018

Merged

List Blur (BLUR) #1723

@Technohacker Technohacker referenced this pull request Oct 7, 2018

Closed

For October 2018 #143

/**
* {@link AddressValidator} for Base58-encoded Cryptonote addresses.
*
* @author Chris Beams

This comment has been minimized.

@Technohacker

Technohacker Oct 10, 2018

Contributor

Damn it, I forgot to change this. Could I post a follow-up PR to fix this?

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Oct 10, 2018

Member

What did u forget? Yes feel free for another PR.

This comment has been minimized.

@Technohacker

Technohacker Oct 11, 2018

Contributor

The author 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment