Skip to content

Commit 059dc7e

Browse files
anandoleecopybara-github
anandolee
authored and
copybara-github
committed
Fix NULL byte handling issue in Python Protobuf find symbols in pool
PiperOrigin-RevId: 882122977
1 parent 7fd0570 commit 059dc7e

3 files changed

Lines changed: 120 additions & 37 deletions

File tree

‎python/descriptor_pool.c‎

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -403,13 +403,15 @@ static PyObject* PyUpb_DescriptorPool_FindFileByName(PyObject* _self,
403403
const char* name = PyUpb_VerifyStrData(arg);
404404
if (!name) return NULL;
405405

406-
const upb_FileDef* file = upb_DefPool_FindFileByName(self->symtab, name);
406+
const upb_FileDef* file = upb_DefPool_FindFileByNameWithSize(
407+
self->symtab, name, PyObject_Size(arg));
407408
if (file == NULL && self->db) {
408409
if (!PyUpb_DescriptorPool_TryLoadFilename(self, arg)) return NULL;
409-
file = upb_DefPool_FindFileByName(self->symtab, name);
410+
file = upb_DefPool_FindFileByNameWithSize(self->symtab, name,
411+
PyObject_Size(arg));
410412
}
411413
if (file == NULL) {
412-
return PyErr_Format(PyExc_KeyError, "Couldn't find file %.200s", name);
414+
return PyErr_Format(PyExc_KeyError, "Couldn't find file %S", arg);
413415
}
414416

415417
return PyUpb_FileDescriptor_Get(file);
@@ -428,14 +430,15 @@ static PyObject* PyUpb_DescriptorPool_FindExtensionByName(PyObject* _self,
428430
const char* name = PyUpb_VerifyStrData(arg);
429431
if (!name) return NULL;
430432

431-
const upb_FieldDef* field =
432-
upb_DefPool_FindExtensionByName(self->symtab, name);
433+
const upb_FieldDef* field = upb_DefPool_FindExtensionByNameWithSize(
434+
self->symtab, name, PyObject_Size(arg));
433435
if (field == NULL && self->db) {
434436
if (!PyUpb_DescriptorPool_TryLoadSymbol(self, arg)) return NULL;
435-
field = upb_DefPool_FindExtensionByName(self->symtab, name);
437+
field = upb_DefPool_FindExtensionByNameWithSize(self->symtab, name,
438+
PyObject_Size(arg));
436439
}
437440
if (field == NULL) {
438-
return PyErr_Format(PyExc_KeyError, "Couldn't find extension %.200s", name);
441+
return PyErr_Format(PyExc_KeyError, "Couldn't find extension %S", arg);
439442
}
440443

441444
return PyUpb_FieldDescriptor_Get(field);
@@ -453,14 +456,16 @@ static PyObject* PyUpb_DescriptorPool_FindMessageTypeByName(PyObject* _self,
453456

454457
const char* name = PyUpb_VerifyStrData(arg);
455458
if (!name) return NULL;
459+
Py_ssize_t name_size = PyObject_Size(arg);
456460

457-
const upb_MessageDef* m = upb_DefPool_FindMessageByName(self->symtab, name);
461+
const upb_MessageDef* m =
462+
upb_DefPool_FindMessageByNameWithSize(self->symtab, name, name_size);
458463
if (m == NULL && self->db) {
459464
if (!PyUpb_DescriptorPool_TryLoadSymbol(self, arg)) return NULL;
460-
m = upb_DefPool_FindMessageByName(self->symtab, name);
465+
m = upb_DefPool_FindMessageByNameWithSize(self->symtab, name, name_size);
461466
}
462467
if (m == NULL) {
463-
return PyErr_Format(PyExc_KeyError, "Couldn't find message %.200s", name);
468+
return PyErr_Format(PyExc_KeyError, "Couldn't find message %S", arg);
464469
}
465470

466471
return PyUpb_Descriptor_Get(m);
@@ -502,12 +507,13 @@ static PyObject* PyUpb_DescriptorPool_FindFieldByName(PyObject* _self,
502507
parent_size);
503508
}
504509
if (parent) {
505-
f = upb_MessageDef_FindFieldByName(parent, child);
510+
f = upb_MessageDef_FindFieldByNameWithSize(
511+
parent, child, PyObject_Size(arg) - parent_size - 1);
506512
}
507513
}
508514

509515
if (!f) {
510-
return PyErr_Format(PyExc_KeyError, "Couldn't find message %.200s", name);
516+
return PyErr_Format(PyExc_KeyError, "Couldn't find field %S", arg);
511517
}
512518

513519
return PyUpb_FieldDescriptor_Get(f);
@@ -526,13 +532,15 @@ static PyObject* PyUpb_DescriptorPool_FindEnumTypeByName(PyObject* _self,
526532
const char* name = PyUpb_VerifyStrData(arg);
527533
if (!name) return NULL;
528534

529-
const upb_EnumDef* e = upb_DefPool_FindEnumByName(self->symtab, name);
535+
const upb_EnumDef* e = upb_DefPool_FindEnumByNameWithSize(self->symtab, name,
536+
PyObject_Size(arg));
530537
if (e == NULL && self->db) {
531538
if (!PyUpb_DescriptorPool_TryLoadSymbol(self, arg)) return NULL;
532-
e = upb_DefPool_FindEnumByName(self->symtab, name);
539+
e = upb_DefPool_FindEnumByNameWithSize(self->symtab, name,
540+
PyObject_Size(arg));
533541
}
534542
if (e == NULL) {
535-
return PyErr_Format(PyExc_KeyError, "Couldn't find enum %.200s", name);
543+
return PyErr_Format(PyExc_KeyError, "Couldn't find enum %S", arg);
536544
}
537545

538546
return PyUpb_EnumDescriptor_Get(e);
@@ -563,12 +571,16 @@ static PyObject* PyUpb_DescriptorPool_FindOneofByName(PyObject* _self,
563571
parent_size);
564572
}
565573
if (parent) {
566-
const upb_OneofDef* o = upb_MessageDef_FindOneofByName(parent, child);
574+
const upb_OneofDef* o = upb_MessageDef_FindOneofByNameWithSize(
575+
parent, child, PyObject_Size(arg) - parent_size - 1);
576+
if (!o) {
577+
return PyErr_Format(PyExc_KeyError, "Couldn't find oneof %S", arg);
578+
}
567579
return PyUpb_OneofDescriptor_Get(o);
568580
}
569581
}
570582

571-
return PyErr_Format(PyExc_KeyError, "Couldn't find oneof %.200s", name);
583+
return PyErr_Format(PyExc_KeyError, "Couldn't find oneof %S", arg);
572584
}
573585

574586
static PyObject* PyUpb_DescriptorPool_FindServiceByName(PyObject* _self,
@@ -578,13 +590,15 @@ static PyObject* PyUpb_DescriptorPool_FindServiceByName(PyObject* _self,
578590
const char* name = PyUpb_VerifyStrData(arg);
579591
if (!name) return NULL;
580592

581-
const upb_ServiceDef* s = upb_DefPool_FindServiceByName(self->symtab, name);
593+
const upb_ServiceDef* s = upb_DefPool_FindServiceByNameWithSize(
594+
self->symtab, name, PyObject_Size(arg));
582595
if (s == NULL && self->db) {
583596
if (!PyUpb_DescriptorPool_TryLoadSymbol(self, arg)) return NULL;
584-
s = upb_DefPool_FindServiceByName(self->symtab, name);
597+
s = upb_DefPool_FindServiceByNameWithSize(self->symtab, name,
598+
PyObject_Size(arg));
585599
}
586600
if (s == NULL) {
587-
return PyErr_Format(PyExc_KeyError, "Couldn't find service %.200s", name);
601+
return PyErr_Format(PyExc_KeyError, "Couldn't find service %S", arg);
588602
}
589603

590604
return PyUpb_ServiceDescriptor_Get(s);
@@ -608,12 +622,13 @@ static PyObject* PyUpb_DescriptorPool_FindMethodByName(PyObject* _self,
608622
upb_DefPool_FindServiceByNameWithSize(self->symtab, name, parent_size);
609623
}
610624
if (!parent) goto err;
611-
const upb_MethodDef* m = upb_ServiceDef_FindMethodByName(parent, child);
625+
const upb_MethodDef* m = upb_ServiceDef_FindMethodByNameWithSize(
626+
parent, child, PyObject_Size(arg) - parent_size - 1);
612627
if (!m) goto err;
613628
return PyUpb_MethodDescriptor_Get(m);
614629

615630
err:
616-
return PyErr_Format(PyExc_KeyError, "Couldn't find method %.200s", name);
631+
return PyErr_Format(PyExc_KeyError, "Couldn't find method %S", arg);
617632
}
618633

619634
static PyObject* PyUpb_DescriptorPool_FindFileContainingSymbol(PyObject* _self,
@@ -622,6 +637,9 @@ static PyObject* PyUpb_DescriptorPool_FindFileContainingSymbol(PyObject* _self,
622637

623638
const char* name = PyUpb_VerifyStrData(arg);
624639
if (!name) return NULL;
640+
if (PyObject_Size(arg) != strlen(name)) {
641+
return PyErr_Format(PyExc_KeyError, "Couldn't find symbol %S", arg);
642+
}
625643

626644
const upb_FileDef* f =
627645
upb_DefPool_FindFileContainingSymbol(self->symtab, name);
@@ -630,7 +648,7 @@ static PyObject* PyUpb_DescriptorPool_FindFileContainingSymbol(PyObject* _self,
630648
f = upb_DefPool_FindFileContainingSymbol(self->symtab, name);
631649
}
632650
if (f == NULL) {
633-
return PyErr_Format(PyExc_KeyError, "Couldn't find symbol %.200s", name);
651+
return PyErr_Format(PyExc_KeyError, "Couldn't find symbol %S", arg);
634652
}
635653

636654
return PyUpb_FileDescriptor_Get(f);

‎python/google/protobuf/internal/descriptor_pool_test.py‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
from google.protobuf import duration_pb2
3434
from google.protobuf import struct_pb2
3535
from google.protobuf import timestamp_pb2
36+
from absl.testing import parameterized
3637
from google.protobuf import unittest_features_pb2
3738
from google.protobuf import unittest_import_pb2
3839
from google.protobuf import unittest_import_public_pb2
@@ -1707,5 +1708,70 @@ def testFindAllExtensionsReturnsNoneList(self):
17071708
self.assertEqual(len(extensions), 0)
17081709

17091710

1711+
class DescriptorPoolParaTest(parameterized.TestCase):
1712+
1713+
@parameterized.named_parameters(
1714+
(
1715+
'Message',
1716+
'google.protobuf.python.internal.Factory1Message',
1717+
'FindMessageTypeByName',
1718+
),
1719+
(
1720+
'Field',
1721+
'google.protobuf.python.internal.Factory1Message.list_value',
1722+
'FindFieldByName',
1723+
),
1724+
(
1725+
'File',
1726+
'google/protobuf/internal/factory_test1.proto',
1727+
'FindFileByName',
1728+
),
1729+
(
1730+
'Enum',
1731+
'google.protobuf.python.internal.Factory1Enum',
1732+
'FindEnumTypeByName',
1733+
),
1734+
(
1735+
'Oneof',
1736+
'google.protobuf.python.internal.Factory2Message.oneof_field',
1737+
'FindOneofByName',
1738+
),
1739+
(
1740+
'Extension',
1741+
'google.protobuf.python.internal.another_field',
1742+
'FindExtensionByName',
1743+
),
1744+
('Service', 'proto2_unittest.TestService', 'FindServiceByName'),
1745+
('Method', 'proto2_unittest.TestService.Foo', 'FindMethodByName'),
1746+
)
1747+
def testFindNULLByte(self, normal_name, method_name):
1748+
# Attack: Craft malicious type name with embedded null byte
1749+
malicious_name = normal_name + '\x00Tail.Content.Ignore'
1750+
method = getattr(descriptor_pool.Default(), method_name)
1751+
des = method(normal_name)
1752+
1753+
if hasattr(des, 'full_name'):
1754+
self.assertEqual(normal_name, des.full_name)
1755+
else:
1756+
self.assertEqual(normal_name, des.name)
1757+
with self.assertRaises(KeyError) as exc:
1758+
method(malicious_name)
1759+
self.assertIn('Tail.Content', str(exc.exception))
1760+
1761+
@parameterized.named_parameters(
1762+
('Message', 'google.protobuf.python.internal.Factory1Message'),
1763+
('Field', 'google.protobuf.python.internal.Factory1Message.list_value'),
1764+
)
1765+
def testFindNullByteSymbol(self, normal_name):
1766+
malicious_name = normal_name + '\x00Tail.Content'
1767+
file = descriptor_pool.Default().FindFileContainingSymbol(normal_name)
1768+
self.assertEqual(
1769+
'google/protobuf/internal/factory_test1.proto', file.name
1770+
)
1771+
with self.assertRaises(KeyError) as exc:
1772+
descriptor_pool.Default().FindFileContainingSymbol(malicious_name)
1773+
self.assertIn('Tail.Content', str(exc.exception))
1774+
1775+
17101776
if __name__ == '__main__':
17111777
unittest.main()

‎python/google/protobuf/pyext/descriptor_pool.cc‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -235,16 +235,16 @@ static int GcClear(PyObject* pself) {
235235
}
236236

237237
PyObject* SetErrorFromCollector(DescriptorPool::ErrorCollector* self,
238-
const char* name, const char* error_type) {
238+
PyObject* arg, const char* error_type) {
239239
BuildFileErrorCollector* error_collector =
240240
reinterpret_cast<BuildFileErrorCollector*>(self);
241241
if (error_collector && !error_collector->error_message.empty()) {
242-
PyErr_Format(PyExc_KeyError, "Couldn't build file for %s %.200s\n%s",
243-
error_type, name, error_collector->error_message.c_str());
242+
PyErr_Format(PyExc_KeyError, "Couldn't build file for %s %S\n%s",
243+
error_type, arg, error_collector->error_message.c_str());
244244
error_collector->Clear();
245245
return nullptr;
246246
}
247-
PyErr_Format(PyExc_KeyError, "Couldn't find %s %.200s", error_type, name);
247+
PyErr_Format(PyExc_KeyError, "Couldn't find %s %S", error_type, arg);
248248
return nullptr;
249249
}
250250

@@ -261,7 +261,7 @@ static PyObject* FindMessageByName(PyObject* self, PyObject* arg) {
261261

262262
if (message_descriptor == nullptr) {
263263
return SetErrorFromCollector(
264-
reinterpret_cast<PyDescriptorPool*>(self)->error_collector, name,
264+
reinterpret_cast<PyDescriptorPool*>(self)->error_collector, arg,
265265
"message");
266266
}
267267

@@ -284,7 +284,7 @@ static PyObject* FindFileByName(PyObject* self, PyObject* arg) {
284284
py_pool->pool->FindFileByName(absl::string_view(name, name_size));
285285

286286
if (file_descriptor == nullptr) {
287-
return SetErrorFromCollector(py_pool->error_collector, name, "file");
287+
return SetErrorFromCollector(py_pool->error_collector, arg, "file");
288288
}
289289
return PyFileDescriptor_FromDescriptor(file_descriptor);
290290
}
@@ -299,7 +299,7 @@ PyObject* FindFieldByName(PyDescriptorPool* self, PyObject* arg) {
299299
const FieldDescriptor* field_descriptor =
300300
self->pool->FindFieldByName(absl::string_view(name, name_size));
301301
if (field_descriptor == nullptr) {
302-
return SetErrorFromCollector(self->error_collector, name, "field");
302+
return SetErrorFromCollector(self->error_collector, arg, "field");
303303
}
304304

305305

@@ -320,8 +320,7 @@ PyObject* FindExtensionByName(PyDescriptorPool* self, PyObject* arg) {
320320
const FieldDescriptor* field_descriptor =
321321
self->pool->FindExtensionByName(absl::string_view(name, name_size));
322322
if (field_descriptor == nullptr) {
323-
return SetErrorFromCollector(self->error_collector, name,
324-
"extension field");
323+
return SetErrorFromCollector(self->error_collector, arg, "extension field");
325324
}
326325

327326

@@ -342,7 +341,7 @@ PyObject* FindEnumTypeByName(PyDescriptorPool* self, PyObject* arg) {
342341
const EnumDescriptor* enum_descriptor =
343342
self->pool->FindEnumTypeByName(absl::string_view(name, name_size));
344343
if (enum_descriptor == nullptr) {
345-
return SetErrorFromCollector(self->error_collector, name, "enum");
344+
return SetErrorFromCollector(self->error_collector, arg, "enum");
346345
}
347346

348347

@@ -363,7 +362,7 @@ PyObject* FindOneofByName(PyDescriptorPool* self, PyObject* arg) {
363362
const OneofDescriptor* oneof_descriptor =
364363
self->pool->FindOneofByName(absl::string_view(name, name_size));
365364
if (oneof_descriptor == nullptr) {
366-
return SetErrorFromCollector(self->error_collector, name, "oneof");
365+
return SetErrorFromCollector(self->error_collector, arg, "oneof");
367366
}
368367

369368

@@ -386,7 +385,7 @@ static PyObject* FindServiceByName(PyObject* self, PyObject* arg) {
386385
absl::string_view(name, name_size));
387386
if (service_descriptor == nullptr) {
388387
return SetErrorFromCollector(
389-
reinterpret_cast<PyDescriptorPool*>(self)->error_collector, name,
388+
reinterpret_cast<PyDescriptorPool*>(self)->error_collector, arg,
390389
"service");
391390
}
392391

@@ -406,7 +405,7 @@ static PyObject* FindMethodByName(PyObject* self, PyObject* arg) {
406405
absl::string_view(name, name_size));
407406
if (method_descriptor == nullptr) {
408407
return SetErrorFromCollector(
409-
reinterpret_cast<PyDescriptorPool*>(self)->error_collector, name,
408+
reinterpret_cast<PyDescriptorPool*>(self)->error_collector, arg,
410409
"method");
411410
}
412411

@@ -426,7 +425,7 @@ static PyObject* FindFileContainingSymbol(PyObject* self, PyObject* arg) {
426425
absl::string_view(name, name_size));
427426
if (file_descriptor == nullptr) {
428427
return SetErrorFromCollector(
429-
reinterpret_cast<PyDescriptorPool*>(self)->error_collector, name,
428+
reinterpret_cast<PyDescriptorPool*>(self)->error_collector, arg,
430429
"symbol");
431430
}
432431

0 commit comments

Comments
 (0)