Skip to content

Commit 8ff262a

Browse files
drebbe-intrepiddrebbe-intrepid
authored
Merge pull request #261 from intrepidcs/codex/fix-240-payload-capacity
Prevent ExtraDataPtr reads beyond payload capacity
2 parents c76cddd + c829a75 commit 8ff262a

6 files changed

Lines changed: 497 additions & 18 deletions

File tree

‎include/object_spy_message.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ typedef struct
3939
{
4040
PyObject_HEAD icsSpyMessage msg;
4141
bool noExtraDataPtrCleanup;
42+
// Private bound, independent of writable protocol/length/ownership fields.
43+
size_t extraDataCapacity;
4244
} spy_message_object;
4345
#pragma pack(pop)
4446

@@ -47,6 +49,7 @@ typedef struct
4749
{
4850
PyObject_HEAD icsSpyMessageJ1850 msg;
4951
bool noExtraDataPtrCleanup;
52+
size_t extraDataCapacity;
5053
} spy_message_j1850_object;
5154
#pragma pack(pop)
5255

@@ -63,5 +66,8 @@ extern PyTypeObject spy_message_j1850_object_type;
6366
#define PySpyMessageJ1850_GetObject(obj) ((spy_message_j1850_object*)obj)
6467

6568
bool setup_spy_message_object(PyObject* module);
69+
size_t spy_message_extra_data_length(const icsSpyMessage& msg);
70+
bool spy_message_validate_extra_data(const spy_message_object* obj);
71+
void spy_message_record_received_extra_data(PyObject* obj);
6672

6773
#endif // _OBJECT_SPY_MESSAGE_H_

‎src/methods.cpp‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1845,6 +1845,19 @@ PyObject* meth_transmit_messages(PyObject* self, PyObject* args)
18451845
if (!PyTuple_CheckExact(tuple)) {
18461846
return set_ics_exception(exception_argument_error(), "Second argument must be of tuple type!");
18471847
}
1848+
for (Py_ssize_t i = 0; i < PyTuple_Size(tuple); ++i) {
1849+
PyObject* item = PyTuple_GetItem(tuple, i);
1850+
if (!PySpyMessage_CheckExact(item) && !PySpyMessageJ1850_CheckExact(item)) {
1851+
if (created_tuple)
1852+
Py_DECREF(tuple);
1853+
return set_ics_exception(exception_runtime_error(), "Expected SpyMessage or SpyMessageJ1850");
1854+
}
1855+
if (!spy_message_validate_extra_data((spy_message_object*)item)) {
1856+
if (created_tuple)
1857+
Py_DECREF(tuple);
1858+
return NULL;
1859+
}
1860+
}
18481861
try {
18491862
ice::Library* lib = dll_get_library();
18501863
if (!lib) {
@@ -1956,6 +1969,7 @@ PyObject* meth_get_messages(PyObject* self, PyObject* args)
19561969
// Looks like icsneo40 does its own memory management so don't delete when we dealloc
19571970
msg->noExtraDataPtrCleanup = true;
19581971
}
1972+
spy_message_record_received_extra_data(_obj);
19591973
PyTuple_SetItem(tuple, i, _obj);
19601974
}
19611975
PyObject* result = Py_BuildValue("(O,i)", tuple, errors);
@@ -2834,6 +2848,7 @@ PyObject* meth_coremini_read_tx_message(PyObject* self, PyObject* args) // Scrip
28342848
}
28352849
gil.restore();
28362850
}
2851+
spy_message_record_received_extra_data(msg);
28372852
return msg;
28382853
} catch (ice::Exception& ex) {
28392854
return set_ics_exception(exception_runtime_error(), (char*)ex.what());
@@ -2885,7 +2900,7 @@ PyObject* meth_coremini_read_rx_message(PyObject* self, PyObject* args) // Scrip
28852900
auto gil = PyAllowThreads();
28862901
if (!icsneoScriptReadRxMessage(handle,
28872902
index,
2888-
&PySpyMessageJ1850_GetObject(msg_mask)->msg,
2903+
&PySpyMessageJ1850_GetObject(msg)->msg,
28892904
&PySpyMessageJ1850_GetObject(msg_mask)->msg)) {
28902905
gil.restore();
28912906
return set_ics_exception(exception_runtime_error(), "icsneoScriptReadRxMessage() Failed");
@@ -2912,6 +2927,8 @@ PyObject* meth_coremini_read_rx_message(PyObject* self, PyObject* args) // Scrip
29122927
}
29132928
gil.restore();
29142929
}
2930+
spy_message_record_received_extra_data(msg);
2931+
spy_message_record_received_extra_data(msg_mask);
29152932
return Py_BuildValue("(O,O)", msg, msg_mask);
29162933
} catch (ice::Exception& ex) {
29172934
return set_ics_exception(exception_runtime_error(), (char*)ex.what());
@@ -2952,6 +2969,8 @@ PyObject* meth_coremini_write_tx_message(PyObject* self, PyObject* args) // icsn
29522969
}
29532970
msg = (void*)&PySpyMessage_GetObject(msg_obj)->msg;
29542971
}
2972+
if (!spy_message_validate_extra_data((spy_message_object*)msg_obj))
2973+
return NULL;
29552974
try {
29562975
ice::Library* lib = dll_get_library();
29572976
if (!lib) {
@@ -2991,6 +3010,12 @@ PyObject* meth_coremini_write_rx_message(PyObject* self, PyObject* args) // icsn
29913010
if (!PyNeoDeviceEx_GetHandle(obj, &handle)) {
29923011
return NULL;
29933012
}
3013+
if ((PySpyMessage_CheckExact(msg_obj) || PySpyMessageJ1850_CheckExact(msg_obj)) &&
3014+
!spy_message_validate_extra_data((spy_message_object*)msg_obj))
3015+
return NULL;
3016+
if ((PySpyMessage_CheckExact(msg_mask_obj) || PySpyMessageJ1850_CheckExact(msg_mask_obj)) &&
3017+
!spy_message_validate_extra_data((spy_message_object*)msg_mask_obj))
3018+
return NULL;
29943019
void* msg = NULL;
29953020
void* msg_mask = NULL;
29963021
if (j1850) {

‎src/object_spy_message.cpp‎

Lines changed: 107 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,37 @@
11
#include "object_spy_message.h"
22

3+
size_t spy_message_extra_data_length(const icsSpyMessage& msg)
4+
{
5+
if (msg.Protocol == SPY_PROTOCOL_A2B || msg.Protocol == SPY_PROTOCOL_ETHERNET ||
6+
msg.Protocol == SPY_PROTOCOL_SPI || msg.Protocol == SPY_PROTOCOL_WBMS)
7+
return (msg.NumberBytesHeader << 8) | msg.NumberBytesData;
8+
return msg.NumberBytesData;
9+
}
10+
11+
bool spy_message_validate_extra_data(const spy_message_object* obj)
12+
{
13+
if (obj->msg.ExtraDataPtr && spy_message_extra_data_length(obj->msg) > obj->extraDataCapacity) {
14+
PyErr_SetString(PyExc_ValueError, "ExtraDataPtr length exceeds its buffer capacity");
15+
return false;
16+
}
17+
return true;
18+
}
19+
20+
void spy_message_record_received_extra_data(PyObject* o)
21+
{
22+
spy_message_object* obj = (spy_message_object*)o;
23+
// The DLL owns the pointer; its original reported length is the readable bound.
24+
obj->extraDataCapacity = obj->msg.ExtraDataPtr ? spy_message_extra_data_length(obj->msg) : 0;
25+
obj->noExtraDataPtrCleanup = true;
26+
}
27+
328
static int spy_message_object_alloc(spy_message_object* self, PyObject* args, PyObject* kwds)
429
{
530
(void)args;
631
(void)kwds;
732
memset(&self->msg, 0, sizeof(self->msg));
833
self->noExtraDataPtrCleanup = false;
34+
self->extraDataCapacity = 0;
935
return 0;
1036
}
1137

@@ -98,6 +124,8 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name)
98124
Py_DECREF(temp);
99125
return data;
100126
} else if (PyUnicode_CompareWithASCIIString(attr_name, "ExtraDataPtr") == 0) {
127+
if (!spy_message_validate_extra_data((spy_message_object*)o))
128+
return NULL;
101129
spy_message_j1850_object* obj = (spy_message_j1850_object*)o;
102130
unsigned char* ExtraDataPtr = (unsigned char*)obj->msg.ExtraDataPtr;
103131
bool extra_data_ptr_enabled = obj->msg.ExtraDataPtrEnabled != 0;
@@ -108,14 +136,7 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name)
108136
&& obj->msg.ExtraDataPtr != NULL) {
109137
extra_data_ptr_enabled = true;
110138
}
111-
int actual_size = 0;
112-
// Some newer protocols are packing the length into NumberBytesHeader also so lets handle it here...
113-
if (obj->msg.Protocol == SPY_PROTOCOL_A2B || obj->msg.Protocol == SPY_PROTOCOL_ETHERNET ||
114-
obj->msg.Protocol == SPY_PROTOCOL_SPI || obj->msg.Protocol == SPY_PROTOCOL_WBMS) {
115-
actual_size = (obj->msg.NumberBytesHeader << 8) | obj->msg.NumberBytesData;
116-
} else {
117-
actual_size = obj->msg.NumberBytesData;
118-
}
139+
size_t actual_size = spy_message_extra_data_length(((spy_message_object*)o)->msg);
119140
if (extra_data_ptr_enabled && actual_size && obj->msg.ExtraDataPtr) {
120141
PyObject* tuple = PyTuple_New(actual_size);
121142
for (int i = 0; i < actual_size; ++i) {
@@ -130,7 +151,7 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name)
130151
}
131152
}
132153

133-
static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* value)
154+
static int spy_message_object_setattr_impl(PyObject* o, PyObject* name, PyObject* value)
134155
{
135156
// tp_setattro receives NULL for deletion. Let descriptors reject deletion
136157
// before any custom setter inspects the value or changes message storage.
@@ -155,14 +176,6 @@ static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* val
155176
return -1;
156177
obj->msg.NumberBytesHeader = static_cast<uint8_t>(length);
157178
return 0;
158-
} else if (PyUnicode_CompareWithASCIIString(name, "Protocol") == 0) {
159-
// Ethernet behavior is backward to CAN and will crash if enabled.
160-
long protocol = PyLong_AsLong(value);
161-
if (protocol == -1 && PyErr_Occurred())
162-
PyErr_Clear(); // let PyObject_GenericSetAttr report the type error
163-
else if (protocol == SPY_PROTOCOL_ETHERNET)
164-
obj->msg.ExtraDataPtrEnabled = 0;
165-
return PyObject_GenericSetAttr(o, name, value);
166179
} else if (PyUnicode_CompareWithASCIIString(name, "ExtraDataPtr") == 0) {
167180
if (!PyTuple_Check(value)) {
168181
PyErr_Format(PyExc_AttributeError,
@@ -200,6 +213,7 @@ static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* val
200213
if (obj->msg.ExtraDataPtr != NULL && !obj->noExtraDataPtrCleanup)
201214
delete[] (unsigned char*)obj->msg.ExtraDataPtr;
202215
obj->msg.ExtraDataPtr = buffer;
216+
obj->extraDataCapacity = (size_t)length;
203217
if (packs_length)
204218
obj->msg.NumberBytesHeader = static_cast<uint8_t>(length >> 8);
205219
obj->msg.NumberBytesData = static_cast<uint8_t>(length & 0xFF);
@@ -219,6 +233,7 @@ static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* val
219233
if (obj->msg.ExtraDataPtr != NULL) {
220234
delete[] (unsigned char*)obj->msg.ExtraDataPtr;
221235
obj->msg.ExtraDataPtr = NULL;
236+
obj->extraDataCapacity = 0;
222237
}
223238
} else if (enabled != 0 && obj->msg.Protocol == SPY_PROTOCOL_ETHERNET) {
224239
// Ethernet always needs to be set to 0
@@ -230,6 +245,81 @@ static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* val
230245
}
231246
}
232247

248+
static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* value)
249+
{
250+
// Subclass deletion descriptors may execute Python. Never include them in
251+
// a native-message rollback transaction.
252+
if (!value)
253+
return PyObject_GenericSetAttr(o, name, value);
254+
// These assignments can reinterpret or enlarge the payload length. Roll back
255+
// the complete native message, including Data/Header bytes, on failure.
256+
bool scalar_length = PyUnicode_CompareWithASCIIString(name, "NumberBytesData") == 0 ||
257+
PyUnicode_CompareWithASCIIString(name, "NumberBytesHeader") == 0 ||
258+
PyUnicode_CompareWithASCIIString(name, "Protocol") == 0;
259+
bool data = PyUnicode_CompareWithASCIIString(name, "Data") == 0;
260+
bool header = PyUnicode_CompareWithASCIIString(name, "Header") == 0;
261+
bool changes_length = scalar_length || data || header;
262+
// __index__ can execute arbitrary Python, including replacing the payload.
263+
// Finish all such conversions BEFORE snapshotting the message for rollback.
264+
PyObject* normalized = NULL;
265+
if (value && scalar_length) {
266+
normalized = PyNumber_Index(value);
267+
if (!normalized)
268+
return -1;
269+
long byte = PyLong_AsLong(normalized);
270+
if ((byte == -1 && PyErr_Occurred()) || byte < 0 || byte > 255) {
271+
Py_DECREF(normalized);
272+
if (!PyErr_Occurred())
273+
PyErr_SetString(PyExc_ValueError, "Message length and protocol fields must be in range 0..255");
274+
return -1;
275+
}
276+
// Reject truncation rather than letting T_UBYTE issue a warning inside
277+
// the transaction: a user warning hook can also replace the payload.
278+
} else if (value && (data || header) && PyTuple_Check(value) &&
279+
PyTuple_Size(value) <= (data ? 8 : 4)) {
280+
normalized = PyTuple_New(PyTuple_Size(value));
281+
if (!normalized)
282+
return -1;
283+
for (Py_ssize_t i = 0; i < PyTuple_Size(value); ++i) {
284+
PyObject* byte = PyNumber_Index(PyTuple_GetItem(value, i));
285+
if (!byte) {
286+
Py_DECREF(normalized);
287+
return -1;
288+
}
289+
PyTuple_SET_ITEM(normalized, i, byte);
290+
}
291+
}
292+
spy_message_object* obj = (spy_message_object*)o;
293+
icsSpyMessage original = obj->msg;
294+
int result = 0;
295+
if (scalar_length) {
296+
// These native fields, like Data/Header, must not dispatch subclass
297+
// descriptors inside the transaction: they can replace/free payloads.
298+
unsigned char byte = (unsigned char)PyLong_AsLong(normalized);
299+
if (PyUnicode_CompareWithASCIIString(name, "NumberBytesData") == 0)
300+
obj->msg.NumberBytesData = byte;
301+
else if (PyUnicode_CompareWithASCIIString(name, "NumberBytesHeader") == 0)
302+
obj->msg.NumberBytesHeader = byte;
303+
else {
304+
obj->msg.Protocol = byte;
305+
if (byte == SPY_PROTOCOL_ETHERNET)
306+
obj->msg.ExtraDataPtrEnabled = 0;
307+
}
308+
} else {
309+
result = spy_message_object_setattr_impl(o, name, normalized ? normalized : value);
310+
}
311+
Py_XDECREF(normalized);
312+
if (result == 0 && changes_length && obj->msg.ExtraDataPtr &&
313+
spy_message_extra_data_length(obj->msg) > obj->extraDataCapacity) {
314+
// Restore before allocating the exception: cyclic-GC finalizers may
315+
// execute Python during allocation and must see the committed state.
316+
obj->msg = original;
317+
PyErr_SetString(PyExc_ValueError, "ExtraDataPtr length exceeds its buffer capacity");
318+
return -1;
319+
}
320+
return result;
321+
}
322+
233323
static PyMemberDef spy_message_object_members[] = {
234324
{ "StatusBitField", T_UINT, offsetof(spy_message_object, msg.StatusBitField), 0, "StatusBitField" },
235325
{ "StatusBitField2", T_UINT, offsetof(spy_message_object, msg.StatusBitField2), 0, "StatusBitField2" },

‎tests/extra_data_capacity_mock.cpp‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
#include <cstring>
2+
#include "icsnVC40.h"
3+
4+
#ifdef _WIN32
5+
#define API extern "C" __declspec(dllexport) int __stdcall
6+
#else
7+
#define API extern "C" int
8+
#endif
9+
10+
static unsigned char payload[300];
11+
static int calls;
12+
13+
static void receive(icsSpyMessage* msg)
14+
{
15+
memset(msg, 0, sizeof(*msg));
16+
for (int i = 0; i < 300; ++i)
17+
payload[i] = (unsigned char)i;
18+
msg->Protocol = SPY_PROTOCOL_ETHERNET;
19+
msg->NumberBytesHeader = 1;
20+
msg->NumberBytesData = 44;
21+
msg->ExtraDataPtr = payload;
22+
}
23+
24+
API icsneoGetDLLVersion() { return 1; }
25+
API icsneoWaitForRxMessagesWithTimeOut(void*, unsigned int) { return 1; }
26+
API icsneoGetMessages(void*, icsSpyMessage* msg, int* count, int* errors)
27+
{
28+
receive(msg);
29+
*count = 1;
30+
*errors = 0;
31+
return 1;
32+
}
33+
API icsneoScriptReadTxMessage(void*, unsigned int, icsSpyMessage* msg)
34+
{
35+
receive(msg);
36+
return 1;
37+
}
38+
API icsneoScriptReadRxMessage(void*, unsigned int, icsSpyMessage* msg, icsSpyMessage* mask)
39+
{
40+
receive(msg);
41+
receive(mask);
42+
return 1;
43+
}
44+
API icsneoTxMessages(void*, icsSpyMessage*, int, int) { ++calls; return 1; }
45+
API capacity_mock_calls() { return calls; }

0 commit comments

Comments
 (0)