Issue with Poly1305-Donna on iOS about libsodium HOT 17 CLOSED

Hello,

Seeing what's around these two lines would make it easier to understand what's going on, but: are you properly prepending zero padding? There should be crypto_secretbox_zerobytes() zero bytes before the message, and the pointer you need to give crypto_secretbox() is the one where the padding starts.

The encrypted result also needs to start with crypto_secretbox_boxzerobytes() (which is always less than crypto_secretbox_zerobytes()) null bytes.

Granted, this API is a bit confusing, so I really need to document it (read: write examples for each operation).

from libsodium.

Specifically, the buffer receiving the output needs to be larger (16 bytes larger, or zerobytes() - boxzerobytes()) than the plaintext message in order to provide authenticated encryption.

from libsodium.

I paste you right away my encryption function so you can see it, I must say I don't remember exactly this confusing api so I need few more minutes to tell if there is this bug or not:

  NSMutableData *encryptedData = [NSMutableData dataWithData:nonce];
  [encryptedData setLength:crypto_secretbox_noncebytes() + crypto_secretbox_zerobytes() + [data length]];

  // Extend the input data to allocate enough space for the authenticator
  NSMutableData *extendedData = [NSMutableData dataWithLength:crypto_secretbox_zerobytes()];
  [extendedData appendData:data];

  ret = crypto_secretbox([encryptedData mutableBytes] + crypto_secretbox_noncebytes(),
                         [extendedData bytes], [extendedData length],
                         [nonce bytes],
                         [key bytes]);

from libsodium.

I seem to have prepended the 0-padding as expected. What's could be confusing in my code is that I also prepend the nonce just before the padding.

Moreover in my first message I didn't paste you the end of my function but I eventually remove the zero padding before returning, the whole decryption function was:

  NSMutableData *data = [NSMutableData dataWithLength:[encryptedData length] - crypto_secretbox_noncebytes()];

  ret = crypto_secretbox_open([data mutableBytes],
                              [encryptedData bytes] + crypto_secretbox_noncebytes(),
                              [encryptedData length] - crypto_secretbox_noncebytes(),
                              [encryptedData bytes],
                              [key bytes]);
  if (ret != 0)
    return nil;

  return [NSData dataWithBytes:[data bytes] + crypto_secretbox_zerobytes()
                        length:[data length] - crypto_secretbox_zerobytes()];

from libsodium.

Here is an example: https://gist.github.com/jedisct1/6178503

Can you check if it works as expected on your environment?

Please note that the first 16 bytes (crypto_secretbox_BOXZEROBYTES) of cpad are always 0. So you don't need to store these if you like. But the next 16 bytes (crypto_secretbox_ZEROBYTES - crypto_secretbox_BOXZEROBYTES) are required to verify the integrity of the ciphertext.

from libsodium.

I tried your code and it works but like I previously said it's maybe more complicated than that, here are my observations:

• I really cannot use your example in the exact same context than the context where my code fails, so I executed it at the start of my app
• By using sodium_init() in your code crypto_onetimeauth_pick_best_implementation() picks crypto_onetimeauth_poly1305_53_implementation. In this case your code works, and my code also works.
• If I remove the call to sodium_init() to force the use of crypto_onetimeauth_poly1305_donna_implementation, your code works, my code fails in one instance, but most of the time my code works, this is one point single call in my code base which make it fails repeatedly. Based on my previous code, I even found a workaround, if I explicitly copy the encryptedData before calling the decrypt function it then works:

NSData *copyData = [NSData dataWithBytes:[encryptedData bytes] length:[encryptedData length]];
decrypt(copyData,...);

(and by the way thanks for your help, it's a very strange problem, I waited a bit before reporting it, with the old implementation I never had any problems).

Of course based on the choice picked by sodium_init() it's maybe expected to have some problems with the alternative implementation, I must say I don't call sodium_init() in my code (I explicitly set it to crypto_onetimeauth_poly1305_53_implementation).

from libsodium.

Code that sometimes works, sometimes segfaults is quite often a pain in the ass to debug :)

It it's also almost always due to writing to unallocated memory.

Can you try to enable Guard Malloc and see if it trips?

from libsodium.

Based on instruments (tool from XCode) I don't seem to have a leak at this point but I don't know how effective it is though.

I couldn't use Guard Malloc because it only works in the simulator (http://stackoverflow.com/questions/9436854/guard-malloc-doesnt-work) and unfortunately my app can't run in the simulator.

from libsodium.

Maybe you can trim down your app to something minimal that runs in the simulator.
Instruments can detect leaks, but not {under|over}flows.

from libsodium.

Indeed, I can't be sure it doesn't come from my app, this is a big app with a lot of code and lot of networking involved I really can't easily isolate this problem, I only can reproduce the issue after a lot of mandatory networking protocols exchanges.

Maybe close this issue and if nobody reopens it, it meant it was probably an error in my code, and otherwise there will still be a trace of the problem in the closed issues.

from libsodium.

Yes, a smaller test case would be really useful in order to diagnose and fix this no matter what.
I'm going to close this issue, but feel free to reopen it if you have more insights.
Have a nice day, Seb!

from libsodium.

Some ARM systems are not too fond of unaligned word memory accesses. This is a long shot, but try replacing every instance of U8TO32_LE and U32TO8_LE in auth_poly1305_donna.c with fU8TO32_LE_SLOW and fU32TO8_LE_SLOW.

from libsodium.

Hello Samuel,

I tried as you proposed and it seems to fix the issue, here are the changes I tried https://gist.github.com/seb-m/6179677

from libsodium.

Good catch. And this is not specific to ARM CPUs. So, well, I guess we have to use the slow version except on CPUs known to support unaligned word memory accesses.

from libsodium.

Hi Seb.

Can you try the current git master?

from libsodium.

Yes. It's all good!

from libsodium.

Excellent.

Thanks a lot for spotting this issue, and thanks a million to Samuel for the fix!

from libsodium.