[SERVER-53064] Connections created using transient SSL params should not be stored in the global DBConnectionPool - security risk Created: 24/Nov/20  Updated: 29/Oct/23  Resolved: 21/Feb/21

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 5.0.0

Type: Task Priority: Major - P3
Reporter: Andrew Shuvalov (Inactive) Assignee: Andrew Shuvalov (Inactive)
Resolution: Fixed Votes: 0
Labels: pm-1791_milestone-P, pm-1791_non-cloud-blocking
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Fully Compatible
Sprint: Sharding 2021-01-25, Sharding 2021-02-22
Participants:

 Description   

The global DBConnectionPool keeps reusable connections. If a connection was created using transient SSL params, it should not be recycled into this pool, because it can be picked from some other code logic and that will be a security risk.



 Comments   
Comment by Githook User [ 20/Feb/21 ]

Author:

{'name': 'Andrew Shuvalov', 'email': 'andrew.shuvalov@mongodb.com', 'username': 'shuvalov-mdb'}

Message: SERVER-53064: connection created using transient SSL params cannot be recycled
Branch: master
https://github.com/mongodb/mongo/commit/5199abc5b9113e310a79d9ec29a5ac6b77ad5682

Comment by Andrew Shuvalov (Inactive) [ 20/Jan/21 ]

Hi spencer.jackson, just want to get your opinion for above. Please don't worry about the code, just the summary that there is a potential future danger as the code evolves, where putting some protective fences right now will require some more significant refactoring. I can either proceed with agreed priority or close this with "won't fix".

Comment by Andrew Shuvalov (Inactive) [ 20/Jan/21 ]
  1. I verified the cheahuychou.mao assessment above that the recycling the connection to global pool never happens
  2. I made an experiment by adding some temporary print statements to the pool `done()` method and ran all tenant migration tests - the log was never printed
  3. I actually prepared a fix that will prevent this from happening in the future

The reasoning why I still think a fix is necessary: we are only one refactoring away from the security condition being triggered. Indeed we use DBClientConnection::connect, which is not using global pool. However, I don't see why a refactoring that would also make using the DBClientReplicaSet::connect is impossible. Nobody will ever remember this little detail and the security risk may be introduced by unrelated code changes years down the road. So I'm posting the fix.

Comment by Andrew Shuvalov (Inactive) [ 10/Dec/20 ]

cheahuychou.mao said:

"I don’t think this issue exists on the donor side since we use separate NetworkInterfaceTL/ConnectionPool for each migration.

I also don't this issue exists on the recipient side. Here is the reasoning:

  1.  DBConnectionPool creates each DBClientBase connection inside DBConnectionPool::get here and saves it for reuse inside DBConnectionPool::release here.  
  2. globalConnPool.get and globalConnPool.release are only used inside ScopedDbConnection and DBClientReplicaSet.
  3. TenantMigrationRecipientService calls ConnectionString::connect on the donor’s primary connection string so it uses DBClientConnection::connect not DBClientReplicaSet::connect (here is the code that I had to change to make the recipient use SSL connection to connect to the donor)."

I will confirm it later when pending SSL related code is submitted, I still want to do an experiment but it requires changes that are not in the repo yet.

Generated at Thu Feb 08 05:29:49 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.