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

fix sodium constant type (size_t instead of ulong_long) #113

Merged
merged 1 commit into from
Oct 8, 2014
Merged

fix sodium constant type (size_t instead of ulong_long) #113

merged 1 commit into from
Oct 8, 2014

Conversation

paddor
Copy link
Contributor

@paddor paddor commented Oct 3, 2014

It seems like libsodium exposes all constants as size_t (I checked at least the files src/libsodium/crypto_box/curve25519xsalsa20poly1305/box_curve25519xsalsa20poly1305_api.c and src/libsodium/crypto_generichash/blake2/generichash_blake2_api.c of libsodium). So this FFI declaration seems wrong and is probably the reason for issue #112.

I haven't actually tested this. @patte Could you test this on your Raspberry Pi?

@tarcieri Does this sound plausible? I don't know a lot about C.

@jedisct1
Copy link
Contributor

jedisct1 commented Oct 4, 2014

unsigned long long is used for plaintext & ciphertext lengths. This is utterly annoying, but required to retain binary compatibility with NaCl.
size_t is used everywhere else to represent the size of an object.

@Asmod4n
Copy link
Contributor

Asmod4n commented Oct 4, 2014

For me it looks like ulong_long is used where something can come from a external source and size_t is used when it happens on the local machine.

@paddor
Copy link
Contributor Author

paddor commented Oct 4, 2014

@jedisct1 I don't get it. The libsodium source says size_t in this case. See https://github.com/jedisct1/libsodium/blob/master/src/libsodium/crypto_box/curve25519xsalsa20poly1305/box_curve25519xsalsa20poly1305_api.c#L8.

Why does rbnacl have to use ulong_long here?

@patte
Copy link

patte commented Oct 4, 2014

@paddor
I tested your fork on the raspberry and I can confirm that it works! nicely done!
Here the versions I used:
zmq (4.0.4)
celluloid (0.16.0 e42a6b3) (paddor/celluloid)
celluloid-io (0.16.0 567b769) (paddor/celluloid-io)
celluloid-zmq-zap (0.0.1)
rbnacl (3.1.2 00de2f7)
rbnacl-libsodium (1.0.0)

@paddor
Copy link
Contributor Author

paddor commented Oct 7, 2014

@tarcieri @jedisct1 Any updates here?

@tarcieri
Copy link
Contributor

tarcieri commented Oct 8, 2014

@paddor I haven't had time to investigate this in-depth but it would seem this change doesn't actually honor the libsodium type signatures and changing it to :size_t across the board could cause (potentially invisible) breakages.

This seems like something that needs to be handled with care.

@Asmod4n
Copy link
Contributor

Asmod4n commented Oct 8, 2014

All constants in NaCl and libsodium are of type size_t @tarcieri
Von einem mobilen Gerät gesendet

Am 08.10.2014 um 03:56 schrieb Tony Arcieri notifications@github.com:

@paddor I haven't had time to investigate this in-depth but it would seem this change doesn't actually honor the libsodium type signatures and changing it to :size_t across the board could cause (potentially invisible) breakages.

This seems like something that needs to be handled with care.


Reply to this email directly or view it on GitHub.

@paddor
Copy link
Contributor Author

paddor commented Oct 8, 2014

@tarcieri Thanks for the answer. Your concerns make sense, of course.

I just logged the usage of the method #sodium_constant to see for what constants it's used. I ended up with this list:

crypto_box_curve25519xsalsa20poly1305_noncebytes
crypto_box_curve25519xsalsa20poly1305_zerobytes
crypto_box_curve25519xsalsa20poly1305_boxzerobytes
crypto_box_curve25519xsalsa20poly1305_beforenmbytes
crypto_box_curve25519xsalsa20poly1305_publickeybytes
crypto_box_curve25519xsalsa20poly1305_secretkeybytes
crypto_secretbox_xsalsa20poly1305_keybytes
crypto_secretbox_xsalsa20poly1305_noncebytes
crypto_secretbox_xsalsa20poly1305_zerobytes
crypto_secretbox_xsalsa20poly1305_boxzerobytes
crypto_sign_ed25519_seedbytes
crypto_sign_ed25519_publickeybytes
crypto_sign_ed25519_secretkeybytes
crypto_sign_ed25519_bytes
crypto_onetimeauth_poly1305_bytes
crypto_onetimeauth_poly1305_keybytes
crypto_hash_sha256_bytes
crypto_hash_sha512_bytes
crypto_generichash_blake2b_bytes_min
crypto_generichash_blake2b_bytes_max
crypto_generichash_blake2b_keybytes_min
crypto_generichash_blake2b_keybytes_max
crypto_pwhash_scryptsalsa208sha256_saltbytes
crypto_auth_hmacsha256_bytes
crypto_auth_hmacsha256_keybytes
crypto_auth_hmacsha512256_bytes
crypto_auth_hmacsha512256_keybytes

I just git grep -B1'd all of them in the libsodium sources and can confirm that they're all of type size_t.

@tarcieri
Copy link
Contributor

tarcieri commented Oct 8, 2014

@paddor thanks for doing that! @jedisct1 I'm confused what you were talking about then.

Going to go ahead and merge this...

tarcieri added a commit that referenced this pull request Oct 8, 2014
fix sodium constant type (size_t instead of ulong_long)
@tarcieri tarcieri merged commit 8a0853f into RubyCrypto:master Oct 8, 2014
@paddor
Copy link
Contributor Author

paddor commented Oct 8, 2014

@tarcieri No problem, and thanks!

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.

5 participants