ICS SSL under Linux x64

[Angus Robertson 574]

In this function. ICS does not use PAnsiChar as a null terminated string, it passes the actual TBytes length to the OpenSSL API, since the hash or digest is binary data.

 

It would seem the old digest is being corrupted before verification. 

 

Angus

 

[ap2021 0]

Irrespective of that, even if all the right data is sent to OpenSSL's EVP_DigestVerify  function, unlike Windows, in Linux it fails - returns 0 and those messages...

 

[Angus Robertson 574]

So you are saying you've saved a TBytes digest created by IcsAsymSignDigestTB and used it with IcsAsymVerifyDigestTB without any string, encoding or base64 conversions, and it fails?

 

Specifically,as in the function doTestSignClick in the JoseTst sample which displays the digest in hex and base64?  

 

I will convert that sample to FMX, as I said earlier, but it is not a priority.

 

Angus

 

[Angus Robertson 574]

One other thing you could check before verify is the length of the digest.  It varies depending on your private key type and signing hash, but is generally the length of the key, so

an RSA 2048 SHA256 digest will be 256 bytes long, binary.  The JoseTst sample reports the hash length for all combinations of keys and hashes.

 

This could be a simple check for the digest not being corrupted at some point.  I might do some investigation to see if I can reliably add such a check to verify. 

 

Angus

 

 

[ap2021 0]

On 10/4/2024 at 11:38 PM, Angus Robertson said:

So you are saying you've saved a TBytes digest created by IcsAsymSignDigestTB and used it with IcsAsymVerifyDigestTB without any string, encoding or base64 conversions, and it fails?

Yes, pretty much. Except that I was not creating this JWT, it's someone else's. It does validate using the same code under Windows. - "used it with IcsAsymVerifyDigestTB without any string, encoding or base64 conversions" And yes, it's 256 bytes long, binary digest I'm passing in.

 

So far I triple-checked the OpenSSL function definition, data types/sizes, etc. Binary stream is the same under Windows, BTW, so I know it comes through exactly the same here.

 

I'm trying to build the latest SSL now, to see if that was just a 3.0 bug...

[ap2021 0]

And typically Linux: OpenSSL 3.3.2 breaks yum and has a new dependency on libcurl (which I cannot install because of yum issue)...

 

Had to rebuild SSL with MD, because of libldap.so, used by yum, so I can install libcurl.

 

So, I now have the latest OpenSSL 3.3.2 here, but still the same error:

 

error:02000077:rsa routines::wrong signature length

error:1C880004:Provider routines::RSA lib

 

Same, even if I eliminate any possibility of typecasting issues & even hard-code 256 size to avoid using Length(), as in:

        ret := EVP_DigestVerify(DigestCtx, @OldDigest[0], 256, PAnsiChar(Data), Length(Data));

 

Same works under Win32 & Win64...

Edited October 6 by ap2021

[ap2021 0]

Found the problem!

 

I had this function, used to build a key from E&N:

 

  function BNfromBase64(const _s : String) : PBIGNUM;
  var
    len : integer;
    decoded : ansistring;
  begin
    decoded := IcsBase64UrlDecode(_s);
    len := length(decoded);
    result := BN_bin2bn(@decoded[1], len, nil);
  end;

 

It was returning rubbish from IcsBase64UrlDecode under Linux, hence the length (which I had to calculate on a separate line to be able to see it in debugger) was fluctuating.

 

I have replaced IcsBase64UrlDecode with TNetEncoding.Base64.DecodeStringToBytes (also changing the type from ansistring to tbytes and the index in the final call to 0 from 1) and it's fixed 😉

 

So, ICS version of IcsBase64UrlDecode is not Linux-proof, you should take note.

 

From what I could see in the debugger, it was treating the string as 0-based and ansichar as a word, although that could be a Linux debugger's quirk.

[Angus Robertson 574]

You should have been using IcsBase64UrlDecodeA to return an AnsiString, or IcsBase64UrlDecodeTB for TBytes, to allow binary to be handled without loss.

 

No idea why PBIGNUM is here, not used for digests, only raw keys. 

 

The function you show seems to be copied from IcsJoseJWKGetPKey, but our version casts to AnsiString and uses IcsBase64UrlDecodeA.

 

Angus

 

[ap2021 0]

IcsBase64UrlDecodeTB  has the same problem, because the fault is in Base64Decode.

 

So, you should be able to reproduce it by just calling Base64Decode, try:

3I2c2shPwo4E7Y9Npyp7A5s4bkw9u3BV0sNsn4Ph2WAZFXduaTy8ZirXa8cQF2_Riwckxcqf2DYmdUKGDb1FCBESW34oV2V5PopqS2llIfwU6gwrbWycQuai7FUG-UZWfud64ZRqbk8oyyYD7gHfijUiZ-nOHd3ozzDS0NNnH6TKCHiwKu-ocqzT7pDUMr3ozu2Gv0Gb4txtQk4m2QbNn4uNmYrSjgKzNH3HYWQgBYAEEgCUkG-nH4Ui1IsBvPCEIig_UhHn-OtJ9ZHnGmQnRQo22VMRWa9WPXYBt4RCA3eljmDYXLfQNua8Z69e40cGoXrSza90KsuXqNLo-0xQ0w

 

Edited October 8 by ap2021

[Angus Robertson 574]

If you have found and fixed a problem in ICS, please let others have it. 

 

Angus

 

[ap2021 0]

Sure. I haven't done a lot of testing and do not know which exact line has caused it for me. Plus, the fact there are a number of IcsBase64UrlDecodeXXX functions there, which should have seemingly all worked, i.e.: it's somewhat confusing. But I do know that TNetEncoding.Base64URL.DecodeStringToBytes, which I am now using, is the solution.

 

You may not be able to use TNetEncoding.Base64URL.DecodeStringToBytes, though, because it may not exist in older versions of Delphi, just not exactly sure how far back you have to support it.

 

If I do do more testing and figure out a single-line fix for this in ICS, I will share it here.

[ap2021 0]

Using IcsBase64UrlDecode was my mistake, IcsBase64UrlDecodeA actually worked. With IcsBase64UrlDecode, it was relying on implicit conversions, I guess, and while it worked under Windows, it wasn't working under Linux. Similarly with TB versions, it had to be TBA.

Edited October 12 by ap2021

[Angus Robertson 574]

The main issue with the various Base64 functions is they are mostly used for ASCII functions in protocols with strings, and having only AnsiString versions means lots of string casts to avoid warnings. 

 

I've been reworking a lot of low level stuff, tried to remove the String versions, but got so many warnings from dozens of units, had to restore them.  

 

But I have improved the code comments to suggest which functions can be used with binary, ideally TB versions, and which for text only. 

 

Angus

 

[ap2021 0]

Another very annoying thing under Linux is that ansistring's look Chinese in the debugger:

There's no telling what the values are. And typecasting when inspecting values does not work, it would say "ansistring not defined", i.e.:

My next hurdle is that it would not load PEM cert: it just cryptically fails. If I were to add "string" vars here, do the assignments, re-run and check the values of strings, then it does show the values. There's of course nothing you can do about that, it's a Delphi bug. But I might write another similar function to use TBytes instead - that would at least show something...

Its not permissions, as I ran it as "root" and it was my file to start with.

 

But as it did the validation Ok now, I also know that all shared libraries are fine. Go figure ;-(

 

Ok, never mind, I did figure, no issues now, it loads and everything 😉

Edited October 12 by ap2021

[ap2021 0]

On 10/12/2024 at 10:47 AM, ap2021 said:

Using IcsBase64UrlDecode was my mistake, IcsBase64UrlDecodeA actually worked. With IcsBase64UrlDecode, it was relying on implicit conversions, I guess, and while it worked under Windows, it wasn't working under Linux. Similarly with TB versions, it had to be TBA.

Interestingly enough, this "fix" broke it for me under Windows. The fix for which, was to just use TB version. Ansistrings are unpredictable now, Delphi supports them reluctantly and there are probably unwarranted conversions going on under the hood, somewhere, using pansichar's.

[ap2021 0]

Angus,

 

Just wanted to add that fundamentally, sometimes you need to process actual text strings (ANSI or not) and sometimes binary data and it would be nice if this distinction was immediately obvious.

 

It's a universal semantical headache, and C++ has the same problem, because there are just so many data types available and they may all look the same and essentially do the same thing.

 

For example in OpenSSL, there are such function declarations:

 

int EVP_DigestVerifyInit_ex(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
                            const char *mdname, OSSL_LIB_CTX *libctx,
                            const char *props, EVP_PKEY *pkey,
                            const OSSL_PARAM params[]);

 

Here, mdname is semantically a string. So it can be / actually is 0-terminated and with no size needed separately, hence pansichar is the correct type we should use in translated Pascal definitions. Unless we know for a fact that the target function expects and can handle Unicode text in UTF8, then we must use putf8char datatype in the tarnslated definitions, or it would cause problems, unless additionally typecast.

 

But here:

 

int EVP_DigestSignUpdate(EVP_MD_CTX *ctx, const void *data, size_t dsize);

 

The data parameter is semantically binary data - they are trying to convery this by using the word "data", there's also a requirement to pass in the size separately and they are trying to use a different data type for it, not just "const char *" (which it could have been, interchangeably), but "const void *".

 

Sometimes, in exactly same way, they may use "const unsigned char *" type instead, but that's because they are just as confused about it as we are in Delphi:

 

__owur int EVP_DigestVerify(EVP_MD_CTX *ctx, const unsigned char *sigret,
                            size_t siglen, const unsigned char *tbs,
                            size_t tbslen);

 

In fact they may have interchangeably used "const char *" for the same data in odd places, because apart from the semantics, it would achieve the same result and if the original developer was tired or lazy, the type used may not reflect its semantical significance. Besides, C++ has no special data type for binary data, so this issue has no silver bullet solution in C++.

 

This confusion then spills over into all other languages and then you translate it in your code, without regard to semantics, as:

 

    EVP_DigestVerify :                       function(ctx: PEVP_MD_CTX; sigret: PAnsiChar; siglen: size_t; tbsret: PAnsiChar; tbslen: size_t): Integer; cdecl = Nil;  { V8.52, V8.64 corrected declaration }

 

using PAnsiChar type. This may have been acceptable in Delphi 1 days, but today we have a better data type we should use instead: TBytes. You do not want sematics to be lost in translation, although the original semantics may need to be deduced first, because in C++ it may not be very clear.

 

These functions should be declared in Delphi with TBytes type instead, i.e.:

 

    EVP_DigestVerify :                       function(ctx: PEVP_MD_CTX; sigret: TBytes; siglen: size_t; tbsret: TBytes; tbslen: size_t): Integer; cdecl = Nil;  { A future version, properly corrected declaration }

or

    EVP_DigestVerify :                       function(ctx: PEVP_MD_CTX; const sigret: TBytes; siglen: size_t; const tbsret: TBytes; tbslen: size_t): Integer; cdecl = Nil;  { A future version, properly corrected declaration }

 

Which is essentially the same for the operation of the function, but is semantically much clearer and 1) allows Delphi to enforce the type and avoid any internal conversions, 2) will directly accept TBytes parameters without resorting to very frequent PAnsiChar() typecasts and 3) will keep it much clearer to the user/coder.

 

For instance, this will allow you to avoid weird conversions like these:

 

function IcsAsymVerifyDigest(const Data, OldDigest: AnsiString; PublicKey: PEVP_PKEY; HashDigest: TEvpDigest = Digest_sha256): Boolean;
begin
   Result := IcsAsymVerifyDigestTB(IcsStringAToTBytes(Data), IcsStringAToTBytes(OldDigest), PublicKey, HashDigest);
end;

 

And will allow you to get rid of AnsiString and AnsiChar parameters, at least the majority of them.

 

With some thought before changes and some testing after, it would really clean up this mess. It cannot be blindly applied to any such translated datatype, each function will need to be looked at separately to figure out the underlying meaning of the type used. But again, perhaps it does not have to be applied to all declarations at once, you can start with a handful of these functions, either picking the ones that you use, or care about or those that may seem easier and then roll it out to others at will.

 

Also, for Delphi, PAnsiChar literally refers to "0-terminated strings", which causes it to drop parts of data after the first #0 it encounters, when it's doing internal conversions, those we do not see and do not expect, therefore introducing hard to troubleshoot bugs.

 

Does this make sense? Do you agree?

[Angus Robertson 574]

Sorry, that message is far too long for me to read and digest.   I have other priorities. 

 

ICS contains a lot of historic code written over 25 years, sometimes it's worth the effort to clean it up, sometimes too dangerous if it can be easily tested, sometimes not worth the effort. 

 

Angus

 

[ap2021 0]

Fair enough, still a shame to see a good advise go to waste...

[DelphiUdIT 176]

7 hours ago, ap2021 said:

Fair enough, still a shame to see a good advise go to waste...

You can fork the project and mantain your version, after that you may propose a merge. When they have time surely they will do that.

I maintain my version of Indy for example, 'cause their (... his ...) time and priority are not the same as mines.

Edited October 29 by DelphiUdIT