[BUG] HTTPS support in 64bits - Access Violation

[Marcelo Jaloto 1]

Can someone help me?

 

We are having a bug (access violation) with SSL (HTTPS support) only in 64bits. 

 

My current Overbyte ICS version is 8.63.

 

The bug is attached image.

 

 

{* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *}
{ V8.57 convert wire-format concactanted length prefixed strings to TStrings }
function IcsWireFmtToStrList(Buffer: TBytes; Len: Integer; SList: TStrings): Integer;
var
    offset, mylen: integer;
    AStr: AnsiString;
begin
    Result := 0;
    if NOT Assigned(SList) then Exit;
    SList.Clear;
    offset := 0;
    while offset < Len do begin
        mylen := Buffer[offset];
        if mylen = 0 then Exit;  // illegal
        offset := offset + 1;
        SetLength(AStr, mylen);
        Move(Buffer[offset], AStr[1], mylen);
        SList.Add(String(AStr));
        offset := offset + mylen;
    end;
    Result := Slist.Count;
end;

 

 

Edited December 17, 2019 by Marcelo Jaloto

[Angus Robertson 577]

Don 't do much testing with Win64, unusual to find problems, particularly with a simple loop.  But that function is only used in one place in ICS, to decode the ALPN response during an SSL handshake, so the buffer is within the OpenSSL DLL, maybe there is an issue with the buffer alignment or something?  No ICS applications currently use the ALPN response, so I'll hide the exception as a quick solution. 

 

Is this problem with all SSL web sites or just one in particular, which is perhaps returning invalid data in the SSL handshake?  A typical ALPN response is just 'http/1.1, h2' so say that HTTP/2 is supported. 

 

Angus

 

[Guest]

Looking at IcsWireFmtToStrList and i see no check for length overflow, so may be this will solve this bug.

 

function IcsWireFmtToStrList(Buffer: TBytes; Len: Integer; SList: TStrings): Integer;
...
        //if mylen = 0 then Exit;  // illegal
        if (mylen = 0) or (mylen + offset >= Len) then Exit;  // illegal

 

[Guest]

There also length check missed in IcsStrListToWireFmt as the ProtocolName should be limited to 2^8-1 = 255, so

 

function IcsStrListToWireFmt(SList: TStrings; var Buffer: TBytes): Integer;
..
        //if mylen > 0 then begin		
        if (mylen > 0) or (mylen <= 255) then begin   // legal length

 

 

[Angus Robertson 577]

In which component are you seeing the access violation, IcsStrListToWireFmt should only be called in SSL servers. 

 

Angus

 

[Marcelo Jaloto 1]

Thanks for the replies guys.

 

This code is not my own, but I am trying to fix the bug that is occurring. 

 

Regarding ALPN there is the code below which may be influencing perhaps. Is this code below necessary? Is it ok?

procedure THttpServer.OnSslAlpnSelect(Sender: TObject; ProtoList: TStrings;
  var SelProto: string; var ErrCode: TTlsExtError);
var
  vCount: Integer;
begin
  if ProtoList.Count = 0 then
    Exit;

  for vCount := 0 to ProtoList.Count - 1 do
  begin
    if ProtoList[vCount] = ALPN_ID_HTTP11 then
    begin
      SelProto := ALPN_ID_HTTP11;
      ErrCode := teeOk;
      Exit;
    end;
  end;
end;

// ALPN_ID_HTTP11        = 'http/1.1';

 

The parameter ProtoList.Text has the content below:

h2
http/1.1

 

Compiling in 32bits works normally. We are enabling SSL (https) support for our product and are reporting it with 64bits.

 

Is there a 64bit problem in ICS overbyte related to the IcsWireFmtToStrList method and the ALPN event.

 

 

[Angus Robertson 577]

I've updated the ICS OverbyteIcsSslMultiWebServ sample with the code you show (which comes from the older web server sample), built and tested it for Win64, and it's working fine, no exceptions with ALPN which is being reported correctly, using OpenSSL 1.1.1d Win64. 

 

The code you show for the onSslAlpnSelect is correct usage, but not really needed since applications should default to HTTP/1.1 anyway unless told to use HTTP/2.  I did fix a memory bug in V8.62 relating to this which I guess could have come back or not been fixed properly.  I would just comment out the loop so the event returns without changing ErrCode and see if your problem goes away. 

 

I have updated the wsocket ALPN code to suppress any exceptions processing ALPN since it's not really fatal and made IcsWireFmtToStrList check for bad packet formatting. 

 

More importantly, during testing I found a unicode bug in IcsStrListToWireFmt which is used in HTTPS clients sending the ALPN list which sent a corrupted packet, now fixed, but you are not using that since we do not send the h2 protocol. 

 

I'll put the source changes in SVN once my own public web server has been updated and been used for at least 24 hours.

 

Angus

 

[Guest]

@Marcelo Jaloto OnSslAlpnSelect doesn't matter here as the exception in IcsWireFmtToStrList, but can you capture the content of Buffer in IcsWireFmtToStrList, as the content should be :

Quote

00 0C 02 68 32 08 68 74 74 70 2F 31 2E 31

 

[Marcelo Jaloto 1]

This code did not resolve.

function IcsStrListToWireFmt(SList: TStrings; var Buffer: TBytes): Integer;
..
        //if mylen > 0 then begin		
        if (mylen > 0) or (mylen <= 255) then begin   // legal length

 

This code did not resolve.

function IcsWireFmtToStrList(Buffer: TBytes; Len: Integer; SList: TStrings): Integer;
...
        //if mylen = 0 then Exit;  // illegal
        if (mylen = 0) or (mylen + offset >= Len) then Exit;  // illegal

 

workaround:

I commented on some unimportant things for the moment and it worked like this according to the code below.

 

 

 

The problem is in this call using 64bits. Within the IcsWireFmtToStrList Method.

Count := IcsWireFmtToStrList(TBytes(input), inlen, ProtoList);

 

Edited December 18, 2019 by Marcelo Jaloto

[Guest]

@Marcelo Jaloto Good , you have found a solution, but can you please capture and share with us the content of Buffer and Len in IcsWireFmtToStrList ?

 

I am very interested now and want to know if there is a problem with OpenSSL callback function parameters.

[Angus Robertson 577]

Don't believe there a problem in IcsWireFmtToStrList, more likely to be setting the output pointer for the AlpnSelectCallBack function, which means OpenSSL reads a Delphi variable.  Originally it was a local variable but it had gone out of scope when OpenSSL tried to read it. so V8.62 changed it to a TWSocket variable FAlpnProtoAnsi and that worked for Win32, and for Win64 according to my testing here.  But perhaps there is something different about your server implementation to the ICS samples, 

 

As I said before, simply not using the onSslAlpnSelect should have fixed the issue.

 

Angus

 

[Guest]

I might be wrong, but here is my concern about IcsWireFmtToStrList :

From the documentation of SSL_CTX_set_alpn_select_cb https://www.openssl.org/docs/man1.0.2/man3/SSL_select_next_proto.html

Quote

cb is the application defined callback. The in, inlen parameters are a vector in protocol-list format. The value of the out, outlen vector should be set to the value of a single protocol selected from the in, inlen vector. The arg parameter is the pointer set via SSL_CTX_set_alpn_select_cb().

and from the ALPN RFC https://tools.ietf.org/html/rfc7301#section-3.1

Quote

   struct {
       ProtocolName protocol_name_list<2..2^16-1>
   } ProtocolNameList;

   "ProtocolNameList" contains the list of protocols advertised by the
   client, in descending order of preference.  Protocols are named by
   IANA-registered, opaque, non-empty byte strings, as described further
   in Section 6 ("IANA Considerations") of this document.  Empty strings
   MUST NOT be included and byte strings MUST NOT be truncated.

So what does vector means ? , for me as mathematician vector always need a length and the documentation of SSL_CTX_set_alpn_select_cb  does not show if in and inlen include the the length of that vector or not ( the first two bytes), so if it does then the length should be included and the first 2 bytes should be handled differently as one integer ( the length) this might be working as the first will mostly be 0 and the second will have the full length of the vector ( list) so there will be one protocol name included , and that protocol name will have the chars like #8 in side for the second real protocol if it is there while the first will have may be #2 as in the example of the content i included of those h2 and http/1.1

00 0C 02 68 32 08 68 74 74 70 2F 31 2E 31

 

this might be the problem, and i might be wrong as i didn't run the code and don't know what does SSL_CTX_set_alpn_select_cb parameters in real life.

[Angus Robertson 577]

I know the function works because it unpacks and logs real ALPN data, but for completeness I've added logging of the ALPN wire packet:

 

AlpnCB> inlen: 12 - 02683208687474702F312E31
AlpnCB> Protocols: h2,http/1.1

 

I also know the ALPN data may be incorrectly formed, OpenSSL simply passes whatever is received in the TLS initial packet, because until I fixed the bug yesterday, Delphi unicode compilers where sending 086008700470047 for http/1.1 if ALPN was specified in SslContext (not the default). This was correctly ignored by the ICS web server, but I noticed it because one of my web pages reports the client ALPN and was truncated by the first null.  I would eventually have found this when testing the Let's Encrypt TLS challenge which does not work yet due to another OpenSSL callback bug.

 

Angus

 

[Marcelo Jaloto 1]

Hi Angus Sorry for my absence in the answers for a few days. I returned to this case to update Overbyte ICS with its fixes.

The link below does not contain your modifications.
http://wiki.overbyte.eu/wiki/index.php/ICS_Download

Before reporting the issue I had already updated to November 8.63, 2019.

Where are the new updates with the fix for this case?

Could you pass the correct link? Or update the link above?

Thank you!

[Angus Robertson 577]

The ALPN changes are in V8.64 which is not released yet, but the changes can be downloaded from the SVN overnight zip on the page you mention. 

 

However I did not fix any bugs in the server version, just improved the code as previously mentioned. 

 

Angus

 

[Marcelo Jaloto 1]

Hello
I discovered the bug and the solution.

A pointer is 4 bytes in 32 bits and 8 bytes in 64 bits.

 

See more at :
http://docwiki.embarcadero.com/RADStudio/Rio/en/Converting_32-bit_Delphi_Applications_to_64-bit_Windows

 

The IcsWireFmtToStrList method of the OverbyteIcsUtils unit had a problem moving characters with Move without regard to the number of bytes.

 

Wrong:

Move (Buffer [offset], AStr [1], mylen);

 

Correct:

Move (Buffer [offset], AStr [1], mylen * SizeOf (TBytes));

 

The Len parameter comes with the value 12.

 

The first 12 characters in ordinal form in the Buffer parameter byte array are:

(2) 104 50 (8) 104 116 116 112 47 49 46 49

This is equivalent to "2h28http/1.1"

The numbers in parentheses delimit the end of the copy.

 

See the code running and running at 32 and 64 bits.

 

 

 

@Angus Robertson

Can you add this correction / contribution to your source code?

 

Many thanks to all who participated in this case.

Edited January 6, 2020 by Marcelo Jaloto

[Guest]

2 hours ago, Marcelo Jaloto said:

Move (Buffer [offset], AStr [1], mylen * SizeOf (TBytes));

That is wrong.

SizeOf(TBytes) is 4 or 8 , so it will even generate your own example

2 hours ago, Marcelo Jaloto said:

(2) 104 50 (8) 104 116 116 112 47 49 46 49

This is equivalent to "2h28http/1.1"

 

[Chris Rolliston 1]

13 hours ago, Marcelo Jaloto said:

Hello
I discovered the bug and the solution.

A pointer is 4 bytes in 32 bits and 8 bytes in 64 bits.

 

See more at :
http://docwiki.embarcadero.com/RADStudio/Rio/en/Converting_32-bit_Delphi_Applications_to_64-bit_Windows

 

The IcsWireFmtToStrList method of the OverbyteIcsUtils unit had a problem moving characters with Move without regard to the number of bytes.

 

Wrong:

Move (Buffer [offset], AStr [1], mylen);

 

Correct:

Move (Buffer [offset], AStr [1], mylen * SizeOf (TBytes));

 

The Len parameter comes with the value 12.

This correction cannot be correct. TBytes is a reference type (i.e. thinly-veiled pointer) to an array of byte values; Move copies actual data (i.e. the stuff that a reference type references), against units of the actual data. In addition, this code uses Move to copy into an AnsiString, which is also a reference type to an array of byte-sized elements. Ergo, an element size of 1 is correct, being what both SizeOf(Byte) (i.e. the element type for a TBytes) and SizeOf(AnsiChar) (the element type for an AnsiString) both are.

[Angus Robertson 577]

15 hours ago, Marcelo Jaloto said:

The IcsWireFmtToStrList method of the OverbyteIcsUtils unit had a problem moving characters with Move without regard to the number of bytes.

Sorry, but you misunderstand the declaration for TBytes, it is a pointer to a dynamic array of bytes, not a pointer to a array of pointers.  A byte remains the same size however compiled, so your proposed suggestion of copying the content based on the pointer size will cause many problems. 

 

I can only assume your correction was theoretical and you did not test it with an ICS SSL server application that uses the code. 

 

I've just built the OverbyteIcsSslWebServ sample with Win64, and ALPN is working as expected, as I showed earlier in this thread,   I am getting some other strange exceptions running Win64 samples today, so there may be other Win64 issues elsewhere, or with OpenSSL Win64, but not with ALPN.   Don't have time to investigate further at the moment.

 

Angus

[Marcelo Jaloto 1]

The Buffer parameter of type TBytes gets a pointer to a byte array and it must be understood that at 32 bits a pointer is 4 bytes and at 64 bits it is 8 bytes. When you use the Move function you are making a direct copy of memory that needs to take into account how many bytes each position has based on the number of bits. Maybe try another solution that doesn't use the Move function. All places that have the Move function need to be re-validated. And also all places where some kind of pointer calculation is being worked on because for 64bit it needs to be re-validated as Embarcadero recommends.

 

Please read the article below.
http://docwiki.embarcadero.com/RADStudio/Rio/en/Converting_32-bit_Delphi_Applications_to_64-bit_Windows

 

@Kas Ob.

Exactly. With 32bits must be considered 4 bytes and with 64 bits must be considered 8 bytes.

Edited January 7, 2020 by Marcelo Jaloto

[Marcelo Jaloto 1]

Now that it is known where the error is and the parameter values as infomated in my post just isolate the IcsWireFmtToStrList function and do all the necessary 32 and 64 bit tests. In all my tests the problem of Access Violation has been resolved.

 

Another suggestion is to make other code that does not use pointers that results in the same value to work in 32 and 64 bits. However if you want to continue using the Move function you need the number of bytes a pointer occupies in memory.
This was suggested by Embarcadero as a solution proposal.

[Angus Robertson 577]

I quite understand that pointers differ in Win64, but they that code is not copying pointers, it is copying 8-bit bytes. 

 

Will you please build the OverbyteIcsSslWebServ sample with ICS 8.64 Win64 and your changes, make an request to https://localhost/demo.html with Display SSL Info and Logger Dest Event ticked, the log should contain lines with AlpnCB similar to:

 

[16:22:53 127.0.0.1] SNI "localhost" received
16:22:53:024 AlpnCB> inlen: 12 - 02683208687474702F312E31
16:22:53:031 AlpnCB> Protocols: h2,http/1.1
[16:22:53] SSL Application Layer Protocols allowed from client: h2,http/1.1
16:22:53:044 0000000002710020 ICB> SSL_accept: SSLv3/TLS read client hello
 

where those two lines are the input and output from IcsWireFmtToStrList.  I'm not going to look at this further with evidence of a real problem in ICS.  Or you could add IcsLogger to your own server to get the same logging information. 

 

Angus

 

 

[Marcelo Jaloto 1]

@Angus Robertson
I will follow all your recommendations and then post the test results.

[Guest]

33 minutes ago, Marcelo Jaloto said:

Please read the article below.
http://docwiki.embarcadero.com/RADStudio/Rio/en/Converting_32-bit_Delphi_Applications_to_64-bit_Windows

The FSelection in that example is an array of Pointer, that what caused the confusion for you, and in Delphi TBytes is an array of Byte, String is an array of Char while ansistring is an array of AnsiChar and so on .

When you see the inclusion of SizeOf(Pointer) it due that fact the items needed to copied are pointers not bytes, likewise when you use Move with strings then you should use the sizeof bytes to be copied which is equal to Length(string)*SizeOf(Char), 

I hope this clear things for you, and suggest to read more about Pointers and Arrays and how to use Move the right way, and what operation are affected by bit-ness (32bit and 64bit )

[Marcelo Jaloto 1]

@Kas Ob.

I am not confused. You may not understand my explanation or the component code.

 

 

 

This solution is running on 32bit and 64bit returning the same result. And the Access Violation error does not occur.

 

@Kas Ob.

What would be your solution then this case?

 

Edited January 7, 2020 by Marcelo Jaloto