-
Notifications
You must be signed in to change notification settings - Fork 471
Open
Description
I noticed that the connection pool is leaving a “dirty” session. Meaning, after putting a connection back to the pool, the next process taking it might find some session values stored in mssql. I use sp_set_session_context to store variables.
This is a security bug, as the standard way of implementing row level security is to use sp_set_session_context.
tested on:
tedious: 14.1.0
mssql: 7.3.5
sql database: mssql azure database
Expected behaviour:
Session must be clean, connection re-used
Actual behaviour:
session is not cleaned.
Metadata
Metadata
Assignees
Labels
No labels
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
dhensby commentedon Mar 22, 2023
I'd say this is expected behaviour/misuse. Definitely not a security issued unless you can provide me with some real world examples about how an attacker could exploit this.
Connections are taken from the pool and then put back, depending on your using the pool/making requests (it's not clear here what that is as you've not provided any code) it is expected that changes you make to a connection will remain on that connection when it is returned to the pool and you must take care to ensure you're handling this appropriately.
If you're setting contexts on connections you need to make sure that you are resetting those before returning them to the pool.
tobiasgrossmann commentedon Mar 22, 2023
Example, is provided by microsoft: "Scenario for users who connect to the database through a middle-tier application"
https://learn.microsoft.com/en-us/sql/relational-databases/security/row-level-security?view=sql-server-ver16
Due to this error and following Microsoft's practice, the system can randomly disclose data to users who are not supposed to view it.
Think of salaries or medical records. That is a security issue, and one that can get expensive.
The connection is the physical communication channel between SQL Server and the application: the TCP socket, the named pipe, the shared memory region. The session in SQL Server corresponds to the definition of a [session]: a semi-permanent container of state for an information exchange. (Wikipedia)
It's just wrong to not distinguish them.
dhensby commentedon Mar 22, 2023
Regarding the issue you have reported: as you haven't provided any example code, the support I can provide is limited. As I've said, you'll have to clear the session on the connection before returning it to the pool, that will resolve your problem as far as I can tell.
In terms of if this is a security issue: If you can't provide a proof-of-concept about how this is exploitable, then it's not a provable vulnerability and just an unfounded assertion.
If you're somehow adding context to a connection and then returning it to a pool and then pulling it from the pool again whilst not resetting the context and expecting it to magically now have some new context, that is misuse. The same way creating constructing a query in a way that is vulnerable to SQL injection is misuse and not a vulnerability of the library.
Just like SQL injection vulnerabilities, the fact you're not cleaning the connection context can be a security issue for your application but it's not a concern of the underlying library.
Now, if you're asking that when connections are returned or obtained from the pool that
sp_reset_connection
is called, then that's fair enough and is a feature request I'd be open to.dhensby commentedon Mar 22, 2023
For reference, here is a similar issue for SQLAlchemy
tobiasgrossmann commentedon Mar 22, 2023
You are seriously asking me to copy and paste the example from microsoft to this ticket?
https://learn.microsoft.com/en-us/sql/relational-databases/security/row-level-security?view=sql-server-ver16
It is the proof-of-concept.
SQLAlchemy is a different case, cause its the same process. In node.js with mssql it could be different processes and so different users sharing sessions. Which makes no sense. If this is by design, then the design is simple wrong. There is a reason for making a difference between session and connection.
dhensby commentedon Mar 22, 2023
No, I'm asking for a PoC for how this library is vulnerable to a security issue. Specifically one showing that it's a fundamental flaw in the library vs consumers of the library not cleaning up after themselves. That means Node code that shows a predictable and reproducible problem and a rational as to why it's a flaw with the library and not user error.
Yes, I said similar issue, not same. It's about session data being cleaned from the connection when releasing connections from a pool.
Incorrect, an sql pool is not shared across processes. It may be shared across requests but Node is single process and single thread, not multi-process, so the pool is not shared across processes. Each instance of a node process will have its own pool.
I think it depends on the perspective; however I would agree that most of the time you want to get a "clean" connection from the pool. I'm not disagreeing there and, as I said, happy to entertain the idea of cleaning the connection when it's released. What I'm saying is this is not a security issue.
tobiasgrossmann commentedon Mar 22, 2023
Then please add this behavior to the documentation. That not only the connection is pooled, also the session and any may added data is.
PonchoPowers commentedon Nov 8, 2024
What about when people use Workers?
dhensby commentedon Nov 11, 2024
As far as this library and how it's implemented, it is not possible to share a pool between workers.
I believe it is theoretically possible to share sockets between parent and child workers, so a child worker could in theory share an sql connection, but that's well outside the scope of the library.
dataclear commentedon Nov 27, 2024
If the pool of connections can keep sessions open, does that also mean that session handling set statements can also be carried over?
SET DEADLOCK_PRIORITY LOW
,SET IDENTITY_INSERT ON
orSET NOCOUNT ON
?If so does this mean we should set these when obtaining a new connection from the pool?
dhensby commentedon Nov 27, 2024
A connection is returned to the pool in the exact state it is in when it is no longer being used by the application.
This means these statements will stay with the connection, yes, but you can't make any assumptions that the next time you retrieve a connection from the pool that it will be in a particular state if you have no ensured this in the application layer.
The library does not do any "magic" if you put a connection back in the pool in a particular state, that is how it will remain until it is closed or cleaned-up.
heroboy commentedon Jun 6, 2025
I tried this:
Only the first output is
[]
, the rest are[1]
. So theset nocount on
status doesn't stay with the connection.If this can stay, what about declare a variable? Is it remain available when the connection reuse again?
dhensby commentedon Jun 6, 2025
@heroboy that depends how many connections are in your pool and whether you're ever getting the same connection out again.
I wouldn't be relying on the connections retaining state because you have no way to know if the connection you use for one query will be the same as the one you use for the next.
heroboy commentedon Jun 7, 2025
@dhensby
That's why I wrote a for loop that runs 10 times—eventually, there would always be a connection object that inherits the state when
i == 0
. However, in reality, not a single instance ofSET NOCOUNT ON
was inherited.I'm not relying on the connection retaining state, but I do need to know which states are preserved and which are reset. For the states that aren't reset, I need to explicitly set them to the desired values each time I obtain a new connection object.
For example, if the
SET NOCOUNT ON
state were preserved, and my current SQL statement needs to userowsAffected
, then I would have to includeSET NOCOUNT OFF
in this SQL statement.There are too many states in SQL for me to be familiar with all of them. If the connection object retrieved from the connection pool isn't "clean" enough and causes unpredictable effects on the SQL statements being executed, that would be quite troubling for me.
Actually, I care quite a lot about this issue. I've come across sp_reset_connection and also seen related protocol handling in tedious, but I still don't really understand the correct way to approach it. I've thought about how to implement a proper connection pool myself, whether in Node.js or in ADO (the traditional COM-based ADO, not ADO.NET).
I think for this library, it would be sufficient if you could handle it to the same level as ADO.NET.
4 remaining items