This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Harald Welte gerrit-no-reply at lists.osmocom.orgHarald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/c/osmo-msc/+/13630 ) Change subject: libmsc/db.c: fix storing SMS with empty TP-User-Data ...................................................................... libmsc/db.c: fix storing SMS with empty TP-User-Data Thanks to db_sms_test, it was discovered that storing an SMS with empty TP-User-Data (TP-UDL=1) causes buffer overruns in libdbi and it's SQLite3 driver (libdbdsqlite3): DDB NOTICE test_db_sms_store('Empty TP-UD'): ==7791== Invalid write of size 2 ==7791== at 0x857DC60: dbd_quote_binary (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so) ==7791== by 0x5B2B321: dbi_conn_quote_binary_copy (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0) ==7791== by 0x4073B1: db_sms_store (db.c:701) ==7791== by 0x405BB5: test_db_sms_store (db_sms_test.c:310) ==7791== by 0x405BB5: main (db_sms_test.c:546) ==7791== Address 0x7ed1cf0 is 0 bytes after a block of size 0 alloc'd ==7791== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==7791== by 0x857DC4B: dbd_quote_binary (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so) ==7791== by 0x5B2B321: dbi_conn_quote_binary_copy (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0) ==7791== by 0x4073B1: db_sms_store (db.c:701) ==7791== by 0x405BB5: test_db_sms_store (db_sms_test.c:310) ==7791== by 0x405BB5: main (db_sms_test.c:546) ... DDB NOTICE test_db_sms_get('Empty TP-UD'): ==8051== Invalid read of size 1 ==8051== at 0x5B30510: _dbd_decode_binary (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0) ==8051== by 0x857D957: dbd_fetch_row (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so) ==8051== by 0x5B2C86E: dbi_result_seek_row (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0) ==8051== by 0x40828F: next_row (db.c:188) ==8051== by 0x40828F: db_sms_get (db.c:805) ==8051== by 0x406C29: test_db_sms_get (db_sms_test.c:390) ==8051== by 0x405C14: main (db_sms_test.c:547) ==8051== Address 0x8f74641 is 0 bytes after a block of size 1 alloc'd ==8051== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8051== by 0x5DBEB49: strdup (strdup.c:42) ==8051== by 0x857D93C: dbd_fetch_row (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so) ==8051== by 0x5B2C86E: dbi_result_seek_row (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0) ==8051== by 0x40828F: next_row (db.c:188) ==8051== by 0x40828F: db_sms_get (db.c:805) ==8051== by 0x406C29: test_db_sms_get (db_sms_test.c:390) ==8051== by 0x405C14: main (db_sms_test.c:547) ==8051== success, as expected DDB NOTICE verify_sms('Empty TP-UD'): user_data_len mismatch: E0 vs A3 Apparently, dbi_conn_quote_binary_copy() doesn't properly handle zero-length input. Let's guard against this. Observed with: - libdbi-dev 0.9.0-1 - libdbd-sqlite3:amd64 0.9.0-2ubuntu2 Change-Id: If0b2bb557118c5f0e520a2e6c2816336f6028661 --- M src/libmsc/db.c M tests/db_sms/db_sms_test.c M tests/db_sms/db_sms_test.err 3 files changed, 14 insertions(+), 6 deletions(-) Approvals: Jenkins Builder: Verified pespin: Looks good to me, but someone else must approve Harald Welte: Looks good to me, approved diff --git a/src/libmsc/db.c b/src/libmsc/db.c index b564697..add6304 100644 --- a/src/libmsc/db.c +++ b/src/libmsc/db.c @@ -695,14 +695,20 @@ { dbi_result result; char *q_text, *q_daddr, *q_saddr; - unsigned char *q_udata; + unsigned char *q_udata = NULL; time_t now, validity_timestamp; dbi_conn_quote_string_copy(conn, (char *)sms->text, &q_text); dbi_conn_quote_string_copy(conn, (char *)sms->dst.addr, &q_daddr); dbi_conn_quote_string_copy(conn, (char *)sms->src.addr, &q_saddr); - dbi_conn_quote_binary_copy(conn, sms->user_data, sms->user_data_len, - &q_udata); + + /* Guard against zero-length input, as this may cause + * buffer overruns in libdbi / libdbdsqlite3. */ + if (sms->user_data_len > 0) { + dbi_conn_quote_binary_copy(conn, sms->user_data, + sms->user_data_len, + &q_udata); + } now = time(NULL); validity_timestamp = now + sms->validity_minutes * 60; diff --git a/tests/db_sms/db_sms_test.c b/tests/db_sms/db_sms_test.c index 93aed2b..a97f7c7 100644 --- a/tests/db_sms/db_sms_test.c +++ b/tests/db_sms/db_sms_test.c @@ -247,8 +247,6 @@ }, .ud = &sms_tp_ud_set[0], }, -#if 0 - /* FIXME: there is a bug that causes ASAN / Valgrind to complain */ { .name = "Empty TP-UD", .sms = { @@ -267,7 +265,6 @@ }, .ud = NULL, }, -#endif }; static void prepare_sms_test_set(void) diff --git a/tests/db_sms/db_sms_test.err b/tests/db_sms/db_sms_test.err index 73dbd8e..e0a329d 100644 --- a/tests/db_sms/db_sms_test.err +++ b/tests/db_sms/db_sms_test.err @@ -11,6 +11,7 @@ DDB NOTICE test_db_sms_store('Same MSISDN #1'): success, as expected DDB NOTICE test_db_sms_store('Same MSISDN #2'): success, as expected DDB NOTICE test_db_sms_store('Expired SMS'): success, as expected +DDB NOTICE test_db_sms_store('Empty TP-UD'): success, as expected DDB INFO Testing db_sms_get()... DDB NOTICE test_db_sms_get('Regular MO SMS'): success, as expected DDB NOTICE verify_sms('Regular MO SMS'): match @@ -32,6 +33,8 @@ DDB NOTICE verify_sms('Same MSISDN #2'): match DDB NOTICE test_db_sms_get('Expired SMS'): success, as expected DDB NOTICE verify_sms('Expired SMS'): match +DDB NOTICE test_db_sms_get('Empty TP-UD'): success, as expected +DDB NOTICE verify_sms('Empty TP-UD'): match DDB INFO Testing db_sms_get_next_unsent() and db_sms_mark_delivered()... DDB NOTICE db_sms_get_next_unsent(#1): found DDB NOTICE verify_sms('Regular MO SMS'): match @@ -67,4 +70,6 @@ DDB NOTICE test_db_sms_get('Same MSISDN #2'): failure, as expected DDB NOTICE test_db_sms_get('Expired SMS'): unexpected result DDB NOTICE verify_sms('Expired SMS'): match +DDB NOTICE test_db_sms_get('Empty TP-UD'): success, as expected +DDB NOTICE verify_sms('Empty TP-UD'): match full talloc report on 'null_context' (total 0 bytes in 1 blocks) -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/13630 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: If0b2bb557118c5f0e520a2e6c2816336f6028661 Gerrit-Change-Number: 13630 Gerrit-PatchSet: 5 Gerrit-Owner: fixeria <axilirator at gmail.com> Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin at sysmocom.de> Gerrit-CC: Max <suraev at alumni.ntnu.no> Gerrit-MessageType: merged -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190603/c35f1a31/attachment.htm>