Incorrect use of readlink() etc. in xbmc/linux/LinuxTimezone.cpp

  Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Post Reply
bxyz Offline
Junior Member
Posts: 5
Joined: Mar 2012
Reputation: 0
Post: #1
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.

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.
find quote
jmarshall Offline
Team-XBMC Developer
Posts: 25,685
Joined: Oct 2003
Reputation: 169
Post: #2
1. is incorrect: It returns a CStdString, so the CStdString constructor will be called, which will perform a copy.
2. looks correct.
3. no idea what the original coder was thinking here. It looks like they're reading from random memory?
4. Assuming p actually returned something sane from the previous strrchr, would this not just be p++? (go to one char before the slash, and look for the next slash, then grab the next character)?

Do you happen to have a github repo so that a pull req can be easily done for further review?

If not, I can do it for you.

Cheers,
Jonathan

Always read the XBMC online-manual, FAQ and search the forum before posting.
Do not e-mail XBMC-Team members directly asking for support. Read/follow the forum rules.
For troubleshooting and bug reporting please make sure you read this first.


[Image: badge.gif]
find quote
bxyz Offline
Junior Member
Posts: 5
Joined: Mar 2012
Reputation: 0
Post: #3
1. I agree, I didn't check the definition of CStdString.
3. I think the original code is safe in the sense that it doesn't corrupt memory, nor will it attempt to read from invalid or random memory locations. It just doesn't make sense because it always returns NULL. First *(timezoneName + rlrc) is zeroed (array overflow but succeeds because of memory alignment) and then strrchr() is called with timezoneName + rlrc which is now an empty string so strrchr() returns NULL.
4. There's no guarantee that the second strrchr() will not return NULL (which is what I mentioned before), or that the return value of the first strrchr() minus 1 is a valid memory location (which I didn't mention before). Yes, for most values of that symbolic link that I'd consider "sane", these problems wouldn't occur, but if the symbolic link contains garbage, the code might misbehave.

None of these issues seem exploitable at first glance (except the application might crash if 3. was fixed and 4. wasn't) but I thought it'd be prudent to fix them. I don't have a github repo, so I'd appreciate if you took it up from here. I use XBMC a lot, I'm glad I could make this very tiny contribution to improve it.
find quote
jmarshall Offline
Team-XBMC Developer
Posts: 25,685
Joined: Oct 2003
Reputation: 169
Post: #4
Thanks - I'll drop it into a branch and get some opinions.

EDIT: here you go: https://github.com/xbmc/xbmc/pull/817

Cheers,
Jonathan

Always read the XBMC online-manual, FAQ and search the forum before posting.
Do not e-mail XBMC-Team members directly asking for support. Read/follow the forum rules.
For troubleshooting and bug reporting please make sure you read this first.


[Image: badge.gif]
(This post was last modified: 2012-03-27 02:38 by jmarshall.)
find quote
bxyz Offline
Junior Member
Posts: 5
Joined: Mar 2012
Reputation: 0
Post: #5
Great, thank you. Smile

In the unlikely case I could be of any more use regarding this issue, please send a private message. Otherwise I'll return to this forum if and when I have something else to share.
find quote