Mingle Forum to jedna z najpopularniejszych wtyczek do WordPressa służących do tworzenia forów internetowych – do tej pory pobrało ją ponad 120 tysięcy użytkowników. Ja sam z powodzeniem stosuję ją do obsługi użytkowników mojej wtyczki TradeMatik. Działa świetnie, jednak nie ma rzeczy bezbłędnych – wczoraj wykryto poważną lukę bezpieczeństwa umożliwiającą na wstrzyknięcie dowolnego polecenia do bazy MySQL (tzw. sql injection).
Każdemu się zdarza. Autor wtyczki szybko ją naprawił i dziś każdy może sobie zaktualizować do wersji 1.0.31.1 i korzystać bez obaw. A skoro luka została już naprawiona, zobaczmy co było nie tak. Mam nadzieję, że to będzie dobra lekcja dla nasz wszystkich i sami nigdy nie popełnimy takiego błędu.
Jak było?
Cały problem znajdował się w pliku wpf-insert.php. Plik ten potrafi odbierać dane przesłane za pomocą formularza, a następnie wprowadza przesłane wartości do bazy danych. Dla tych, którzy wiedzą co to jest sql injection brzmi niebezpiecznie, prawda? I tak właśnie było.
Ta linijka:
$edit_post_id = $_POST['edit_post_id'];
Odbierała zmienną 'edit_post_id’ wysłaną metodą POST i przypisywała do zmiennej lokalnej o tej samej nazwie. Co dalej się z nią działo?
//SECURITY FIX NEEDED
$sql = ("UPDATE $mingleforum->t_posts SET text = '$content',
subject = '$subject' WHERE id = $edit_post_id");
$wpdb->query($wpdb->prepare($sql));
…otóż to. Zwróćmy uwagę na 1. linijkę zacytowanego kodu – autor zamieścił tam komentarz informujący, że poniższy kod wymaga poprawy z powodów bezpieczeństwa :) Sam widział, że coś jest nie tak, pozostawił to jednak nienaprawione, a co więcej poinformował wszystkich przeglądających jego kod:
„Jeśli chcielibyście się do mnie włamać, wchodźcie tędy.”
Być może tak właśnie odkrywca błędu natknął się na niego? Kto wie.
Na czym jednak błąd polega?
Na nieprawidłowym użyciu metody prepare(). Rzeczywiście służy ona do zabezpieczenia zapytań sql przed wstrzyknięciem do nich złośliwego kodu, jednak w takiej formie, jak ją użyto jest kompletnie bezużyteczna. prepare() przede wszystkim zamienia apostrofy i cudzysłowy na ich bezpieczną formę (zamiast ’ wstawia \’). I co równie ważnie, a może nawet ważniejsze: dodaje na początku i końcu danych apostrofy, by na pewno traktowane one były jak pojedyncze ciągi znaków.
Tyle, że musi ona wiedzieć, które z nich powinna zmienić. Wyobraźmy sobie, że w zmiennej edit_post_id prześlemy taki ciąg znaków:
12;DROP TABLE wp_posts;
Po podstawieniu tego w zmiennej $sql całe zapytanie będzie wyglądało w całości:
"UPDATE $mingleforum->t_posts SET text = '$content', subject = '$subject' WHERE id =12;DROP TABLE wp_posts;"
Co się stanie? Zostanie wykonany warunek WHERE=12, średnik po nim oznacza, że jest to koniec jednego polecenia SQL i zaczyna się następne. Następne natomiast usuwa tabelę z wszystkimi wpisami na naszym blogu*. A to już nie jest fajne.
Autor wtyczki próbował tutaj zastosować na zmiennej $sql metodę prepare(). Nic to jednak nie da. Przypomnijmy, że bierze ona otrzymaną zmienną, dodaje na początku i końcu cudzysłowy „wstrzykiwanych” wartości i „zabezpiecza” odpowiednie apostrofy i cudzysłowy w środku wartości.
Co jednak znaczy odpowiednie? Skąd metoda ma wiedzieć które elementy w ciągu $sql są wartościami? Nie potrafi tego rozpoznać, więc zostawia całą zmienną taką, jaka ona jest. Wstrzyknięcie więc się uda.
Jak powinno być?
Metoda prepare() powinna zostać użyta jeszcze przed podstawieniem do niej przez nas niebezpiecznych zmiennych. Ma ona postać:
prepare('zapytanie z %s', array('niebezpieczny ciąg'));
Wszelkie niebezpieczne ciągi (pobrane np z formularza wysyłanego przez użytkownika) podajemy jako drugi parametr w tablicy (możemy tak podać od razu wiele ciągów do zabezpieczenia). Zostaną one objęte w cudzysłowy/apostrofy jeśli jeszcze nie są, a apostrofy/cudzysłowy w środku ciągu zostaną zabezpieczone przez backshlash \. Następnie po takim zabezpieczeniu zostaną podstawione do pierwszego parametru w miejsca gdzie występują ciągi „%s”**.
Następnie możemy już na takim ciągu bezpiecznie wykonać metodę query(). Całość w prawidłowej formie więc wygląda tak:
$sql = $wpdb->prepare("UPDATE $mingleforum->t_posts
SET text = '$content',subject = '$subject' WHERE id = %s",
array($edit_post_id));
$wpdb->query($sql);
I teraz jest prawidłowo. Zmienna $sql przed wykonaniem jej przez query() przyjmuje postać (jeśli wstrzykniemy do niej niebezpieczny kod pokazany powyżej):
"UPDATE $mingleforum->t_posts SET text = '$content', subject = '$subject' WHERE id ='12;DROP TABLE wp_posts;'"
Sprawdzany jest warunek czy pole „id” w bazie ma dziwną wartość „12;DROP TABLE wp_posts;”. Takiej wartości w tym polu nie ma na pewno, więc polecenie nie zostanie wykonane.
I wszystko nadal pozostaje bezpieczne.
Jak wspomniałem na początku, wtyczka ma już to naprawione, tak więc wszystkich, którzy jeszcze jej nie zaktualizowali, usilnie namawiam by jednak to zrobili.
A tych, którzy będą teraz próbować wykorzystać tę lukę… cóż – próbujcie :) Na moim TradeMatik jest już naprawiona wersja. A jeśli znajdziecie gdzieś wtyczkę w starszej wersji, dodam, że nie napisałem wszystkiego. W innej części kodu wspomnianego pliku jest wpisane dodatkowe zabezpieczenie. Osoby biegłe z łatwością je obejdą. Jednak wszyscy wannabe script kiddies pozostaną na szczęście tylko wannabe. Ja ich z tego stanu wyprowadzać nie mam zamiaru, więc tylko na tym fragmencie kodu, który potrzebował naprawy się skupiłem ;)
___
*) tak, wiem, że nie zawsze się to uda, ale jest to tylko ilustracja jak przebiega sql injection.
**) lub „%d” dla ciągów, które mają być traktowane jako liczby a nie zmienne typu string.
Komentarze
16 odpowiedzi na „Luka bezpieczeństwa w Mingle Forum”
This should now be fixed as of 1.0.31.2 of Mingle Forum
yes, i know you did it :) i intentionally waited when Your plugin will be bug-free only to describe here how bug looked to avoid same bugs in other plugins in the future (here we talk about plugin and theme development) :)
thank you for first comment!
Great! Thanks for putting it up here. It’s always nice to get the word out :D
[…] na dev.wpzlecenia są już trzy wpisy (raaaaju, aż trzy?), a pod jednym nawet udało mi się wpaść w małą dyskusję z autorem wtyczki, w której lukę opisałem, to […]
Otóż nie, nie każdemu sql-injections się zdarzają. Mnie nie. :-)
a uzywasz w ogole sql? :)
@Daniel, pokaż mi dowolny kod, a znajdę w nim błąd ;)
[…] serwisu Follow us on Twitter 88 śledzących RSS Feed / Mail 441 czytelników Kolejny przykład wykorzystania SQL Injection – luka w Mingle Forum 1 głosuj! Mingle Forum to jedna z najpopularniejszych wtyczek do WordPressa służących […]
Dlaczego %d ma być zamiennikiem dla %s? Pierwsze oznacza liczbę całkowitą, a drugie łańcuch znaków. Oczywiście, że zapytania zadziała w obu przypadkach, ale skoro wstawiamy do zapytania identyfikator, który zawsze będzie liczbą, to bezpieczniej jest traktować go jako liczbę, a nie ciąg znaków. Zwłaszcza, że po stronie aplikacji na 90% będziemy rzutowali ID wzięte z POST-a czy GET-a na inta i sprawdzali czy faktycznie jest tam int przed wykonaniem zapytania. A dlaczego? Odpowiedź jest prosta: po co wykonywać zapytania skoro od razu wiemy, że się nie wykona (nie zwróci wyników)?
wiedziałem, że to pytanie padnie :)
tak, w tym przypadku zdecydowanie powinniśmy użyć %d aby zostało to „obrobione” jako liczba. W przykładzie jednak użyłem %s aby pokazać jak zachowuje się funkcja prepare. co ona dokleja, co zmienia… jakbym użył %d całość zostalaby wycięta, do celów ilustracyjnych zostwiłem więc traktowanie przesłanej zmiennej jako ciągu
Witam,
To ja mam trochę inne pytanie. Czy zabezpieczenie takiego zapytania jak w przykładzie, za pomocą prostego dodania funkcji intval(), w postaci: $edit_post_id = intval($_POST[’edit_post_id’]); byłoby wystarczającym zabezpieczeniem przed sql injection?
w tym wypadku – tak. ale uwazaj, bo opieranie sie na intval moze zmniejszyc Twoa czujnosc. raz zastosujesz, raz nie, bo nie bedzie Ci sie chcialo myslec czy tu ma byc integer czy string… a jesli string to przeciez jeszcze inne dodatkowe, zabezpieczenia…
tymczasem prepare uzywa wlasnie intval i „mysli za Ciebie” o niektorych sprawach. lepiej uzyc prepare, okreslic dla niego czy przekazujemy liczbe czy string i tyle.
popatrz: tu wlasnie przyklad bledu, w ktorym autor wtyczki stracil czujnosc:
http://www.exploit-db.com/exploits/17906/
raz uzyl intval, ale juz w nastepnym parametrze zupelnie o tym zapomnial. i dziura gotowa :)
Jasne, zgadzam się w 100%. Czasami jednak, zwłaszcza w mniejszych projektach (do użytku tylko przeze mnie lub znajomych), pozwalam sobie na drogę na skróty poprzez intvala i tak się zastanawiałem czy to wystarczające zabezpieczenie, głównie właśnie przy zapytaniach używających jakiegoś pola 'id’.
Przyłączę się do tych profesjonalnych komentarzy, ale z pozycji całkowitego laika jeśli chodzi o wtyczkę Mingle Forum. Bardzo ciężko coś znaleźć na jej temat w polskim internecie (nie znam angielskiego). Dotarłem tu ze strony „Muzungu.pl”. Zainstalowałem wtyczkę na swojej stronie, skonfigurowałem i wszystko działa oprócz jednego szczegółu. Jak klikam na opcję „Mark All Read” – „Oznacz wszystkie jako przeczytane” wyświetla mi się błąd:
Warning: Cannot modify header information – headers already sent by (output started at /home/zbymal/domains/portable.info.pl/public_html/wp-content/plugins/all-in-one-seo-pack/aioseop.class.php:221) in /home/zbymal/domains/portable.info.pl/public_html/wp-content/plugins/mingle-forum/wpf.class.php on line 1914
Pomożecie ? Z góry dziękuję.
eh, pytasz o cos co jest najczestszym bledem w wp :) wystarczy przepisac w google 'wordpress headers already sent’ :) nie bede wiec opisywal jak rozwiazac, jedynie podpowiem, ze masz ten blad w jednym z dwoch wskazanych plikow
Dzięki za odpowiedź. Poszukam błędu, bo faktycznie mogłem grzebać, albo otwierać jakieś pliki WordPressa, czy jego pluginów w Notatniku i tu ykwi błąd.
Pozdrawiam zbymal