Bug #5251

fix Off-by-one memory access in process_cgivars() CVE-2013-7108

Added by ricardo over 1 year ago. Updated 8 months ago.

Status:ResolvedStart date:12/02/2013
Priority:NormalDue date:
Assignee:ricardo% Done:

100%

Category:Classic UI
Target version:Icinga 1.x - 1.10.2
Icinga Version:10.0.1 OS Version:any

Description

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


Related issues

Related to Core, Classic UI, IDOUtils - Bug #5276: getcgivars() fails to produce proper key/value list causi... Resolved 12/07/2013

Associated revisions

Revision 8ac5bc22
Added by ricardo over 1 year ago

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

Revision b651e321
Added by ricardo over 1 year ago

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

Conflicts:
Changelog

Fixes: #5251

Revision 8f1aa9ab
Added by ricardo over 1 year ago

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

Revision 9f831481
Added by ricardo over 1 year ago

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

History

#1 Updated by ricardo over 1 year ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100

#2 Updated by ricardo over 1 year ago

  • Description updated (diff)
  • Private changed from Yes to No

#3 Updated by ricardo over 1 year ago

Attached patches

#4 Updated by dnsmichi 8 months ago

  • Project changed from 19 to Core, Classic UI, IDOUtils
  • Category set to Classic UI
  • OS Version set to any

Also available in: Atom PDF