Closed icinga-migration closed 10 years ago
Updated by mfrosch on 2014-01-10 11:50:32 +00:00
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;
}
Updated by mfriedrich on 2014-01-10 12:34:13 +00:00
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?
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.
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.
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.
Updated by mfriedrich on 2014-02-11 10:30:43 +00:00
Updated by mfriedrich on 2014-02-11 15:33:40 +00:00
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/
Updated by mfriedrich on 2014-12-08 09:21:47 +00:00
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)
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...
This patch should fix it.
Changesets
2014-01-10 12:40:09 +00:00 by (unknown) 37fc7668f35945d8064a1151c6235b0df333300a
2014-01-10 12:40:37 +00:00 by (unknown) cf0d20ab882cfa1e425c7c229676f2a284401460
2014-01-10 12:41:35 +00:00 by (unknown) f1c4f364d70117ea5d72117bf4fe0cd1b5f8b580
2014-01-10 12:42:12 +00:00 by (unknown) eedf4f7d88cdc50843572224eb38a2f5c78a2dc5
2014-01-10 12:42:40 +00:00 by (unknown) 10aeffc786fb7e192d9264124802bfa8c880a1f1
2014-01-23 15:15:33 +00:00 by (unknown) 144a0b7557ce208421c34113e376169315d8b440