Skip to content
This repository has been archived by the owner. It is now read-only.

[dev.icinga.com #5434] CVE-2014-1878: Possible segfault in cmd.cgi #1418

Closed
icinga-migration opened this issue Jan 10, 2014 · 10 comments
Closed

[dev.icinga.com #5434] CVE-2014-1878: Possible segfault in cmd.cgi #1418

icinga-migration opened this issue Jan 10, 2014 · 10 comments
Milestone

Comments

@icinga-migration
Copy link
Member

@icinga-migration icinga-migration commented Jan 10, 2014

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

Created by mfrosch on 2014-01-10 11:41:01 +00:00

Assignee: mfriedrich
Status: Resolved (closed on 2014-02-11 15:33:40 +00:00)
Target Version: 1.10.3
Last Update: 2014-12-08 09:21:46 +00:00 (in Redmine)

Icinga Version: 1.10.2
OS Version: any

Here is a potential security issue I've got in private.

Props to the GitHub security team and Dirkjan Bussink <dirkjan.bussink@github.com>.

Please keep it private for now! We also try to share information mit Naemon Team...

So we first encountered this problem as a segfault of our Nagios setup. The problem was with the second file patched in the patch, lib/XXX.c. The problem was that due to a large message coming it, the process would segfault since the message didn’t fit in the buffer that was allocated.

This is due wrong usage of snprintf. In case snprintf doesn’t have enough space in the buffer, it will return the number of needed bytes, not the number of bytes written. This is a fairly classic issue that happens quite often. What it means in this case is that it will read memory outside the buffer, in our case resulting in a segfault.

I’ve audited the nagios core code for more cases and found two more. Especially the case found in cgi/cmd.c worries me since I think it might potentially be used to DoS a Nagios install if it’s publicly accessible. It might even be used to overwrite other memory because the return value from the first call to snprintf is used in the second one. 

This patch should fix it.

diff --git i/cgi/cmd.c w/cgi/cmd.c
index 50504eb..426d944 100644
--- i/cgi/cmd.c
+++ w/cgi/cmd.c
@@ -1903,14 +1903,14 @@ static int cmd_submitf(int id, const char *fmt, ...) {
                return ERROR;

        len = snprintf(cmd, sizeof(cmd) - 1, "[%lu] %s;", time(NULL), command_name);
-       if(len < 0)
+       if(len < 0 || len >= sizeof(cmd))
                return ERROR;

        if(fmt) {
                va_start(ap, fmt);
                len2 = vsnprintf(&cmd[len], sizeof(cmd) - len - 1, fmt, ap);
                va_end(ap);
-               if(len2 < 0)
+               if(len2 < 0 || len2 >= sizeof(cmd) - len)
                        return ERROR;
                }

Changesets

2014-01-10 12:40:09 +00:00 by (unknown) 37fc766

Aggressively check for possible buffer overflows in cmd.cgi

Refs #5434

2014-01-10 12:40:37 +00:00 by (unknown) cf0d20a

Merge branch 'fix/cmd-5434' into support/1.10

Refs #5434

2014-01-10 12:41:35 +00:00 by (unknown) f1c4f36

Aggressively check for possible buffer overflows in cmd.cgi

Refs #5434

2014-01-10 12:42:12 +00:00 by (unknown) eedf4f7

Aggressively check for possible buffer overflows in cmd.cgi

Refs #5434

2014-01-10 12:42:40 +00:00 by (unknown) 10aeffc

Merge branch 'fix/cmd-5434' into next

Refs #5434

2014-01-23 15:15:33 +00:00 by (unknown) 144a0b7

Update Changelog.

Refs #4968
Refs #5434
Refs #4427
Refs #4825
Refs #5263
Refs #5545
@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Jan 10, 2014

Updated by mfrosch on 2014-01-10 11:50:32 +00:00

  • Subject changed from Testissue to Possible segfault in cmd.cgi
  • Description updated
  • Category set to 53
  • Status changed from New to Assigned
  • Assigned to set to ricardo
  • Priority changed from Normal to High
  • Target Version set to 1.10.3
@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Jan 10, 2014

Updated by mfrosch on 2014-01-10 12:32:03 +00:00

This is the patch against current git master:

diff --git a/cgi/cmd.c b/cgi/cmd.c
index e8b3e41..2fa61bc 100644
--- a/cgi/cmd.c
+++ b/cgi/cmd.c
@@ -2691,14 +2691,14 @@ static int cmd_submitf(int id, const char *fmt, ...) {

        len = snprintf(cmd, sizeof(cmd) - 1, "[%lu] %s;", time(NULL), command);

-       if (len < 0)
+       if (len < 0 || len >= sizeof(cmd))
                return ERROR;

        if (fmt) {
                va_start(ap, fmt);
                len2 = vsnprintf(&cmd[len], sizeof(cmd) - len - 1, fmt, ap);
                va_end(ap);
-               if (len2 < 0)
+        if (len2 < 0 || len2 >= sizeof(cmd))
                        return ERROR;
        }
@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Jan 10, 2014

Updated by mfriedrich on 2014-01-10 12:34:13 +00:00

  • Assigned to changed from ricardo to mfriedrich
@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Jan 10, 2014

Updated by crfriend on 2014-01-10 23:42:24 +00:00

Do we believe that the problem is isolated to this one particular case, or do we need to dig in and look for others?

Is it an historical issue that may require back-ports?

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Jan 11, 2014

Updated by mfrosch on 2014-01-11 11:10:13 +00:00

crfriend wrote:

Is it an historical issue that may require back-ports?

At least it's not Icinga specific, so it would require a backport.

Though, the risk might be very small.

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Jan 11, 2014

Updated by crfriend on 2014-01-11 17:55:46 +00:00

lazyfrosch wrote:

At least it's not Icinga specific, so it would require a backport.

Though, the risk might be very small.

In looking at the documentation for the snprintf() call -- at least for Solaris -- it is stated that the default behavior for snprintf() when trying to copy more data than allowed by the second argument is to copy one octet less than that value and then to null-terminate the resulting string. So, I'm not sure where the original reporter's segfault comes into play unless somebody then tries to append to it (which would overflow the space). This behavior may be different in Linux.

Ultimately, we're paying the price for the entire UNIX-like "null-terminated string" concept. If we're to defend against string-related overflows we'll either be paying a huge price in CPU cycles with never-ending strlen() calls or we'll have to come up with another mechanism for storing strings. I recall a number of years ago faced with overflow problems finally abandoning the null-terminated string almost completely and instead used a structure which led off with an int that described the current length of the string in the area immediately following the int which made length-checking very inexpensive indeed for calculating whether a new allocation (and copying the "string-struct" thereto, and freeing the old) was required (along with changing the int to reflect the new length).

In any event, and in digging through the code found that the patch in question addresses the single instance where the return-value from snprintf() is used in the CGIs. Testing for an oversize return-value makes good sense, however, in all cases as it'll catch instances where data gets corrupted (by virtue of the truncation) by the program and probably should get flagged as an error.

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Feb 11, 2014

Updated by mfriedrich on 2014-02-11 10:03:53 +00:00

Will be released as 1.8.6, 1.9.5 and 1.10.3 while the latter includes more bug fixes, 1.9 and 1.8 support branches only get the important fixes.

Will ask for a CVE number once the release is tagged and available for users.

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Feb 11, 2014

Updated by mfriedrich on 2014-02-11 10:30:43 +00:00

  • Is Private changed from 1 to 0
@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Feb 11, 2014

Updated by mfriedrich on 2014-02-11 15:33:40 +00:00

  • Subject changed from Possible segfault in cmd.cgi to CVE-2014-1878: Possible segfault in cmd.cgi
  • Status changed from Assigned to Resolved
  • Done % changed from 0 to 100

Assigned CVE number: CVE-2014-1878
Fixed in: 1.10.3, 1.9.5, 1.8.6

https://www.icinga.org/2014/02/11/bugfix-releases-1-10-3-1-9-5-1-8-6/

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Dec 8, 2014

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

  • Project changed from 19 to Core, Classic UI, IDOUtils
  • Category changed from 53 to Classic UI
  • OS Version set to any
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.