Monday, February 13, 2012

CVE-2010-3387 and vdrleaktest

I recently helped triage and fix this one in Ubuntu(maverick) and thought of writing up a post about it. First of all, lets see how zero-length directories mess things up.



equinox@VMubuntu:~$ bash
equinox@VMubuntu:~$ export PATH=":"$PATH
equinox@VMubuntu:~$ echo $PATH
:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/opt/libemu/bin/:/opt/libemu/bin/
equinox@VMubuntu:~$ cat > x
echo test
equinox@VMubuntu:~$ chmod 755 x
equinox@VMubuntu:~$ x
test

At this point, I'm assuming that the behavior would be similar in the case of LD_LIBRARY_PATH too. Before we look at the CVE, lets grep around a bit and try to find where the loading of the so file actually takes place.

equinox@VMubuntu:~/cve-stuff/2010-3387/vdr-1.6.0$ egrep -r "dlopen" *
plugin.c:  handle = dlopen(fileName, RTLD_NOW);

Inside plugin.c we notice the following :-
154 cDll::cDll(const char *FileName, const char *Args)
155 {
156   fileName = strdup(FileName);
157   args = Args ? strdup(Args) : NULL;
158   handle = NULL;
159   plugin = NULL;
160 }


190 bool cDll::Load(bool Log)
191 {
192   if (Log)
193      isyslog("loading plugin: %s", fileName);
194   if (handle) {
195      esyslog("attempt to load plugin '%s' twice!", fileName);
196      return false;
197      }
198   handle = dlopen(fileName, RTLD_NOW);

equinox@VMubuntu:~/cve-stuff/2010-3387/vdr-1.6.0$ egrep -r "new cDll" *
plugin.c:  dlls.Add(new cDll(cString::sprintf("%s/%s%s%s%s", directory, LIBVDR_PREFIX, s, SO_INDICATOR, APIVERSION), Args));

As this was something from 2010, I assumed that it would be fixed in debian and searched around a bit and found this. A small part of the patch is as follows :-

diff -u vdr-1.6.0/debian/vdrleaktest vdr-1.6.0/debian/vdrleaktest
--- vdr-1.6.0/debian/vdrleaktest
+++ vdr-1.6.0/debian/vdrleaktest
@@ -65,7 +65,7 @@
 
 /etc/init.d/vdr stop
 
-LANG=C LD_LIBRARY_PATH="/usr/lib/debug:${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" \
+LANG=C LD_LIBRARY_PATH="/usr/lib/debug${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" \
    valgrind --tool=memcheck --leak-check=yes --num-callers=20 \
    --suppressions=/usr/share/vdr/valgrind.supp \
    /usr/bin/vdr-dbg -v $VIDEO_DIR -c $CFG_DIR -L $PLUGIN_DIR  -r $REC_CMD \

I decided to have a look at the maverick version of the same :-

equinox@VMubuntu:~/cve-stuff/2010-3387$ vim $(which vdrleaktest)


and found this instead.

 68 LANG=C LD_LIBRARY_PATH="/usr/lib/debug;$LD_LIBRARY_PATH" \
equinox@VMubuntu:~$ echo "/usr/lib/debug;$LD_LIBRARY_PATH" 
/usr/lib/debug;

While that does seem awkward and buggy it would have been a security vulnerability if you had a ":" instead of a ";" right? I searched around a bit more for confirmation and found this where what I had in mind was being discussed.

However, the fix proposed at the debian ML seemed vulnerable to me.

-LANG=C LD_LIBRARY_PATH="/usr/lib/debug;$LD_LIBRARY_PATH" \
+LANG=C LD_LIBRARY_PATH="/usr/lib/debug:${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" \
    valgrind --tool=memcheck --leak-check=yes --num-callers=20 \
    --suppressions=/usr/share/vdr/valgrind.supp \
    /usr/bin/vdr-dbg -v $VIDEO_DIR -c $CFG_DIR -L $PLUGIN_DIR  -r $REC_CMD \

Because if you did not have LD_LIBRARY_PATH set, you would have 

equinox@VMubuntu:~$ echo "/usr/lib/debug:${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" /usr/lib/debug:
I'm not quite sure if its the right way to fix this bug; if the patch is not vulnerable I wont have to generate a defdiff, we would just need to do a fake sync. The LP bug can be viewed here.
[EDIT] After a discussion at LP I realized that the above mentioned fixed had made its way into the repository, and not a release; in that case I too found it unnecessary to request a CVE assignment. The current patch is as follows :-
+LANG=C LD_LIBRARY_PATH="/usr/lib/debug${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" \

No comments:

Post a Comment