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

Code review #1506

Merged
merged 5 commits into from Jan 31, 2016

Conversation

Projects
None yet
7 participants
@kpp
Contributor

kpp commented Jan 24, 2016

fix: compare sensitive data with sodium_memcmp
fix: replace memset with sodium_memzero for sensitive data
fix: make increment_nonce & increment_nonce_number independent of user-controlled input
fix: make crypto_core more stable agains null ptr dereference

fix: update apt before installing anything
add: comments about hairy code

@towlie

This comment has been minimized.

Show comment
Hide comment
@towlie

towlie Jan 24, 2016

This breaks compatibility with vanilla NaCl https://nacl.cr.yp.to/
Which is something @irungentoo doesn't want?

towlie commented Jan 24, 2016

This breaks compatibility with vanilla NaCl https://nacl.cr.yp.to/
Which is something @irungentoo doesn't want?

@kpp

This comment has been minimized.

Show comment
Hide comment
@kpp

kpp Jan 24, 2016

Contributor

@towlie , do you have any idea how to make memset be non-optimized (and deleted) by compiler? Only sodium_memzero is safe to clear sensitive data before free it. NaCl does not have such function, Sodium does.
@irungentoo , what do you prefer? memset or sodium_memzero?

Contributor

kpp commented Jan 24, 2016

@towlie , do you have any idea how to make memset be non-optimized (and deleted) by compiler? Only sodium_memzero is safe to clear sensitive data before free it. NaCl does not have such function, Sodium does.
@irungentoo , what do you prefer? memset or sodium_memzero?

@GrayHatter

This comment has been minimized.

Show comment
Hide comment
@GrayHatter

GrayHatter Jan 24, 2016

Collaborator

@kpp toxcore has never been shy to write it's own wrappers for functions. Why not create a tox_wrapper_memzero function and then #ifdef depending on if you're using NaCL or libsodium?

Collaborator

GrayHatter commented Jan 24, 2016

@kpp toxcore has never been shy to write it's own wrappers for functions. Why not create a tox_wrapper_memzero function and then #ifdef depending on if you're using NaCL or libsodium?

@kpp

This comment has been minimized.

Show comment
Hide comment
@kpp

kpp Jan 24, 2016

Contributor

@GrayHatter I did not find a similar function in NaCl. Maybe you can do that? Or do you propose me to write something like:

#ifdef USE_SODIUM
sodium_memzero(ptr, size);
#else
memset(ptr, 0, size);
#endif

I will not do that, because that will be unsafe.

Contributor

kpp commented Jan 24, 2016

@GrayHatter I did not find a similar function in NaCl. Maybe you can do that? Or do you propose me to write something like:

#ifdef USE_SODIUM
sodium_memzero(ptr, size);
#else
memset(ptr, 0, size);
#endif

I will not do that, because that will be unsafe.

@kpp kpp closed this Jan 24, 2016

@kpp kpp reopened this Jan 24, 2016

@GrayHatter

This comment has been minimized.

Show comment
Hide comment
@GrayHatter

GrayHatter Jan 24, 2016

Collaborator

@kpp less safe than it is right now?

Collaborator

GrayHatter commented Jan 24, 2016

@kpp less safe than it is right now?

@mrkiko

This comment has been minimized.

Show comment
Hide comment
@mrkiko

mrkiko Jan 26, 2016

I was thinking about libbsd and their explicit_bzero.

mrkiko commented Jan 26, 2016

I was thinking about libbsd and their explicit_bzero.

@alexbakker

This comment has been minimized.

Show comment
Hide comment
@alexbakker

alexbakker Jan 26, 2016

Contributor

@irungentoo any comments?

Contributor

alexbakker commented Jan 26, 2016

@irungentoo any comments?

kpp added some commits Jan 23, 2016

@irungentoo

This comment has been minimized.

Show comment
Hide comment
@irungentoo

irungentoo Jan 27, 2016

Owner

A bit paranoid but that's fine.

I just need to check if using public_key_cmp everywhere doesn't slow down the code too much.

Owner

irungentoo commented Jan 27, 2016

A bit paranoid but that's fine.

I just need to check if using public_key_cmp everywhere doesn't slow down the code too much.

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Jan 27, 2016

I don't understand this comment. Convert endianness of what?

nurupo commented on toxcore/DHT.c in 94d6333 Jan 27, 2016

I don't understand this comment. Convert endianness of what?

This comment has been minimized.

Show comment
Hide comment
@kpp

kpp Jan 27, 2016

Owner
/* Redefinitions of variables for safe transfer over wire. */
#define TOX_AF_INET 2
#define TOX_AF_INET6 10
#define TOX_TCP_INET 130
#define TOX_TCP_INET6 138
Owner

kpp replied Jan 27, 2016

/* Redefinitions of variables for safe transfer over wire. */
#define TOX_AF_INET 2
#define TOX_AF_INET6 10
#define TOX_TCP_INET 130
#define TOX_TCP_INET6 138
@kpp

This comment has been minimized.

Show comment
Hide comment
@kpp

kpp Jan 27, 2016

Contributor

I just need to check if using public_key_cmp everywhere doesn't slow down the code too much.

@irungentoo Well, asymptotic complexity of comparing public keys using memcmp is O(N), using public_key_cmp is ~N, so there are such public keys that in the worst case compares with memcmp as "slow" as they are compared with public_key_cmp. There must be no slow down.

Contributor

kpp commented Jan 27, 2016

I just need to check if using public_key_cmp everywhere doesn't slow down the code too much.

@irungentoo Well, asymptotic complexity of comparing public keys using memcmp is O(N), using public_key_cmp is ~N, so there are such public keys that in the worst case compares with memcmp as "slow" as they are compared with public_key_cmp. There must be no slow down.

@irungentoo irungentoo merged commit 23b0c9c into irungentoo:master Jan 31, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
uint_fast16_t carry = 0U;
for (; i != 0; --i) {
carry += (uint_fast16_t) nonce[i] + (uint_fast16_t) num_as_nonce[i];
nonce[i] = (unsigned char) carry;

This comment has been minimized.

@irungentoo

irungentoo Jan 31, 2016

Owner

I fixed it already but here the array indexes should have been i - 1.

@irungentoo

irungentoo Jan 31, 2016

Owner

I fixed it already but here the array indexes should have been i - 1.

This comment has been minimized.

@kpp

kpp Jan 31, 2016

Contributor

Nice, I will fix it in 1-2 days

@kpp

kpp Jan 31, 2016

Contributor

Nice, I will fix it in 1-2 days

This comment has been minimized.

@kpp

kpp Feb 2, 2016

Contributor

@irungentoo thank you. That was a bad bug. I wrote unit tests for that case. See PR.

@kpp

kpp Feb 2, 2016

Contributor

@irungentoo thank you. That was a bad bug. I wrote unit tests for that case. See PR.

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