xorl %eax, %eax

CVE-2009-1760: libtorrent Arbitrary File Overwrite

with 5 comments

This bug was discovered by Dimitris Glynos and I have to admit that I’m really sad for seeing Greek people moving to the other side like that. Anyway, libtorrent is a popular open source BitTorrent implementation. This issue affects libtorrent up to 0.14.3 release. Here is the buggy code from that release:

Copyright (c) 2003-2008, Arvid Norberg
All rights reserved.
       ...
        bool extract_single_file(lazy_entry const& dict, file_entry& target
                , std::string const& root_dir)
        {
       ...
               lazy_entry const* p = dict.dict_find("path.utf-8");
       ...
                for (int i = 0, end(p->list_size()); i < end; ++i)
                {
                        if (p->list_at(i)->type() != lazy_entry::string_t)
                                return false;
                        std::string path_element = p->list_at(i)->string_value();
                        if (path_element != "..")
                                target.path /= path_element;
                }
                verify_encoding(target);
                if (target.path.is_complete())
                        return false;
                return true;
        }

This function is used to extract a file from the .torrent file as it is implied by its name. What D. Glynos noticed is that the only check being performed at each entry (p->list_at(i)->string_value()) is just against “..” string. Because of this, an attacker is able access any file of the system using relative paths. D. Glynos gave an example in his advisory. To fix this they added a new function:

+       bool valid_path_element(std::string const& element)
+       {
+               if (element.empty()
+                       || element == "." || element == ".."
+                       || element[0] == '/' || element[0] == '\\'
+                       || element[element.size()-1] == ':')
+                       return false;
+               return true;
+       }
+
+       fs::path sanitize_path(fs::path const& p)
+       {
+               fs::path new_path;
+               for (fs::path::const_iterator i = p.begin(); i != p.end(); ++i)
+               {
+                       if (!valid_path_element(*i)) continue;
+                       new_path /= *i;
+               }
+               TORRENT_ASSERT(!new_path.is_complete());
+               return new_path;
+       }
+

And they updated the vulnerable routine to use it like this:

                        std::string path_element = p->list_at(i)->string_value();
-                       if (path_element != "..")
-                               target.path /= path_element;
+                       target.path /= path_element;
                }
+               target.path = sanitize_path(target.path);
                verify_encoding(target);
+               TORRENT_ASSERT(!target.path.is_complete());
+
                if (target.path.is_complete())

A similar patch was also performed in load_file() function of libtorrent. Once again, shame on us! I am completely disappointed of some Greek coders.

Written by xorl

June 9, 2009 at 12:32

Posted in bugs

5 Responses

Subscribe to comments with RSS.

  1. What is meant by “moving to the other side”? The guy saw a vulnerability and RESPONSIBLY disclosed it. While you may have your views on vulnerability disclosure, once a person (or group of persons) find a vuln, they are free to do as they please with it. Please try to keep this useful resource non-opiniated.
    If I am missing the point, feel free to enlighten me :-)

    thanasisk

    June 9, 2009 at 14:49

  2. Of course he can do whatever he wants with that vulnerability since he discover it. Nevertheless, I am not a supporter of full disclosure and in my opinion it is sad and disappointing to see people that know a couple of things about security moving to the whitehat side. That’s what I meant.
    I usually don’t comment about my positions regarding the authors but this guy is Greek and I feel shame for that.

    xorl

    June 9, 2009 at 15:30

  3. Has ever DGlynos claimed he was a blackhat? Ask around…
    Other people maybe have changed “sides”, DGlynos has not

    thanasisk

    June 9, 2009 at 15:33

  4. You’re right, it was wrong writing: “moving to”, it would be more appropriate to write: “choosing that side”.

    xorl

    June 9, 2009 at 15:37

  5. Agreed with “choosing”, much wiser choice of words. Still, it is his call, as it is your call (or mine or whoever else) to follow a respective lifestyle that affects decisions like that. Let’s not keep beating a dead horse.

    thanasisk

    June 9, 2009 at 15:40


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s