[dev.icinga.com #5276] getcgivars() fails to produce proper key/value list causing a potential buffer-overflow condition

[icinga-migration]

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

Created by crfriend on 2013-12-07 14:38:59 +00:00

Assignee: crfriend
Status: Resolved (closed on 2013-12-21 22:50:47 +00:00)
Target Version: 1.11
Last Update: 2014-12-08 09:21:45 +00:00 (in Redmine)

Icinga Version: 1.10.1
OS Version: any

---

This is related to issue #5251 and addresses the root-cause of that issue which resulted in Icinga getting a CVE attached to it for a potential buffer-overrun condition with maliciously-formed query-strings.

At issue is the getcgivars() routine which fails to produce a properly-formed key/value pointer structure in response to Boolean (i.e. "valueless") keys (the presence of the key is the "value). Instead, it produces an array of pointers that merely reflect keys and values in order; it should produce a valid char pointer and a NULL pointer for said Booleans.

Changesets

2013-12-07 20:29:04 +00:00 by crfriend 0cdc0b2

classic ui: Fix incorrect behaviour of getcgivars() -- #5276

This commit attempts to fix an observed behaviour where the getcgivars()
routine was creating a single list rather than a list of key/value pairs.
One manifestation of this was a QUERY_STRING of "jsonoutput=csvoutput"
(which is technically illegal as both of those are booleans) producing
CSV rather than JSON.  This may also help correct the root-cause of the
off-by-one error in #5251.

refs: #5276

2013-12-07 21:31:44 +00:00 by crfriend 2e13fad

classic ui: Fix incorrect behaviour of getcgivars() -- #5276

Fixed a *very* embarrassing oversight having to do with pointers
to the key and value in cmd.cgi.

refs: #5276
whatthecommit: How on Earth did I miss this???

2013-12-08 13:08:23 +00:00 by crfriend 48b5a3a

classic ui: Fix incorrect behaviour of getcgivars() -- #5276

fixed confusion between "key" and "value" in the large key-comparison
if/else if ladder that kept iamges from being generated correctly.

refs: #5276
whatthecommit: Asleep at the switch again, I see...

2013-12-19 17:33:14 +00:00 by ricardo b79fca3

Merge branch 'fix/getcgivars-function-produces-incorrect-results-5276' into next

fixes: #5276

2013-12-19 19:04:09 +00:00 by ricardo f401370

classic-ui: added Changelog entry for #5276

refs: #5276

2013-12-19 19:06:54 +00:00 by ricardo 9e9178e

Merge branch 'fix/getcgivars-function-produces-incorrect-results-5276' into next

fixes: #5276

2013-12-26 23:47:30 +00:00 by crfriend deb1c74

classic ui: Previous fix for issue #5276 broke cmd.cgi -- #5276

The prior fix for issue #5276 broke cmd.cgi and was causing segfaults
and core dumps on Solaris.  The Linux behaviour would likely be
breakage to the "interval" option.  The way that FSF/SGI handle NULL
pointers to reference strings is responsible for the discrepency.

fixes: #5276 (again)
whatthecommit: That was a very log day.  I wonder what'll surface next.

2014-01-03 22:42:46 +00:00 by ricardo 3549874

classic-ui: removed Changelog entry #5276

No adding of Changelog entries for shipped releases.

refs: #5276

2014-01-03 22:47:31 +00:00 by ricardo aca76cf

Merge branch 'fix/getcgivars-function-produces-incorrect-results-5276' into next

fixes: #5276

---

Relations:

• relates #5251

[icinga-migration]

Updated by crfriend on 2013-12-07 15:09:33 +00:00

Note: The fix for this will have downstream impact in any CGI that makes use of the SEARCH_STRING environment variable. "High" priority was asserted as this represents a potential security problem.

Work on this is underway.

[icinga-migration]

Updated by mfriedrich on 2013-12-07 16:50:50 +00:00

• Is Private changed from 0 to 1

[icinga-migration]

Updated by mfriedrich on 2013-12-07 16:53:06 +00:00

• Target Version changed from 1.10.2 to 1.10.3

Marking this as private until a fix is available.

[icinga-migration]

Updated by crfriend on 2013-12-07 17:14:38 +00:00

dnsmichi wrote:

Marking this as private until a fix is available.

Understood.

I anticipate a fix to be available this afternoon (Eastern US time). The change impacts virtually all the CGIs and I'm working through them one by one. Once that's done I'll push my branch and ask for testing.

Initial testing on the "status.cgi" looks very promising, however.

[icinga-migration]

Updated by crfriend on 2013-12-07 20:50:05 +00:00

• Status changed from New to Feedback
• Estimated Hours set to 5

I pushed a commit to this issue a few minutes ago that looks like it answers the root cause of the related off-by-one error by forcing getcgivars() to produce a guaranteed list of pairs where it could produce a single-element list before. The behavior could be reproduced by passing in a Boolean with a value and the result would be nonsense (e.g. a query string of "csvoutput=jsonoutput" would produce JSON data instead of CSV in status.cgi).

In the revised version, Boolean variables (i.e. those which are active by their mere presence) produce a NULL pointer for the value string instead of a pointer to an empty string (""). This seems to have fixed the above problem.

In addition, both the keys and values are now checked for maximum length, and if that length is exceeded the various cgis now bail out with error status. I believe that behavior to be the safest when confronted with badly-formatted input and is related to buffer-overruns.

Most of the files in the cgi hierarchy were affected by this change, and we affected in the process_cgivars() routine.

Initial tests in my environment show good results, but this needs testing on a wider scale, and I'd also like a few other eyes on the changed code to ensure it's sane.

[icinga-migration]

Updated by crfriend on 2013-12-08 21:15:31 +00:00

Well, two follow-on commits had to be made to correct blunders on my part, but it finally looks reasonably sane at this end. Had I known the complete scope heading into this I might not have attempted it! (A quick count shows better than a thousand lines changed. Even though most of those were "mechanical" it's still a big commit.)

Given the scope, I'd really like others to test things to make sure I didn't miss anything I haven't already caught!

[icinga-migration]

Updated by crfriend on 2013-12-16 00:04:19 +00:00

It looks like this one is CVE-2013-7108 per Ricardo's mail to the Icinga-team list.

[icinga-migration]

Updated by ricardo on 2013-12-19 19:22:01 +00:00

• Priority changed from High to Normal
• Target Version changed from 1.10.3 to 1.11
• Is Private changed from 1 to 0

merged to next.

non private as fix is available, was quick fixed with #5251

thanks to Carl for the fix.

[icinga-migration]

Updated by ricardo on 2013-12-21 22:50:47 +00:00

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

Applied in changeset icinga-core:b79fca3c3477696abdfc15f18f3d69b9d6dc3673.

[icinga-migration]

Updated by mfriedrich on 2014-12-08 09:21:45 +00:00

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