FIX: Memory Leak and Security Risk with Static Token Buffer #264
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses a memory leak and security vulnerability in SQL connection attribute handling by replacing a static buffer with a stack-allocated buffer and adding secure data erasure.
- Replaced static vector buffer with stack-allocated string to prevent memory accumulation
- Added secure erasure of sensitive data after use to mitigate security risks
- Improved memory management for SQL attribute setting operations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/connection/connection.cppLines 172-180 172 SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) {
173 LOG("Setting SQL attribute");
174 SQLPOINTER ptr = nullptr;
175 SQLINTEGER length = 0;
! 176 std::string buffer; // to hold sensitive data temporarily
177
178 if (py::isinstance<py::int_>(value)) {
179 int intValue = value.cast<int>();
180 ptr = reinterpret_cast<SQLPOINTER>(static_cast<uintptr_t>(intValue));Lines 179-189 179 int intValue = value.cast<int>();
180 ptr = reinterpret_cast<SQLPOINTER>(static_cast<uintptr_t>(intValue));
181 length = SQL_IS_INTEGER;
182 } else if (py::isinstance<py::bytes>(value) || py::isinstance<py::bytearray>(value)) {
! 183 buffer = value.cast<std::string>(); // stack buffer
! 184 ptr = buffer.data();
! 185 length = static_cast<SQLINTEGER>(buffer.size());
186 } else {
187 LOG("Unsupported attribute value type");
188 return SQL_ERROR;
189 }Lines 196-206 196 LOG("Set attribute successfully");
197 }
198
199 // Zero out sensitive data if used
! 200 if (!buffer.empty()) {
! 201 std::fill(buffer.begin(), buffer.end(), static_cast<char>(0));
! 202 }
203 return ret;
204 }
205
206 void Connection::applyAttrsBefore(const py::dict& attrs) {📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.connection.connection.cpp: 67.6%
mssql_python.ddbc_bindings.py: 68.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.4%
mssql_python.pybind.connection.connection_pool.cpp: 78.9%
mssql_python.cursor.py: 79.3%
mssql_python.connection.py: 81.7%
mssql_python.helpers.py: 84.7%
mssql_python.auth.py: 85.3%
mssql_python.type.py: 86.8%
mssql_python.pooling.py: 88.8%🔗 Quick Links
|
bewithgaurav
left a comment
bewithgaurav
left a comment
There was a problem hiding this comment.
@gargsaumya - as mentioned in the code coverage comment and the full report as well, I believe we haven't covered this whole function inside connection.cpp - Connection::setAttribute(SQLINTEGER attribute, py::object value)
It would be a bit more helpful for the coverage if we can include some tests inside test_connection.py. could you please include a couple of tests with this PR?
If its already covered, and code coverage is not detecting please let me know.
As we discussed offline, since this is an internal function, we don’t add unit tests for it. The coverage gap related to this function can be taken up in a separate PR to keep the scope of this one focused. |
| length = SQL_IS_INTEGER; | ||
| } else if (py::isinstance<py::bytes>(value) || py::isinstance<py::bytearray>(value)) { | ||
| static std::vector<std::string> buffers; | ||
| buffers.emplace_back(value.cast<std::string>()); |
There was a problem hiding this comment.
This line supports multiple string inside a vector. But new change is only for one string. Did we validate the impact for all scenario?
| ptr = reinterpret_cast<SQLPOINTER>(static_cast<uintptr_t>(intValue)); | ||
| length = SQL_IS_INTEGER; | ||
| } else if (py::isinstance<py::bytes>(value) || py::isinstance<py::bytearray>(value)) { | ||
| static std::vector<std::string> buffers; |
There was a problem hiding this comment.
removing Static keyword and defining the vector outside the loop may solve the issue??
|
|
||
| // Zero out sensitive data if used | ||
| if (!buffer.empty()) { | ||
| std::fill(buffer.begin(), buffer.end(), static_cast<char>(0)); |
There was a problem hiding this comment.
why fill? buffer is not allocated via "new". buffer is also a local variable.
Work Item / Issue Reference
Summary
This pull request improves the handling of sensitive data when setting SQL connection attributes in
connection.cpp. The main change is replacing a static buffer with a stack-allocated buffer to better control memory and securely erase sensitive data after use.Sensitive data handling:
std::string(buffer) to temporarily hold sensitive data when setting SQL attributes. This avoids unnecessary retention of sensitive data in memory.bufferafter it is used, ensuring sensitive data does not remain in memory.