Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to terminate Recv/Send methods if fSSL was destroyed #270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

evgeny-k
Copy link

I'm using TIdHTTP/SSL in background thread.
if I call Post method, I have AV after calling Disconnect in main thread because TIdSSLSocket is destroyed but TIdSSLSocket.Send is still executed ....

call-stack (main thread) when TIdSSLSocket is freed:

IdSSLOpenSSL.TIdSSLSocket.Destroy
System.TObject.Free
IdSSLOpenSSL.TIdSSLIOHandlerSocketOpenSSL.Close
IdTCPConnection.TIdTCPConnection.Disconnect(???)
IdTCPConnection.TIdTCPConnection.Disconnect

AV in TIdSSLSocket.Send because fSSL is nil:

function TIdSSLSocket.Send(const ABuffer: TIdBytes; AOffset, ALength: Integer): Integer;
var
  ret, err: Integer;
begin
  Result := 0;
  repeat
    ret := SSL_write(fSSL, @ABuffer[AOffset], ALength); //<<<< AV

call-stack for this is

IdSSLOpenSSL.TIdSSLSocket.Send(...)
IdSSLOpenSSL.TIdSSLIOHandlerSocketOpenSSL.SendEnc(???,???,224)
IdSSL.TIdSSLIOHandlerSocketBase.WriteDataToTarget(???,???,224)
IdIOHandler.TIdIOHandler.WriteDirect(...)
IdIOHandler.TIdIOHandler.WriteBufferFlush(???)
IdIOHandler.TIdIOHandler.WriteBufferFlush
IdIOHandler.TIdIOHandler.WriteBufferClose
IdHTTP.TIdHTTPProtocol.BuildAndSendRequest(???)
IdHTTP.TIdCustomHTTP.ConnectToHost($7EA93560,???)
IdHTTP.TIdCustomHTTP.DoRequest(???,'https://.....',$7E91CBF0,$7E9B7AA0,(...))
IdHTTP.TIdCustomHTTP.Post('https://......',$8663B0,$7E9B7AA0)

if we add checking for nil for fSSL we can avoid AV

@rlebeau
Copy link
Member

rlebeau commented Nov 21, 2019

Checking for nil is not good enough, all it does is introduce a new race condition and doesn't address the root issue of the TIdSSLSocket object being freed while it is still actively being used. This same issue also affects TIdTCPServer, see #218. In that ticket, just closing the underlying socket without destroying the SSL object worked. A similar solution would likely apply here, too.

@evgeny-k
Copy link
Author

it doesn't work for me.
I'm using client-side component (TIdHTTP).
in my case IdHTTP.Socket.Binding is nil so I can't call IdHTTP.Socket.Binding.CloseSocket()
If I call IdHTTP.Socket.Close; - it calls

procedure TIdSSLIOHandlerSocketOpenSSL.Close;
begin
  FreeAndNil(fSSLSocket);
  if not IsPeer then begin
    FreeAndNil(fSSLContext);
  end;
  inherited Close;
end;

so fSSLSocket is destroyed and AV is raised again.

We can add some variable like fTerminated that would be initialized in destructor and check for it in Recv/Send methods. at least it will allow to terminate SSL methods ...

@rlebeau
Copy link
Member

rlebeau commented Nov 26, 2019

in my case IdHTTP.Socket.Binding is nil so I can't call IdHTTP.Socket.Binding.CloseSocket()

If the Binding is nil then there is no socket allocated at all (either the IOHandler was never opened, or it has already been destroyed), so there should be no SSL object allocated, either.

If I call IdHTTP.Socket.Close; - it calls

procedure TIdSSLIOHandlerSocketOpenSSL.Close;
begin
  FreeAndNil(fSSLSocket);
  if not IsPeer then begin
    FreeAndNil(fSSLContext);
  end;
  inherited Close;
end;

Yes, I'm aware of that. Close() freeing the fSSLSocket object is the root of the other ticket I linked to earlier. Same issue here. So my suggestion is to NOT free that object anymore in Close(), just shutdown+close (not deallocate) the underlying socket instead. Free the fSSLSocket in the destructor instead.

We can add some variable like fTerminated that would be initialized in destructor and check for it in Recv/Send methods. at least it will allow to terminate SSL methods ...

That would not work. If the Recv/Send methods are called on an invalid object, there would be no valid fTerminated member available to access. Undefined behavior.

@Fulgan
Copy link
Member

Fulgan commented Dec 5, 2019

I agree that an AV creates an unnecessary security risk in the library. However, I also agree that this AV is a consequence of anyother problem in the software itself.
Therefore, I suggest this pull request be changed to raise an exception if the fSSL reference is nil. That will prevent the AV without supressing the error.

@rlebeau
Copy link
Member

rlebeau commented Dec 6, 2019

There is no security risk in an AV being raised. And there is really no difference in raising one type of exception versus another. I don't think checking for a nil SSL object is the right solution in this situation. It doesn't actually solve anything.

@rlebeau rlebeau added Element: I/O Handlers Issues related to TIdIOHandler and descendants Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants labels May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Element: I/O Handlers Issues related to TIdIOHandler and descendants Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants