Re: An encryption patch to ensure there are no zero's in data CRC

[Craig Mason-Jones]

Hi Jose,

Just trying this patch, but I seem to be stuck.

The problem:

When we receive the b64 encoded record, that record is:

PASSWORD CRC PADDING* SALT

SALT and CRC are defined size. But PASSWORD and PADDING are variable.

From the b64 decode, I know the full length, but I've still got 2 
variables: PASSWORD len and PADDING len.

If the last byte of CRC is a 0, it's not possible to distinguish it from 
PADDING.

There are three possibilities:

1. CRC will never contain a zero byte (but I don't know enough about the 
maths of the CRC calc to guarantee that); or

2. We enforce a no-zero bytes in CRC rule;

3. We change the data format to eliminate this issue: we could just add 
a single byte somewhere (right at the start?) to indicate the length of 
padding. BUT this would be breaking-backwards-incompatible. (Although we 
could indicate this encoding with a difference encryption prefix eg 
'encrypted2-', or start trying this method, and fallback to the old 
method on failure)

The patch I sent does rule 2, in a backwards compatible fashion: when 
encrypting, it changes any 0's in CRC to 1. When checking, it does the 
same on the calculated CRC (it doesn't need to do this on the extracted 
CRC since if that contained a 0, we would be hitting the bug we're 
trying to avoid, and our CRC check is doomed to fail).

Open to suggestions?

Kind regards,

Craig


On 2023/09/04 06:54, Jose E. Marchesi wrote:
> > Hi,
> >
> > This patch removes any zero bytes from the CRC that is calculated for
> > a data checksum.
> >
> > I remove them when the CRC is calculated for encryption, and then
> > remove them too from the calculated CRC for decryption. I don't know
> > whether it's impossible for a CRC to contain a zero, but since we're
> > using strlen on the encrypted data, if there were a zero in the CRC,
> > the decryption would fail.
> Woulnd't it be better to not use strlen on the encrypted data?  We
> ultimately print out an ASCII encoded version of that data, and strlen
> would make sense there, but definitely not in the raw bytes.
>
> > Craig
> >
> > --- ../recutils/src/rec-crypt.c 2023-09-03 17:56:51.475134762 +0200
> > +++ src/rec-crypt.c     2023-09-03 18:36:37.110005393 +0200
> > @@ -41,6 +41,14 @@
> >                          strlen (REC_ENCRYPTED_PREFIX)) == 0));
> >   }
> >   
> > +void remove_zero_bytes_from_uint32(uint32_t *up);
> > +void remove_zero_bytes_from_uint32(uint32_t *up) {
> > +  char *p = (char*)up;
> > +  for (int i=0; i<sizeof(uint32_t); i++) {
> > +    if (p[i]==0) p[i]=1;
> > +  }
> > +}
> > +
> >   bool
> >   rec_encrypt (char   *in,
> >                size_t  in_size,
> > @@ -69,6 +77,10 @@
> >   #if defined WORDS_BIGENDIAN
> >     crc = rec_endian_swap (crc);
> >   #endif
> > +  /* We append the CRC to the string, but use strlen in
> > +     decryption, so we ensure there aren't any 0's in
> > +     the crc */
> > +  remove_zero_bytes_from_uint32(&crc);
> >   
> >     real_in_size = in_size + 4;
> >     real_in = malloc (real_in_size + 4);
> > @@ -231,7 +243,11 @@
> >   #if defined WORDS_BIGENDIAN
> >         crc = rec_endian_swap (crc);
> >   #endif
> > -      if (crc32 (*out, outlen - 4) != crc)
> > +      // We don't have to remove any zero's from the retrieved CRC value,
> > +      // since it was located by strnlen, so by definition it won't contain 0
> > +      uint32_t calculatedCrc = crc32 (*out, outlen - 4);
> > +      remove_zero_bytes_from_uint32(&calculatedCrc);
> > +      if (calculatedCrc != crc)
> >           {
> >             gcry_cipher_close (handler);
> >             return false;