keyring symmetric crypt functions fail silently on non-bsize-aligned data

[GoogleCodeExporter]

What steps will reproduce the problem?
1. use keyring to setup a state, and a encrypt a buffer that is not a
multiple of the algorithm blocksize.

What is the expected output? What do you see instead?

ideally, i would like to see an error.

instead, the crypt function silently returns.  this leaves the developer
thinking (hoping) his sensitive information that he wanted encrypted is in
fact encrypted and that he can send it to some insecure place.  instead the
crypto functions left the buffer in plaintext.  this might cause plaintext
data to be sent to insecure places.

libinterp/keyring.c seems to do the checks.  perhaps instead of returning,
an error("not multiple of blocksize") can be raised.  this is a change of
behaviour but seems helpful to developers and security.

Original issue reported on code.google.com by mechiel@ueber.net on 16 Jul 2009 at 9:47

[GoogleCodeExporter]

i agree it would be better to generate an error.
i ought to have changed that when i added some more functions to Keyring.

Original comment by Charles....@gmail.com on 16 Jul 2009 at 1:37

• Changed state: Accepted

[GoogleCodeExporter]

see how you fare with the version attached

Original comment by Charles....@gmail.com on 18 Jul 2009 at 9:52

Attachments:

• keyring.c

[GoogleCodeExporter]

just read the changes, will run the code after this post.

wrong calling of blowfish cbc can cause a panic.  keyring.c doesn't enforce 
that the
buffer be a multiple of the block size.  the libsec's blowfish asserts 
alignment:

void
bfCBCencrypt(uchar *buf, int n, BFstate *s)
{
        int i;
        uchar *p;
        u32int bo[2], bi[2], b;

        assert((n & 7) == 0);

aes cbc is fine as libsec handles that case (perhaps that caused the confusion).

tiny nit:  in Keyring_desecb, i think the cast in "if(f->state ==
(Keyring_DESstate*)H)" can be removed just as in all the other cases?

although it's now more likely that a programmer doesn't pass a nil/H state to 
the
encryption function (since setup throws an error on failure), wouldn't it be 
nicer if
crypting with nil state would throw an error too?  my imagination on why you 
would
call the functions with nil state fails me.  returning when buffer is nil makes 
sense
though.  i also wonder if taking min(f->buf->len, f->n) is the kindest thing to 
do to
the programmer.  if f->n is what the programmer requested, and keyring does 
something
differently if the buffer is too small.  changing "n" won't leak sensitive data 
of
course, so it's not really harmful.

Original comment by mechiel@ueber.net on 18 Jul 2009 at 10:31

[GoogleCodeExporter]

just tested aes cbc and des.  they seem to work fine.  errors are caught as 
intended.

Original comment by mechiel@ueber.net on 18 Jul 2009 at 10:56

[GoogleCodeExporter]

here's another variant with more error checking

Original comment by Charles....@gmail.com on 5 Aug 2009 at 10:20

Attachments:

• keyring.c

[GoogleCodeExporter]

i've installed that at 343

Original comment by Charles....@gmail.com on 5 Aug 2009 at 6:59

• Changed state: Fixed