Skip to content

Comments

Shutdown ssl connection with client if radius request packet validation fails#122

Merged
fmauchle merged 1 commit intoradsecproxy:masterfrom
hjain-arista:tls-ssl-shutdown
Apr 28, 2023
Merged

Shutdown ssl connection with client if radius request packet validation fails#122
fmauchle merged 1 commit intoradsecproxy:masterfrom
hjain-arista:tls-ssl-shutdown

Conversation

@hjain-arista
Copy link
Contributor

@hjain-arista hjain-arista commented Apr 24, 2023

On upgrading radsecproxy from 1.9.1 to 1.9.2, we started seeing an issue where tls clients were reporting a socket timeout on sending a malformed radius packet (with wrong secret) instead of radsecproxy closing the SSL connection (which was the behaviour in 1.9.1). Debugging this, we found an issue in radsecproxy whereby some threads were not exiting (namely tlsserverwr) leading to this.

When radsrv returns 0 because of radius packet validation failure, tlsserverrd does not set any state to indicate to tlsserverwr thread that SSL connection with client is closed. It only calls pthread_cond_signal(&client->replyq->cond);. tlsserverwr thread on receiving the thread conditional signal moves ahead and checks whether SSL is shutdown using SSL_get_shutdown(client->ssl) but since this returns false, it again starts waiting for the thread conditional signal and therefore never exits. At this time, tlsserverrd is also waiting for the tlsserverwr thread to end. Below logs indicate this scenario

Mon Apr 24 00:57:16 2023: radtlsget: got 45 bytes                                                                                                                                          
Mon Apr 24 00:57:16 2023: tlsserverrd: got Radius message from 10.0.0.101                                                                                                                  
Mon Apr 24 00:57:16 2023: buf2radmsg: Accounting/CoA/Disconnect-Request message authentication failed                                                                                      
Mon Apr 24 00:57:16 2023: radsrv: ignoring request from 10.0.0.101 (10.0.0.101), validation failed.                                                                                        
Mon Apr 24 00:57:16 2023: freerq: called with refcount 1                                                                                                                                   
Mon Apr 24 00:57:16 2023: tlsserverrd: message authentication/validation failed, closing connection from 10.0.0.101                                                                        
Mon Apr 24 00:57:16 2023: tlsserverrd: waiting for writer to end                                                                                                                           
Mon Apr 24 00:57:16 2023: tlsserverwr: got signal                                                                                                                                          
Mon Apr 24 00:57:16 2023: tlsserverwr: waiting for signal

This fix is to shutdown the SSL connection with the client if radsrv returns 0 so that tlsserverwr thread can also exit successfully. This leads to tlsservernew thread exiting which frees the SSL connection with the client along with closing the socket. Below logs demonstrate the fix.

Mon Apr 24 03:03:58 2023: radtlsget: got 45 bytes                                                                                                                                          
Mon Apr 24 03:03:58 2023: tlsserverrd: got Radius message from 10.0.0.101                                                                                                                  
Mon Apr 24 03:03:58 2023: buf2radmsg: Accounting/CoA/Disconnect-Request message authentication failed                                                                                      
Mon Apr 24 03:03:58 2023: radsrv: ignoring request from 10.0.0.101 (10.0.0.101), validation failed.                                                                                        
Mon Apr 24 03:03:58 2023: freerq: called with refcount 1                                                                                                                                   
Mon Apr 24 03:03:58 2023: tlsserverrd: message authentication/validation failed, closing connection from 10.0.0.101                                                                        
Mon Apr 24 03:03:58 2023: tlsserverrd: calling SSL shutdown                                                                                                                                
Mon Apr 24 03:03:58 2023: tlsserverrd: waiting for writer to end                                                                                                                           
Mon Apr 24 03:03:58 2023: tlsserverwr: got signal                                                                                                                                          
Mon Apr 24 03:03:58 2023: tlsserverwr: ssl connection shutdown; exiting as requested                                                                                                       
Mon Apr 24 03:03:58 2023: tlsserverrd: reader for 10.0.0.101 exiting

@fmauchle fmauchle self-assigned this Apr 25, 2023
fmauchle added a commit that referenced this pull request Apr 28, 2023
@fmauchle
Merge pull request #122
64b4356
@fmauchle fmauchle merged commit 1e7ff25 into radsecproxy:master Apr 28, 2023
fmauchle added a commit that referenced this pull request May 2, 2023
@fmauchle
packport PR #122 to 1.9
910b077
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@fmauchle fmauchle

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hjain-arista @fmauchle