Ralf Junker

The function declarations for f_EVP_PKEY_get_raw_private_key() and f_EVP_PKEY_get_raw_public_key() each have a wrong parameter type: outlen: Integer. The correct type is outlen: size_t. On 64-bit, this can lead to memory overwrite if the 64-bit size_t is written to the 32-bit Integer. Reference: https://www.openssl.org/docs/man1.1.1/man3/EVP_PKEY_get_raw_private_key.html Ralf

The patch below suggests fixes for 7 memory leaks in OverbyteIcsWSocket.pas. They surfaced after the OverbyteIcsSslWebServ.dpr sample project was linked against YuOpenSSL. A total of > 10k memory allocations reported not freed, adding up to > 500k bytes of leaked memory. The leaks were easy to spot because YuOpenSSL does not use the OpenSSL DLLs but compiles all OpenSSL code into the application binary. As a side effect, YuOpenSSL uses the Delphi memory manager and by default allows memory trackers to see OpenSSL memory allocations. Otherwise this does normally not happen when the OpenSSL DLLs employ their own memory management. The leaks then sum up in the DLLs' memory space, and out of sight of Delphi's memory trackers. For anyone who would like to test their ICS applications for memory leaks with YuOpenSSL, here is the link: https://www.yunqa.de/delphi/products/openssl/index Index: OverbyteIcsWSocket.pas =================================================================== --- OverbyteIcsWSocket.pas (revision 1492) +++ OverbyteIcsWSocket.pas (working copy) @@ -13933,7 +13933,8 @@ for I := 0 to Result - 1 do begin P := f_OPENSSL_sk_value(CertStack, I); if Assigned(P) then { V8.64 sanity test } - Add(f_X509_dup(PX509(P))); + // Add(f_X509_dup(PX509(P))); // ralfjunker fix memory leak. + Add(PX509(P)); // ralfjunker fix memory leak. end; end; @@ -14866,7 +14867,8 @@ OneErr := String(LastOpenSslErrMsg(FALSE)); OneErr := 'Error Cert ' + IntToStr(tot) + ' - ' + OneErr; end; - end; + // end; ralfjunker fix memory leak. + end else f_BIO_free(MemBio); // ralfjunker fix memory leak. // ignore private keys and other stuff if OneErr <> '' then begin @@ -17369,7 +17371,8 @@ procedure TX509Base.FreeAndNilX509Inters; { V8.41 } begin if Assigned(FX509Inters) then begin - f_OPENSSL_sk_free(FX509Inters); + // f_OPENSSL_sk_free(FX509Inters); // ralfjunker fix memory leak. + f_OPENSSL_sk_pop_free(FX509Inters, @f_X509_free); // ralfjunker fix memory leak. FX509Inters := nil; end; end; @@ -18334,7 +18337,10 @@ // pending, search stack for server certificate, might not be first { first in stack is server certificate } - SetX509(PX509(f_OPENSSL_sk_delete(MyStack, 0))); + // SetX509(PX509(f_OPENSSL_sk_delete(MyStack, 0))); // ralfjunker fix memory leak. + x := PX509(f_OPENSSL_sk_delete(MyStack, 0)); // ralfjunker fix memory leak. + SetX509(x); // ralfjunker fix memory leak. + f_X509_free(x); // ralfjunker fix memory leak. { remaineder are intermediates } if (TotInfo > 1) and (IncludeInters > croNo) then begin @@ -19394,7 +19400,8 @@ begin result := '' ; if not Assigned(X509) then Exit; - pubkey := f_X509_get_pubkey(X509); + // pubkey := f_X509_get_pubkey(X509); // ralfjunker fix memory leak. + pubkey := f_X509_get0_pubkey(X509); // ralfjunker fix memory leak. Result := GetKeyDesc(pubkey); end; @@ -19500,17 +19507,27 @@ {* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *} procedure TX509Base.LoadIntersFromPemFile(const FileName: String); { V8.41 } +var // ralfjunker fix memory leak. + x: PStack; // ralfjunker fix memory leak. begin InitializeSsl; - SetX509Inters(IcsSslLoadStackFromInfoFile(FileName, emCert)); + // SetX509Inters(IcsSslLoadStackFromInfoFile(FileName, emCert)); // ralfjunker fix memory leak. + x := IcsSslLoadStackFromInfoFile(FileName, emCert); // ralfjunker fix memory leak. + SetX509Inters(x); // ralfjunker fix memory leak. + f_OPENSSL_sk_free(x); // ralfjunker fix memory leak. end; {* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *} procedure TX509Base.LoadIntersFromString(const Value: String); { V8.41 } +var // ralfjunker fix memory leak. + x: PStack; // ralfjunker fix memory leak. begin InitializeSsl; - SetX509Inters(IcsSslLoadStackFromInfoString(Value, emCert)); + // SetX509Inters(IcsSslLoadStackFromInfoString(Value, emCert)); // ralfjunker fix memory leak. + x := IcsSslLoadStackFromInfoString(Value, emCert); // ralfjunker fix memory leak. + SetX509Inters(x); // ralfjunker fix memory leak. + f_OPENSSL_sk_free(x); // ralfjunker fix memory leak. end;

https://sourceforge.net/p/gexperts/bugs/208/

Sometimes, the CodeFormatter removes paragraph indentation from within a comment if the IndentComments option is enabled. The test case below demonstrates the problem. The formatter removes indentation from the 2nd paragraph. I can reproduce this with the formatter dunit tests and the FormatterSettings-Borland.ini setting, which has IndentComments enabled. unit testfile_CommentIndent; interface { 1st paragraph. 2nd paragraph should stay indented. } procedure p1; implementation end. IMO, the formatter should not change the comment, and older GExperts did not do so. However, recent GExperts compiled from SVN show the new behavior. It might have been introduced in SVN 3083, which fixed wrong comment indentation before procedures. I welcome the fix in that it no longer adds indentation to entire comments before procedures, but I believe it should leave indentation within the comment itself unchanged.

Agreed. But maybe they are used or will be used somewhere else?

EVP_PKEY_paramgen() pkey should be a double pointer parameter. OverbyteIcsLIBEAY.pas line 1752, SVN 2063: f_EVP_PKEY_paramgen: function(pctx: PEVP_PKEY_CTX; pkey: PEVP_PKEY): Integer; cdecl = Nil; Notice the two ** asterisks in the OpenSSL's evp.h C declaration: int EVP_PKEY_paramgen(EVP_PKEY_CTX *ctx, EVP_PKEY **ppkey); Possible fixes: { More C like: Better if ppkey may be nil. } f_EVP_PKEY_paramgen: function(pctx: PEVP_PKEY_CTX; ppkey: PPEVP_PKEY): Integer; cdecl = Nil; { More Pascal like. Notice that nil cannot be passed this way. } f_EVP_PKEY_paramgen: function(pctx: PEVP_PKEY_CTX; var pkey: PEVP_PKEY): Integer; cdecl = Nil; Reference: https://www.openssl.org/docs/man1.1.1/man3/EVP_PKEY_paramgen.html Ralf

f_CRYPTO_get_ex_data should return Pointer, not Integer. The code in question is at OverbyteIcsLIBEAY.pas line 1847, SVN 1476: f_CRYPTO_get_ex_data: function(r: PCRYPTO_EX_DATA; idx: integer): integer; cdecl = Nil; The corresponding C declaration from OpenSSL's crypto.h: void *CRYPTO_get_ex_data(const CRYPTO_EX_DATA *ad, int idx); Suggested solution: f_CRYPTO_get_ex_data: function(r: PCRYPTO_EX_DATA; idx: integer): Pointer; cdecl = Nil; Reference: https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_data.html Ralf

At the time of this writing, TCryptoExNewFunc is declared as a function while it should be a procedure. OverbyteIcsLIBEAY.pas line 1752, SVN 1476: TCryptoExNewFunc = function(parent: Pointer; ptr: Pointer; ad: PCRYPTO_EX_DATA; idx: integer; argl: DWord; argp: Pointer): Integer; cdecl; This is the corresponding C declaration from OpenSSL's crypto.h, documented here: https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_EX_new.html typedef void CRYPTO_EX_new (void *parent, void *ptr, CRYPTO_EX_DATA *ad, int idx, long argl, void *argp); This is the Pascal translation matching the C declaration: TCryptoExNewFunc = procedure(parent: Pointer; ptr: Pointer; ad: PCRYPTO_EX_DATA; idx: integer; argl: DWord; argp: Pointer); cdecl; Ralf

In OverbyteIcsWSocket.pas, PunyServerName is always filled with 8-bit data, even if it is a 16-bit UnicodeString on Unicode Delphis. This happens in OverbyteIcsWSocket.pas line 15916 (SVN 1469): Move(DataExt[5], Ws.FCliHelloData.PunyServerName[1], Slen); The line above works well with non Unicode Delphis, but copies incorrect data in Unicode enabled Delphis. Cause is the declaration of PunyServerName (line 3488): TClientHelloData = record ServerName: String; PunyServerName: String; // <-- Problem here. ... Later on, this results in incorrect conversion to ServerName. See that the OverbyteIcsSslWebServ demo outputs garbled SNI and Server Name instead of localhost: HTTPS Server is waiting for connections https://localhost/demo.html HTTP Server is waiting for connections http://localhost/demo.html [10:17:36 127.0.0.1] SNI "潬慣桬獯t Client Hello: Server Name: 潬慣桬獯t [10:18:15 127.0.0.1] SNI "潬慣桬獯t Client Hello: Server Name: 潬慣桬獯t A quick remedy is to declare PunyServerName as AnsiString and work around the warning "OverbyteIcsWSocket.pas(15968): W1057 Implicit string cast from 'AnsiString' to 'string'", possibly be adding a typecast. TClientHelloData = record ServerName: String; PunyServerName: AnsiString; // <-- 8-bit string, even in Unicode Delphis. ... Ralf

OverbyteIcsLIBEAY.pas line 1994 (SVN 1469) declares f_EVP_DigestSignInit() like this: f_EVP_DigestSignInit: function(ctx: PEVP_MD_CTX; pctx: PEVP_PKEY_CTX; etype: PEVP_MD; impl: PEngine; pkey: PEVP_PKEY): Integer; cdecl = Nil; In Pascal, pctx is a single pointer, but C declares pctx as a double pointer: int EVP_DigestSignInit(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx, const EVP_MD *type, ENGINE *e, EVP_PKEY *pkey); Following nearby declarations, Pascal should probably change pctx to a var parameter. Reference: https://www.openssl.org/docs/man1.1.1/man3/EVP_DigestSignInit.html Ralf

Thanks for the explanation. Removing outdated files seems reasonable as they likely confuse first time ICS users looking at the samples for good inspiration but end up confused. Until removal, might I suggest just to delete the offending line? It has no functional value anyway, its sole purpose is informational. This it would allow users of older Delphis to compile the OverbyteIcsHttpsServer1 sample without confusion. Ralf

OverbyteIcsSSLEAY.pas line 2160 (SVN 1464) declares f_SSL_set_msg_callback() to use a TProto_msg_cb callback. TProto_msg_cb is defined in line 1270 as a function: TProto_msg_cb = function (write_p, version, content_type: integer; buf: PAnsiChar; size_t: integer; ssl: PSSL; arg: Pointer): Integer; cdecl; { V8.40 handshake protocol message callback } This is different from the OpenSSL definition. The C header declares the callback as a procedure which does not return a value (void): void SSL_CTX_set_msg_callback(SSL_CTX *ctx, void (*cb)(int write_p, int version, int content_type, const void *buf, size_t len, SSL *ssl, void *arg)); Reference: https://www.openssl.org/docs/man1.1.1/man3/SSL_set_msg_callback.html Ralf

OverbyteIcsSSLEAY.pas line 2097 (SVN 1464) declares f_SSL_clear() like this: f_SSL_clear: procedure(S: PSSL); cdecl = nil; This is different from the OpenSSL definition. The Pascal declaration is a procedure, but the C declaration a function with an Integer return value: int SSL_clear(SSL *ssl); Reference: https://www.openssl.org/docs/man1.1.1/man3/SSL_clear.html Ralf

OverbyteIcsSSLEAY.pas line 2095 (SVN 1464) declares f_SSL_bytes_to_cipher_list() like this: f_SSL_bytes_to_cipher_list: function(s: PSSL; cbytes: PAnsiChar; len: size_t; isv2format: Boolean; sk, scvsvs: PSTACK_OF_SSL_CIPHER): LongInt; cdecl = nil; { V8.64 } This is different from the OpenSSL definition. The Pascal declaration passes sk and scvsvs as simple pointers, instead of pointer addresses or var parameters. In contrast, the C header uses a double pointer for both parameters: int SSL_bytes_to_cipher_list(SSL *s, const unsigned char *bytes, size_t len, int isv2format, STACK_OF(SSL_CIPHER) **sk, STACK_OF(SSL_CIPHER) **scsvs); Reference: https://www.openssl.org/docs/man1.1.1/man3/SSL_bytes_to_cipher_list.html Ralf

Samples\Delphi\SslInternet\OverbyteIcsHttpsServer1.pas line 415 has a reference to OverbyteIcsWSocket.SslWSocketCopyRight which does not exist: Display(' ' + Trim(OverbyteIcsWSocket.SslWSocketCopyRight)); As a result, the OverbyteIcsHttpsServer1.pas unit does not compile. Ralf