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

Created by ricardo on 2013-12-02 22:37:18 +00:00

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

Icinga Version: 10.0.1
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) Off-by-one memory access
c) 4.9 AV:N/AC:M/Au:S/C:P/I:N/A:P
d) The icinga web gui are susceptible to an "off-by-one read" error, which is resulting from an improper assumption in the handling of user submitted CGI parameters. To prevent buffer overflow attacks agains the web gui, icinga checks for valid string length of user submitted parameters. Any parameter, which is bigger than MAX_INPUT_BUFFER-1 characters long will be discarded. However, by sending a specially crafted cgi parameter, the check routine can be forced to skip the terminating null pointer and read the heap address right after the end of the parameter list. Depending on the memory layout, this may result in a memory curruption condition/crash or reading of sensitive memory locations.
The flaw can be found in multiple locations of the "process_cgivars" function in several files.
Code excerpt and explanation of the issue:

                int process_cgivars(void) {
                                char **variables;
                                int error = FALSE;
                                int x;

1 =>                        variables = getcgivars();
                                to_expand[0] = '\0';

2 =>                        for (x = 0; variables[x] != NULL; x++) {

                                                /* do some basic length checking on the variable identifier to prevent buffer overflows */
3 =>                                        if (strlen(variables[x]) >= MAX_INPUT_BUFFER - 1) {
4 =>                                                        x++;
5 =>                                                        continue;
                                                }

Explanation:

1. The getcgivars reads in all user submitted CGI parameters keys and values, returning a pointer to an array of key/values. The key/values are stored in a consecutive list. For instance, if the user submits the key/value pairs "a=b&c=d", then the resulting list would be:

    variables[0] = "a" (key1)
    variables[1] = "b" (value1)
    variables[2] = "c" (key2)
    variables[3] = "d" (value2)
    variables[4] = NULL
   

2. The list of key/value pairs are processed one by one (not as pairs, but one list item at a time)

3. A check is in place to discard any variable name which is >= MAX_INPUT_BUFFER -1. As the comments imply, this check is performed to discard variable identifier, which is the parameter name (aformentioned key). In the example above, those parameter names would be "a" and "c".

4. If the checked variable length is indeed >= MAX_INPUT_BUFFER -1, this varible should be skipped

5. The loop continues to read the next variable.

The problem of this check is located in line 4: If the examined variable is a variable identifier (above denoted as "key"), the check will skip the corresponding value and continue with the next pair. However, if the variable identifier (key) length is < MAX_INPUT_BUFFER-1, but the corresponding value is >= MAX_INPUT_BUFFER-1 AND the variable is the last in the parameter list, the final NULL terminator of the list will be skipped, and the loop will read past this address. Depending on the heap structure, this address may be controllable and result in an arbitrary read access.

This URL would not trigger the error:
http://icinga/cgi-bin/config.cgi?aaaa\[..2000 times]aaaa=b
This URL would trigger the error:
http://icinga/cgi-bin/config.cgi?b=aaaa\[..2000 times]
e) The problematic "process_cgivars" function can be found in following files:
cgi/avail.c
cgi/cmd.c
cgi/config.c
cgi/extinfo.c
cgi/histogram.c
cgi/notifications.c
cgi/outages.c
cgi/status.c
cgi/statusmap.c
cgi/summary.c
cgi/trends.c

f) One simple mitigation to this problem is to not increment the variable "x" at all (step 4 in the above example). That way both, the parameter name and value, are always checked for valid length.This is done for example in the file cgi/history.c

Attachments

• 0001-classic-ui-fix-Off-by-one-memory-access-in-process_c_1.8.4.patch ricardo - 2013-12-15 22:24:41 +00:00
• 0001-classic-ui-fix-Off-by-one-memory-access-in-process_c_1.9.3.patch ricardo - 2013-12-15 22:24:41 +00:00
• 0001-classic-ui-fix-Off-by-one-memory-access-in-process_c_1.10.1.patch ricardo - 2013-12-15 22:24:41 +00:00

Changesets

2013-12-03 08:47:30 +00:00 by ricardo 8ac5bc2

classic-ui: fix Off-by-one memory access in process_cgivars() #5251

Wrong increment in process_cgivars() got removed.

Thanks to DTAG Group Information Security for the findings.

refs: #5251
whatthecommit: Obligatory placeholder commit message

2013-12-03 15:31:53 +00:00 by ricardo b651e32

Merge branch 'fix/Off-by-one-memory-access-5251' into support/1.10

Conflicts:
	Changelog

Fixes: #5251

2013-12-13 11:47:16 +00:00 by ricardo 8f1aa9a

classic-ui: fix Off-by-one memory access in process_cgivars() #5251

Wrong increment in process_cgivars() got removed.

Thanks to DTAG Group Information Security for the findings.

backport to 1.9

refs: #5251
whatthecommit: Obligatory placeholder commit message

2013-12-13 11:47:56 +00:00 by ricardo 9f83148

classic-ui: fix Off-by-one memory access in process_cgivars() #5251

Wrong increment in process_cgivars() got removed.

Thanks to DTAG Group Information Security for the findings.

backport to 1.8

refs: #5251
whatthecommit: Obligatory placeholder commit message

Relations:

• relates #5251