Hi, I sent this email a while ago[0] and got no answer. re-sending. Bug is still affecting the source code. cheers, Hugo [0] http://lists.pjsip.org/pipermail/pjsip_lists.pjsip.org/2018-April/040928.html original message: Dear pjsip developers, The latest trunk revision 5778 is vulnerable to a global buffer overflow (tls_init function in pjlib/src/pj/ssl_sock_gtls.c). This bug may be triggered by remote attackers to crash the client. ASan stacktrace: ================================================================= ==23860==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7ffff6a70b60 at pc 0x7ffff51fc301 bp 0x7fffffffc860 sp 0x7fffffffc850 WRITE of size 4 at 0x7ffff6a70b60 thread T0 #0 0x7ffff51fc300 in tls_init ../src/pj/ssl_sock_gtls.c:480 #1 0x7ffff5203212 in tls_ciphers_fill ../src/pj/ssl_sock_gtls.c:2006 #2 0x7ffff52036a6 in pj_ssl_cipher_name ../src/pj/ssl_sock_gtls.c:2113 #3 0x7ffff4d356ec in ring::SIPCall::getDetails[abi:cxx11]() const /home/hlefeuvre/Development/ring-daemon/src/sip/sipcall.cpp:1134 #4 0x7ffff4c08d7f in ring::Manager::getCallDetails(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/hlefeuvre/Development/ring-daemon/src/manager.cpp:2851 #5 0x7ffff4e32489 in DRing::getCallDetails(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/hlefeuvre/Development/ring-daemon/src/client/callmanager.cpp:289 #6 0x4ff429 in DBusCallManager::getCallDetails(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/hlefeuvre/Development/ring-daemon/bin/dbus/dbuscallmanager.cpp:93 #7 0x508af4 in cx::ring::Ring::CallManager_adaptor::_getCallDetails_stub(DBus::CallMessage const&) /home/hlefeuvre/Development/ring-daemon/bin/dbus/dbuscallmanager.adaptor.h:1045 #8 0x50b03b in DBus::Callback<cx::ring::Ring::CallManager_adaptor, DBus::Message, DBus::CallMessage const&>::call(DBus::CallMessage const&) const /home/hlefeuvre/Development/ring-daemon/contrib/x86_64-linux-gnu/include/dbus-c++-1/dbus-c++/util.h:283 #9 0x53c696 in DBus::Slot<DBus::Message, DBus::CallMessage const&>::call(DBus::CallMessage const&) const (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x53c696) #10 0x53b789 in DBus::InterfaceAdaptor::dispatch_method(DBus::CallMessage const&) (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x53b789) #11 0x54410e in DBus::ObjectAdaptor::handle_message(DBus::Message const&) (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x54410e) #12 0x543685 in DBus::ObjectAdaptor::Private::message_function_stub(DBusConnection*, DBusMessage*, void*) (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x543685) #13 0x7ffff6c3f812 (/lib/x86_64-linux-gnu/libdbus-1.so.3+0x21812) #14 0x7ffff6c30d93 in dbus_connection_dispatch (/lib/x86_64-linux-gnu/libdbus-1.so.3+0x12d93) #15 0x5331a9 in DBus::Connection::Private::do_dispatch() (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x5331a9) #16 0x536110 in DBus::Dispatcher::dispatch_pending(std::__cxx11::list<DBus::Connection::Private*, std::allocator<DBus::Connection::Private*> >&) (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x536110) #17 0x535f0e in DBus::Dispatcher::dispatch_pending() (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x535f0e) #18 0x539c61 in DBus::BusDispatcher::do_iteration() (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x539c61) #19 0x53990f in DBus::BusDispatcher::enter() (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x53990f) #20 0x4930fc in DBusClient::event_loop() /home/hlefeuvre/Development/ring-daemon/bin/dbus/dbusclient.cpp:250 #21 0x48d0d0 in main /home/hlefeuvre/Development/ring-daemon/bin/main.cpp:236 #22 0x7ffff35ed82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) #23 0x48c718 in _start (/home/hlefeuvre/Development/ring-daemon/bin/.libs/lt-dring+0x48c718) 0x7ffff6a70b60 is located 0 bytes to the right of global variable 'tls_ciphers' defined in '../src/pj/ssl_sock_gtls.c:207:3' (0x7ffff6a70520) of size 1600 0x7ffff6a70b60 is located 32 bytes to the left of global variable 'tls_last_error' defined in '../src/pj/ssl_sock_gtls.c:210:12' (0x7ffff6a70b80) of size 4 SUMMARY: AddressSanitizer: global-buffer-overflow ../src/pj/ssl_sock_gtls.c:480 tls_init Shadow bytes around the buggy address: 0x10007ed46110: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007ed46120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007ed46130: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007ed46140: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007ed46150: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x10007ed46160: 00 00 00 00 00 00 00 00 00 00 00 00[f9]f9 f9 f9 0x10007ed46170: 04 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00 0x10007ed46180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007ed46190: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007ed461a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007ed461b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe ==23860==ABORTING Please, note that this stacktrace is coming from a slightly older version of pjsip, so line numbers are not exactly corresponding to the ones from the latest svn revision, but the bug is still present. Analysis: At line 494, tls_init writes a 0 to the id field of tls_ciphers[i] before checking for i < PJ_ARRAY_SIZE(tls_ciphers): 466 /* Initialize GnuTLS. */ 467 static pj_status_t tls_init(void) 468 { [snip] 488 for (i = 0; ; i++) { 489 unsigned char id[2]; 490 const char *suite;[snip] 491 492 suite = gnutls_cipher_suite_info(i, (unsigned char *)id, 493 NULL, NULL, NULL, NULL); 494 tls_ciphers[i].id = 0; 495 /* usually the array size is bigger than the number of available 496 * ciphers anyway, so by checking here we can exit the loop as soon 497 * as either all ciphers have been added or the array is full */ 498 if (suite && i < PJ_ARRAY_SIZE(tls_ciphers)) { [snip] 504 } 505 506 tls_available_ciphers = i; 507 } 508 509 return PJ_SUCCESS; 510 } In this case, PJ_ARRAY_SIZE(tls_ciphers) is 100 and i = 100, so tls_ciphers[100].id = 0 is OOB write. Patch: In the attached patch we add a check making sure that i is smaller than PJ_ARRAY_SIZE(tls_ciphers) before updating tls_ciphers[i]. This should be enough to fix the issue. Index: ssl_sock_gtls.c =================================================================== --- ssl_sock_gtls.c (revision 5783) +++ ssl_sock_gtls.c (working copy) @@ -491,11 +491,16 @@ suite = gnutls_cipher_suite_info(i, (unsigned char *)id, NULL, NULL, NULL, NULL); - tls_ciphers[i].id = 0; + + if (i < PJ_ARRAY_SIZE(tls_ciphers)) + tls_ciphers[i].id = 0; + else + break; + /* usually the array size is bigger than the number of available * ciphers anyway, so by checking here we can exit the loop as soon * as either all ciphers have been added or the array is full */ - if (suite && i < PJ_ARRAY_SIZE(tls_ciphers)) { + if (suite) { tls_ciphers[i].id = (pj_ssl_cipher) (pj_uint32_t) ((id[0] << 8) | id[1]); tls_ciphers[i].name = suite; -- Hugo Lefeuvre (hle) | www.owl.eu.com RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C
Index: ssl_sock_gtls.c
===================================================================
--- ssl_sock_gtls.c (revision 5783)
+++ ssl_sock_gtls.c (working copy)
@@ -491,11 +491,16 @@
suite = gnutls_cipher_suite_info(i, (unsigned char *)id,
NULL, NULL, NULL, NULL);
- tls_ciphers[i].id = 0;
+
+ if (i < PJ_ARRAY_SIZE(tls_ciphers))
+ tls_ciphers[i].id = 0;
+ else
+ break;
+
/* usually the array size is bigger than the number of available
* ciphers anyway, so by checking here we can exit the loop as soon
* as either all ciphers have been added or the array is full */
- if (suite && i < PJ_ARRAY_SIZE(tls_ciphers)) {
+ if (suite) {
tls_ciphers[i].id = (pj_ssl_cipher)
(pj_uint32_t) ((id[0] << 8) | id[1]);
tls_ciphers[i].name = suite;
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Visit our blog: http://blog.pjsip.org pjsip mailing list pjsip@xxxxxxxxxxxxxxx http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
