2012-03-27, 00:33
I noticed some weird code in xbmc/linux/LinuxTimezone.cpp: CLinuxTimezone::GetOSConfiguredTimezone().
1. The function returns a pointer to a variable on the stack: timezoneName.
2. It tries to null terminate a string returned by readlink() but it doesn't take into account that readlink() may not leave space for the null character. This can cause an array overflow if the value of the symbolic link is sufficiently long (255 bytes). (Fortunately the stack is unlikely to get corrupted because the space allocated for the array is usually rounded up to obey memory alignment rules.)
3. strrchr() is used incorrectly: pointers to the end of null terminated strings are passed to it instead of pointers to the beginning of strings.
4. A pointer, that may be NULL, is incremented (the return value of strrchr()).
I've created a small patch (see below) to illustrate how these problems could be fixed. I don't know how the "Slackware approach" handles the /etc/localtime-copied-from symbolic link, the new code is safe but I might have guessed the intention of the original coder incorrectly.
The patch applies cleanly to xbmc-11.0 as well as to the latest git sources.
1. The function returns a pointer to a variable on the stack: timezoneName.
2. It tries to null terminate a string returned by readlink() but it doesn't take into account that readlink() may not leave space for the null character. This can cause an array overflow if the value of the symbolic link is sufficiently long (255 bytes). (Fortunately the stack is unlikely to get corrupted because the space allocated for the array is usually rounded up to obey memory alignment rules.)
3. strrchr() is used incorrectly: pointers to the end of null terminated strings are passed to it instead of pointers to the beginning of strings.
4. A pointer, that may be NULL, is incremented (the return value of strrchr()).
I've created a small patch (see below) to illustrate how these problems could be fixed. I don't know how the "Slackware approach" handles the /etc/localtime-copied-from symbolic link, the new code is safe but I might have guessed the intention of the original coder incorrectly.
Code:
diff -Naur xbmc-11.0/xbmc/linux/LinuxTimezone.cpp xbmc-11.0_modified/xbmc/linux/LinuxTimezone.cpp
--- xbmc-11.0/xbmc/linux/LinuxTimezone.cpp 2012-03-21 22:07:50.000000000 +0000
+++ xbmc-11.0_modified/xbmc/linux/LinuxTimezone.cpp 2012-03-26 22:21:05.196013296 +0000
@@ -165,18 +165,28 @@
CStdString CLinuxTimezone::GetOSConfiguredTimezone()
{
- char timezoneName[255];
+ // Shouldn't this be dynamically allocated?
+ static char timezoneName[256];
// try Slackware approach first
+ // readlink() doesn't null terminate the string, hence the sizeof(...)-1
ssize_t rlrc = readlink("/etc/localtime-copied-from"
- , timezoneName, sizeof(timezoneName));
+ , timezoneName, sizeof(timezoneName)-1);
if (rlrc != -1)
{
+ // this is safe because rlrc <= sizeof(timezoneName)-1
timezoneName[rlrc] = '\0';
- const char* p = strrchr(timezoneName+rlrc,'/');
+ char* p = strrchr(timezoneName,'/');
if (p)
- p = strrchr(p-1,'/')+1;
+ {
+ char* q = p;
+ *q = 0;
+ p = strrchr(timezoneName,'/');
+ *q = '/';
+ if (p)
+ p++;
+ }
return p;
}The patch applies cleanly to xbmc-11.0 as well as to the latest git sources.
