mORMot Open Source

souchaud

Member

Registered: 2015-05-05

Posts: 6

SynCrypto, EncryptPKCS7 and RawByteString

Hi,

I want to use SynCrypto's AESCBC.
I wanted to use EncryptPKCS7 function but it takes RawByteString argument (like some other func of SynCrypto).
My input data is an array of byte. So to avoid several copy when converting the array of byte into and from the RawByteString (I think it is the only clean solution?), I reimplemented the EncryptPKCS7/DecryptPKCS7 with array of byte.

So my question is: why [En|De]cryptPKCS7 uses RawByteString rather than array of byte? Am I missing something :-) ?

Here is the reimplemented funcs ( :

TCryptoMormot = class
  private
    FAES : TAESCBC
  ...

function TCryptoMormot.EncryptPKCS7(
  const AInput: TBytes;
  AIVAtBeginning: boolean): TBytes;
var len, padding, iv: cardinal;
    P: Pointer;
    LIV: TAESBlock;
begin
  if FAES = nil then
    Raise Exception.Create(RS_AES_NIL);

  // use PKCS7 padding, so expects AESBlockSize=16 bytes blocks
  len := length(AInput);
  padding := AESBlockSize - (len and (AESBlockSize-1));
  if AIVAtBeginning then
    iv := AESBlockSize else
    iv := 0;
  SetLength(result, iv + len + padding);
  if AIVAtBeginning then begin
    FillRandom(LIV);
    FAES.IV := LIV;
    PAESBlock(result)^ := FAES.IV;
  end;
  move(Pointer(AInput)^, PByteArray(result)^[iv], len);
  FillChar(PByteArray(result)^[iv+len], padding, padding);
  // encryption
  P := @PByteArray(result)^[iv];
  FAES.Encrypt(P, P, len+padding);
end;

function TCryptoMormot.DecryptPKCS7(
  const AInput: TBytes;
  AIVAtBeginning: boolean): TBytes;
var len,iv: integer;
begin
  if FAES = nil then
    Raise Exception.Create(RS_AES_NIL);

  // validate input
  len := length(AInput);
  if (len < AESBlockSize) or (len and (AESBlockSize-1) <> 0) then
    raise ESynCrypto.Create('Invalid content');

  // decrypt
  if AIVAtBeginning then begin
    FAES.IV := PAESBlock(AInput)^;
    dec(len, AESBlockSize);
    iv := AESBlockSize;
  end else
    iv := 0;
  SetLength(result, len);
  FAES.Decrypt(@PByteArray(AInput)^[iv], result, len);
  // delete right padding
  if ord(result[len - 1]) > AESBlockSize then
    raise ESynCrypto.Create('Invalid content');
  SetLength(result, len - ord(result[len - 1]));
end;

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,661

Website

Re: SynCrypto, EncryptPKCS7 and RawByteString

TBytes are dynamic array of bytes so:
- they would first initialize the buffer with 0, even if it is not necessary since most of the time we will just fill them with data;
- they won't use proper COW like RawByteString when you change an item, just basic COW for the variable reference;
- some part of the RTL is not even thread safe (reference counting is not using atomic operations), whereas RawByteString is safer.

So TBytes are slightly slower, and may be less reliable than RawByteString in some use cases.
Those are one of the reasons why we rely on them, even for BLOB storage.

I've added RawByteStringToBytes() and BytesToRawByteString() functions.
See http://synopse.info/fossil/info/5f657445d7

souchaud

Member

Registered: 2015-05-05

Posts: 6

Re: SynCrypto, EncryptPKCS7 and RawByteString

Ok, thank you for the precisions, I am a beginner with Pascal so I don't know what is best to use.

There are still some things that tickles me:
- If EncryptPKCS7 uses RawByteString, to avoid the copy and be faster than TBytes, this force the user to use RawByteString from the beginning, which can be cumbersome.
- Actually, like in the SynCrypto TAESAbstract.Encrypt procedure, I should have use buffer instead of TBytes. And implement sthg like this :
  procedure EncryptPKCS7(BufIn, BufOut: pointer; SizeIn: cardinal, out SizeOut: cardinal; AIVAtBeginning: boolean);
- RawByteString has been deprecated by delphi for mobile app

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,661

Website

Re: SynCrypto, EncryptPKCS7 and RawByteString

Indeed.

I've added overloaded TAESAbstract.EncryptPKCS7 and TAESAbstract.DecryptPKCS7 methods, using TBytes as input/output kind of parameters.
See http://synopse.info/fossil/info/cd3c7ac4c7

Also included EncryptPKC7Buffer() method, if needed.
For the "decrypt" version, since the output buffer would be resized after process (i.e. remove padding data), I did not implement a method yet.

About RawByteString deprecation, it is IMHO a real PITA.
Something which I like not at all, even if you can re-enable its use, using Andy's low level patch.
This is one the reasons why we look at FPC compatibility more than NextGen support - even if we provide NextGen access, but for the clients only.

souchaud

Member

Registered: 2015-05-05

Posts: 6

Re: SynCrypto, EncryptPKCS7 and RawByteString

Great, your implementing my needs on the fly :-) !

> Also included EncryptPKC7Buffer() method, if needed.
> For the "decrypt" version, since the output buffer would be resized after process (i.e. remove padding data),
> I did not implement a method yet.
For the DecryptPKCS7Buffer func (and also for the EncryptPKCS7Buffer func) you could alloc the output buffer in the func and gives the ownership to the caller. It is not a big deal if the remaining padding bytes are unused, actually, the implementation with RawBytesString and TBytes does the same, isn't?

I think there is a bug in your commit (I ran into the same when implementing with tbytes). The padding size must be get using len-1 rather than len as tbytes start from 0 whereas RawByteString start from 1.

function TAESAbstract.DecryptPKCS7(const Input: TBytes;
  IVAtBeginning: boolean): TBytes;
var len,iv: integer;
begin
  // validate input
  len := length(Input);
  if (len<AESBlockSize) or (len and (AESBlockSize-1)<>0) then
    raise ESynCrypto.Create('Invalid content');
  // decrypt
  if IVAtBeginning then begin
    fIV := PAESBlock(Input)^;
    dec(len,AESBlockSize);
    iv := AESBlockSize;
  end else
    iv := 0;
  SetLength(result,len);
  Decrypt(@PByteArray(Input)^[iv],pointer(result),len);
  // delete right padding
  if ord(result[len])>AESBlockSize then
    raise ESynCrypto.Create('Invalid content');
  SetLength(result,len-result[len]);
end;

fix :

  if ord(result[len-1]) > AESBlockSize then
    raise ESynCrypto.Create('Invalid content');
  SetLength(result,len-ord(result[len-1]));

Last edited by souchaud (2015-05-06 09:43:11)

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,661

Website

Re: SynCrypto, EncryptPKCS7 and RawByteString

My mistake!

I've tried to fix - and also enhance the latest modifications.

See http://synopse.info/fossil/info/b8381f31f0

Thanks for your help!

souchaud

Member

Registered: 2015-05-05

Posts: 6

Re: SynCrypto, EncryptPKCS7 and RawByteString

Ok,

Actually, I think too that it is better if the caller of EncryptPKC7Buffer alloc the output buffer.
I 'll try to implement the DecryptPKC7Buffer like this when I'll have time.

function TAESAbstract.DecryptPKCS7Buffer(Input,Output: Pointer; InputLen,OutputLen: cardinal;
  IVAtBeginning: boolean): Cardinal; // return the real output buffer len

But yes, I think too that an api like this is a bit confusing for the user (OutputLen param > returned output buffer len).

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,661

Website

Re: SynCrypto, EncryptPKCS7 and RawByteString

If your purpose is to use TBytes, just use the new method.
If your purpose is to work with binary buffers, use the pointer method.
I confirm that such an API may be confusing...

In practice, Setlength() is immediate, thanks to how FastMM4 handles reallocation of a memory block.
So there is no speed penalty with the current implementation.
As a result, I would not mind using this TBytes version. There would be no speed gain.

souchaud

Member

Registered: 2015-05-05

Posts: 6

Re: SynCrypto, EncryptPKCS7 and RawByteString

I need to use binary buffer (at first I though TBytes was binary buffer but it is not).
I'll use the TBytes method for DecryptPKCS7, as the copy from TBytes to the caller buffer is negligible.
Anyway, this made me improve in pascal primitive type understanding.

souchaud

Member

Registered: 2015-05-05

Posts: 6

Re: SynCrypto, EncryptPKCS7 and RawByteString

Thanks for your time.