New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid error check due to signed/unsigned confusion #780

Closed
hannob opened this Issue Nov 11, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@hannob
Contributor

hannob commented Nov 11, 2015

In the function _read_text_file_content_without_trailing_newline in the file
modules/afsocket/transport-unix-socket.c
there is a bug in the error check.

The code sets content_len to the return value of _read_text_file_content. That function returns -1 in case of an error. Then there is a check for (content_len <= 0).

The problem: cotent_len is of type gsize (unsigned), while _read_text_file_content returns gssize (signed). If it returns -1 this can't be stored in conten_len and will underflow, thus the error check will be skipped.

This causes some invalid memory reads which I detected with Address Sanitizer.

To fix this content_len must also be gssize (signed), so it can be -1. This patch (also attached) will fix it:

a/modules/afsocket/transport-unix-socket.c  2015-10-27 09:08:53.000000000 +0100
+++ b/modules/afsocket/transport-unix-socket.c  2015-11-11 17:30:21.019247981 +0100
@@ -86,7 +86,7 @@
 static gssize
 _read_text_file_content_without_trailing_newline(const gchar *filename, gchar *buf, gsize buflen)
 {
-  gsize content_len;
+  gssize content_len;

   content_len = _read_text_file_content(filename, buf, buflen);
   if (content_len <= 0)

syslog-ng-signed-underflow.txt

@hannob

This comment has been minimized.

Show comment
Hide comment
@hannob

hannob Nov 11, 2015

Contributor

Some further Info: This affects both syslog-ng 3.7.2 and the current git code.

The Address Sanitizer stack trace looks like this:

==10668==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7f013e792f3e at pc 0x7f013fe1b643 bp 0x7f013e792e90 sp 0x7f013e792e80
READ of size 1 at 0x7f013e792f3e thread T2
    #0 0x7f013fe1b642 in _read_text_file_content_without_trailing_newline modules/afsocket/transport-unix-socket.c:94
    #1 0x7f013fe1b642 in _add_nv_pair_proc_read_unless_unset modules/afsocket/transport-unix-socket.c:109
    #2 0x7f013fe1bc15 in _feed_aux_from_procfs modules/afsocket/transport-unix-socket.c:170
    #3 0x7f013fe1bc15 in _feed_credentials_from_cmsg modules/afsocket/transport-unix-socket.c:195
    #4 0x7f013fe1bc15 in _feed_aux_from_cmsg modules/afsocket/transport-unix-socket.c:206
    #5 0x7f013fe1bc15 in _unix_socket_read modules/afsocket/transport-unix-socket.c:241
    #6 0x7f013fe1bdfc in log_transport_unix_dgram_socket_read_method modules/afsocket/transport-unix-socket.c:252
    #7 0x7f014511a537 in log_transport_read lib/transport/logtransport.h:51
    #8 0x7f014511a537 in log_proto_buffered_server_read_data_method lib/logproto/logproto-buffered-server.c:563
    #9 0x7f014511d4ec in log_proto_buffered_server_read_data lib/logproto/logproto-buffered-server.c:617
    #10 0x7f014511d4ec in log_proto_buffered_server_fetch_into_buffer lib/logproto/logproto-buffered-server.c:652
    #11 0x7f014511da80 in log_proto_buffered_server_fetch lib/logproto/logproto-buffered-server.c:803
    #12 0x7f01450bf183 in log_proto_server_fetch lib/logproto/logproto-server.h:118
    #13 0x7f01450bf183 in log_reader_fetch_log lib/logreader.c:357
    #14 0x7f01450bf778 in log_reader_work_perform lib/logreader.c:86
    #15 0x7f01450d55e0 in _work lib/mainloop-io-worker.c:52
    #16 0x7f0145149600 in iv_work_thread_do_work /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_work.c:118
    #17 0x7f0145146ca9 in iv_run_tasks /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_task.c:48
    #18 0x7f014514d16a in iv_main /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_main_posix.c:106
    #19 0x7f014514908a in iv_work_thread /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_work.c:200
    #20 0x7f014514e5e8 in iv_thread_handler /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_thread_posix.c:142
    #21 0x7f0144ad9433 (/lib64/libpthread.so.0+0x7433)
    #22 0x7f014481e6dc in __clone (/lib64/libc.so.6+0xe96dc)

0x7f013e792f3e is located 132951710424894 bytes inside==10668==AddressSanitizer: while reporting a bug found another one.Ignoring.
Contributor

hannob commented Nov 11, 2015

Some further Info: This affects both syslog-ng 3.7.2 and the current git code.

The Address Sanitizer stack trace looks like this:

==10668==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7f013e792f3e at pc 0x7f013fe1b643 bp 0x7f013e792e90 sp 0x7f013e792e80
READ of size 1 at 0x7f013e792f3e thread T2
    #0 0x7f013fe1b642 in _read_text_file_content_without_trailing_newline modules/afsocket/transport-unix-socket.c:94
    #1 0x7f013fe1b642 in _add_nv_pair_proc_read_unless_unset modules/afsocket/transport-unix-socket.c:109
    #2 0x7f013fe1bc15 in _feed_aux_from_procfs modules/afsocket/transport-unix-socket.c:170
    #3 0x7f013fe1bc15 in _feed_credentials_from_cmsg modules/afsocket/transport-unix-socket.c:195
    #4 0x7f013fe1bc15 in _feed_aux_from_cmsg modules/afsocket/transport-unix-socket.c:206
    #5 0x7f013fe1bc15 in _unix_socket_read modules/afsocket/transport-unix-socket.c:241
    #6 0x7f013fe1bdfc in log_transport_unix_dgram_socket_read_method modules/afsocket/transport-unix-socket.c:252
    #7 0x7f014511a537 in log_transport_read lib/transport/logtransport.h:51
    #8 0x7f014511a537 in log_proto_buffered_server_read_data_method lib/logproto/logproto-buffered-server.c:563
    #9 0x7f014511d4ec in log_proto_buffered_server_read_data lib/logproto/logproto-buffered-server.c:617
    #10 0x7f014511d4ec in log_proto_buffered_server_fetch_into_buffer lib/logproto/logproto-buffered-server.c:652
    #11 0x7f014511da80 in log_proto_buffered_server_fetch lib/logproto/logproto-buffered-server.c:803
    #12 0x7f01450bf183 in log_proto_server_fetch lib/logproto/logproto-server.h:118
    #13 0x7f01450bf183 in log_reader_fetch_log lib/logreader.c:357
    #14 0x7f01450bf778 in log_reader_work_perform lib/logreader.c:86
    #15 0x7f01450d55e0 in _work lib/mainloop-io-worker.c:52
    #16 0x7f0145149600 in iv_work_thread_do_work /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_work.c:118
    #17 0x7f0145146ca9 in iv_run_tasks /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_task.c:48
    #18 0x7f014514d16a in iv_main /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_main_posix.c:106
    #19 0x7f014514908a in iv_work_thread /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_work.c:200
    #20 0x7f014514e5e8 in iv_thread_handler /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_thread_posix.c:142
    #21 0x7f0144ad9433 (/lib64/libpthread.so.0+0x7433)
    #22 0x7f014481e6dc in __clone (/lib64/libc.so.6+0xe96dc)

0x7f013e792f3e is located 132951710424894 bytes inside==10668==AddressSanitizer: while reporting a bug found another one.Ignoring.
@dnsjts

This comment has been minimized.

Show comment
Hide comment
@dnsjts

dnsjts Nov 12, 2015

Contributor

Nice catch, could you open a pull request which contains this patch?

Contributor

dnsjts commented Nov 12, 2015

Nice catch, could you open a pull request which contains this patch?

@hannob

This comment has been minimized.

Show comment
Hide comment
@hannob

hannob Nov 12, 2015

Contributor

pull request sent:
#783

Contributor

hannob commented Nov 12, 2015

pull request sent:
#783

@bazsi bazsi closed this in #783 Nov 12, 2015

@bkil-syslogng

This comment has been minimized.

Show comment
Hide comment
@bkil-syslogng

bkil-syslogng Jan 28, 2016

Contributor

-Werror with either -Wconversion or -Wsign-conversion would have caught this

Contributor

bkil-syslogng commented Jan 28, 2016

-Werror with either -Wconversion or -Wsign-conversion would have caught this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment