This issue has been migrated from Redmine: https://dev.icinga.com/issues/5250

Created by ricardo on 2013-12-02 20:38:21 +00:00

Assignee: ricardo
Status: Resolved (closed on 2013-12-03 15:42:13 +00:00)
Target Version: 1.10.2
Last Update: 2014-12-08 09:21:44 +00:00 (in Redmine)

Icinga Version: 1.10.0
OS Version: any

Info from DTAG Group Information Security:

a) application
b) problem
c) CVSS
d) detailed description
e) reference / position in the source code
f) recommendation

a) Icinga 1.9.1
b) Buffer Overflow
c) 8.5 AV:N/AC:M/Au:S/C:C/I:C/A:C
d) The icinga web gui is susceptible to several buffer overflow flaws, which can be triggered as a logged on user. A remote attacker may utilize a CSRF (cross site request forgery) attack vector against a logged in user to exploit this flaw remotely. Depending on the target system, this may result in code execution and eventually full compromise of the icinga server.

All occurences of this flaw are a result of incorrect string handling of CGI parameters. Although the flaw can be found in several locations of the code, it follows the same pattern, which should be discussed in detail. All susceptible code locations look like the following (annotations on the left side, explained below)

                void display_nav_table(time_t ts_start, time_t ts_end) {
                    char *temp_buffer;
1 =>            char url[MAX_INPUT_BUFFER] = "";
                    char stripped_query_string[MAX_INPUT_BUFFER] = "";
                    char date_time[MAX_INPUT_BUFFER];
                    struct tm *t;
                    time_t ts_midnight = 0L;
                    time_t current_time = 0L;

                    /* define base url */
                    switch (CGI_ID) {
                    case HISTORY_CGI_ID:
2 =>                strcat(url, HISTORY_CGI);
                        break;
                    case NOTIFICATIONS_CGI_ID:
2 =>        strcat(url, NOTIFICATIONS_CGI);
                        break;
                    case SHOWLOG_CGI_ID:
2 =>                strcat(url, SHOWLOG_CGI);
                        break;
                    default:
2 =>                strcat(url, "NO_URL_DEFINED");
                        break;
                    }

                    /* get url options but filter out "ts_end", "ts_start" and "start" */
                    if (getenv("QUERY_STRING") != NULL && strcmp(getenv("QUERY_STRING"), "")) {
3 =>                if(strlen(getenv("QUERY_STRING")) > MAX_INPUT_BUFFER) {
                            printf("display_nav_table(): Could not allocate memory for stripped_query_string\n");
                            exit(1);
                        }
4 =>                strcpy(stripped_query_string, getenv("QUERY_STRING"));
                        strip_html_brackets(stripped_query_string);

5 =>                for (temp_buffer = my_strtok(stripped_query_string, "&"); temp_buffer != NULL; temp_buffer = my_strtok(NULL, "&")) {
                            if (strncmp(temp_buffer, "ts_start=", 9) != 0 && strncmp(temp_buffer, "ts_end=", 6) != 0 && strncmp(temp_buffer, "start=", 6) != 0) {
                                if (strstr(url, "?"))
                                    strcat(url, "&");
                                else
                                    strcat(url, "?");
6 =>                        strcat(url, temp_buffer);
                            }
                        }
                    }

1. The function defines an empty char array of length MAX_INPUT_BUFFER
2. Depending of the value of CGI_ID (which is the filename of the cgi calling this function), a string is concatenated to the empty array. At this point, the string "url" has a length depending on the CGI_ID
3. Then the user submitted query string checked for length < MAX_INPUT_BUFFER. This is done to prevent buffer overflows in a copy routine which is called at a later point. However, this check is not correct, as it does not take the length of the already copied string into account, which was performed in step 2
4. The user submitted query string is copied into the char array "stripped_query_string". This is done to sanitize the input values for displaying
5. From the user submitted query string, all relevant parameters are extracted using a modified strtok function
6. At this point, the buffer overflow happens as a result of a miscalculation of the availiable space in the "url" array. The strcat copies the user submitted valued right behind the already copied string from step 2. The resulting string may exceed the size of the allocated space of url (MAX_INPUT_BUFFER), which eventually results in a buffer overflow.

This memory corruption condition may result in controlling the program flow by modifying the stack content, which may finally lead to arbitrary code execution on the icinga server.

e) The flaw can be found in multiple locations of the code base. Following files/functions are affected:
cgi/cgiutils.c: display_nav_table
cgi/cgiutils.c: page_limit_selector
cgi/cgiutils.c: print_export_link
cgi/cgiutils.c: page_num_selector
cgi/status.c: status_page_num_selector
cgi/config.c display_command_expansion

f) Unsafe API calls such as "strcat" should be avoided, if possible. Free space of the destination buffer should be calculated correctly to prevent buffer overflows. In the above example the length of the string, which has been copied into the "url" variable, should be taken into account before executing the strcat API call.

Attachments

• 0001-classic-ui-fix-possible-buffer-overflows-5250_1.8.4.patch ricardo - 2013-12-15 22:17:13 +00:00
• 0001-classic-ui-fix-possible-buffer-overflows-5250_1.9.3.patch ricardo - 2013-12-15 22:17:13 +00:00
• 0001-classic-ui-fix-possible-buffer-overflows-5250_1.10.1.patch ricardo - 2013-12-15 22:17:13 +00:00

Changesets

2013-12-03 08:45:04 +00:00 by ricardo 3c002b7

classic-ui: fix possible buffer overflows #5250

Add checks to functions using strcat. Now the legth of the resulting
string gets calculated and checked if it would exceed the max length
for this char array.

Thanks to DTAG Group Information Security for the findings.

refs: #5250
whatthecommit: fixed my stupidness ...

2013-12-03 15:30:35 +00:00 by ricardo aaa6ef0

Merge branch 'fix/buffer-overflows-5250' into support/1.10

Conflicts:
	Changelog

Fixes: #5250

2013-12-13 11:29:55 +00:00 by ricardo eaecd28

classic-ui: fix possible buffer overflows #5250

Add checks to functions using strcat. Now the legth of the resulting
string gets calculated and checked if it would exceed the max length
for this char array.

Thanks to DTAG Group Information Security for the findings.

backported to 1.9

refs: #5250
whatthecommit: fixed my stupidness ...

2013-12-13 11:41:10 +00:00 by ricardo cdb7890

classic-ui: fix possible buffer overflows #5250

Add checks to functions using strcat. Now the legth of the resulting
string gets calculated and checked if it would exceed the max length
for this char array.

Thanks to DTAG Group Information Security for the findings.

backported to 1.8

refs: #5250
whatthecommit: fixed my stupidness ...