Suggested changes to PurpleCipherOps and need for code review

Ethan Blanton elb at pidgin.im
Wed May 1 11:05:07 EDT 2013


Tomasz Wasilczyk spake unto us the following wisdom:
> In my opinion, current PurpleCipherOps structure is incomplete and
> could be improved. I think, most of these is related to 2.x.y backward
> compatibility, so we could tidy things up for 3.0.0.
> 
> My suggestions:
> - merge set_key() with set_key_with_len() (using ssize_t len instead
> of size_t, if we don't want to provide length)
> - add get_digest_size(), if we don't want to guess buffer length for digest()
> - add (optional) length parameter for set_salt()

All of this seems fine to me.  I'm not sure the lengths should be
optional, but I'd have to see how you intend to derive them to be
sure.  (Note that strlen() isn't really reasonable here.)

> - add output_size parameter to encrypt() and decrypt() to prevent
> buffer overflow (outlen is used to store actual data length after
> encryption/decryption)

A common technique for this is to use outlen as an in/out parameter;
it contains the buffer size at call time, and the bytes written at
return time.  It may or may not be applicable here.

> - add some reserved fields at the end of struct, there are none left
> (this struct should be hidden anyway, but maybe not this time)

I'd say go ahead and hide it while you're in there.

> I need it for master password branch, where I would like to add
> lacking "master password" feature. I decided to use PBKDF2 for turning
> passphrase into encryption key as it seems to be wide standard and can
> be adjusted later. Thus, when setting up new master passphrase, there
> will be some information stored: used key derivation method (only
> pbkdf2-sha256, for now) and specific data for selected method (salt,
> iterations count). Default iterations count parameter could be changed
> later. Encryption will be done with AES (provided by NSS and GnuTLS).

This seems broadly appropriate to me.

> I've done a draft implementation of pbkdf2 using source code grabbed
> from gnutls. For a reference, I also done independent implementation
> using NSS library. Please review it, especially gnutls one (patch is
> made against my masterpassword branch). Though, it hasn't been
> triple-checked yet, so it may still contain minor bugs.

I haven't had time to review this and don't know when I might.

Ethan




More information about the Devel mailing list