search

chilek / lms

Lan Management System (LMS) public GIT repo
http://lms.org.pl
126 stars 135 forks source link

Błędy typu XSS w różnych formularzach #1910

Open kosaa opened 3 years ago

kosaa commented 3 years ago

Opis błędu osoba z dostepem do edycji klienta moze wykonac dowolny kod po stronie przegladarki osoby, ktora wchodzi na profil tego klienta

Odtworzenie problemu

  1. otworz lub dodaj nowego klienta
  2. wejdz w jego edycje
  3. otworz dowolny adres
  4. w polu opis adresu wpisz:

Oczekiwane zachowanie zostanie wyswietlone okno o tresci 1

Zrzuty ekranu

Zrzut ekranu 2021-01-13 o 23 31 57 Zrzut ekranu 2021-01-13 o 23 32 11

p.s. taki sam efekt maja pola:

@edit /?m=netinfo&id=x - pole description /?m=whproductiteminfo&id=x - pole product item info

wiecej nie sprawdzalem i dlugo tez nie szuklem, ale sadzac po latwosci znalezienia tego typu miejsc, mozna sadzic ze jest ich wiecej

chilek commented 3 years ago

Dzięki za informację. Tak - zapewne wszystkie pola są na to podatne - sprawdzałem przed chwilą dla pól adresu - tam gdzie wpisać się tekst daje się wpisać dowolny element HTML łącznie z <script>...</script>. Od strony szablonów Smarty da się to myślę załatwić opakowaniem prezentacji danych poprzez {htmlspecialchars($value)}. Zobaczymy na co JS pozwala w tym zakresie...

chilek commented 3 years ago

Od strony JS, a konkretnie jQuery jest to banalnie proste - .text().

chilek commented 3 years ago

Przy zapisie pól informacji, notatek, wiadomości i notatki na dokumencie w kartotece klienta również są już usuwane niebezpieczne elementy HTML (na razie <script>; pewnie w przyszłości dojdzie usuwanie innych potencjalnie niebezpiecznych elementów). Trzeba będzie jeszcze poprawić dane w tabeli customers usuwają ze wszystkich zachowanych wspomnianych pól również niezbezpieczne elementy HTML.

kosaa commented 3 years ago

samo <

chilek commented 3 years ago

https://demo.lms.org.pl/?m=whdocuments&doctypefix=100"><

U mnie alert JS nie generuje się, ale za to generuje się błąd SQL przy okazji. @rafalpietraszewicz ?

kosaa commented 3 years ago
Zrzut ekranu 2021-01-14 o 14 46 56
chilek commented 3 years ago

macos bigsur 11.1 (20C69) chrome Wersja 87.0.4280.141 (x86_64)

Mam dokładnie ten sam Chrome, ale oczywiście w Fedora 33!

chilek commented 3 years ago

Ciekawe czy domyślnie nie dopuszcza elementu <b> z prostymi atrybutami, które nie są szkodliwe - trzeba by chyba najpierw przetestować.

Sprawdziłem na demo - przy domyślnej konfiguracji HTMLPurifier nie wycina elementu <b>!

kosaa commented 3 years ago

do kolekcji

@awbnet dopiero teraz zauwazylem, dzieki - na zdrowie ;)

chilek commented 3 years ago

@kosaa poprawione. Będziemy systematycznie, stopniowo analizować formularze poszczególnych typów zasobów i eliminować ewentualne XSS-y.

kosaa commented 3 years ago

oki, to juz nie bede dalej sprawdzac, reszte zostawiam, wazne ze udalo sie wskazac problem

z rzeczy ktore jeszcze przykuwaja uwage to _smarty_function_userpaneltip i zmienna $text

nie odpalalem juz kodu, ale mozliwe ze takie cos by przeszlo, albo cos w tym stylu gdyby troche podlubac: ><iframe src=http://abc.xyz/index.html>

@edit posprawdzalem merge requesty, i nawet te niedawno otwarte maja w sobie nowe podatnosci, wydaje mi sie ze bez jakiegos systemowego rozwiazania sie nie obejdzie

na pierwszy rzut oka kazde bezposrednie drukowanie zmiennych w szablonie powinno byc traktowane z automatu jak luka ktora wprowadza mozliwosc przejecia sesji dowolnego usera i byc zglaszane na etapie pull requesta

taki przyklad w jednym z aktualnych mr: <option value="{$nn.id}" {if $nn.id == $vlaninfo.netnodeid} selected{/if}>{$nn.name} ({$nn.id})</option>

{$nn.name} az kusi zeby cos tam podac, z drugiej strony gdyby wszedzie tego tak paranoicznie pilnowac to tylko zniecheci to osoby ktore przy tym pracuja - ale moze to jedyne rozwiazanie?

chilek commented 3 years ago

oki, to juz nie bede dalej sprawdzac, reszte zostawiam, wazne ze udalo sie wskazac problem

Sprawdzaj, sprawdzaj - dzięki temu jest mobilizacja do poprawek ;-) Kolejna porcja poprawek poszła przed chwilą - dzięki za pozdrowienia dla Fedory w alercie :D

chilek commented 3 years ago

Z moich obserwacji wynika, że:

  1. Gdybyśmy przynajmniej w kontrolkach listy wyborów użytkownika używali zawsze tych samych nazw zmiennych w pętli to byśmy mogli w prosty sposób odszukać grepem wystąpienia właściwości name i rname i w sprawny sposób hurtowo poprawiać.
  2. Listę wyboru użytkowników można byłoby wydzielić do funkcji Smarty i w niej "centralnie" escapeować złośliwy kod HTML/JS.
chilek commented 3 years ago

Od strony szablonów smarty jest bardziej elegancki sposób:

{$dane|escape}
interduo commented 3 years ago
  1. Najważniejsze: Czy userpanel jest podatny na to w jakiś sposób i skrypty wejściowe np. lms-rtparser.php?
  2. Czy Fronend to najlepsze miejsce na Sanityzację?
  3. Czy rozważaliście zastosowanie DOMPurify: https://github.com/cure53/DOMPurify ?
chilek commented 3 years ago

Najważniejsze: Czy userpanel jest podatny na to w jakiś sposób i skrypty wejściowe np. lms-rtparser.php?

Dla treści HTML w mailach otrzymywanych od klientów robimy czyszczenie z niebezpiecznych treści w oparciu o HTMLPurifier. Dla treści tekstowych tego nie robimy, więc niewykluczone, że da się wstrzyknąć złośliwy kod JS. Napisz po prostu do siebie maila tekstowego następującej treści:

<script>alert(1);</script>

i poddaj go "obróbce skrawaniem" w lms-rtparser.php. Ciekawe jakie z tego zgłoszenie wyjdzie.

Czy Frontend to najlepsze miejsce na Sanityzację?

Każde miejsce jest dobre. Można to też robić w backendzie przed zapisem danych do bazy, żeby potem we frontendzie nie robić |escape, ale przecież wcześniej dane nie było eskejpowane, więc mogą stare dane robić problem.

UWAGA! wykradnięcie ciastka sesji o nazwie SID tak naprawdę niewiele daje, bo przy każdym otwieraniu zawartości sprawdzamy czy ciastko pasuje do podstawowych atrybutów klienta www (przeglądarki), np. czy IP jest zgodne z wcześniej przy logowaniu wyznaczonym.

chilek commented 3 years ago

Sprawdziłem przed chwilą rzeczony przykład przez lms-rtparser.php - treść eskejpuje się prawidłowo!

interduo commented 3 years ago

Można to też robić w backendzie przed zapisem danych do bazy, żeby potem we frontendzie nie robić |escape, ale przecież wcześniej dane nie było eskejpowane, więc mogą stare dane robić problem.

Stare dane można wyczyścić za pomocą upgradedb.php. Zobacz ile miejsc jest tych miejsc... na pewno chcesz grepować i edytować wszystkie szablony formularzy i potem pilnować czy czasami nowo dodane pole ma dodane |escape i te wszystkie intval() w plikach modules/*php ? Czy nie uprościło by to znacząco ilości linii kodu w całym projekcie w przyszłych nowych formularzach i w obecnych przebudowywanych?

chilek commented 3 years ago

Stare dane można wyczyścić za pomocą upgradedb.php.

Pisałem już o tym jakie są tego zagrożenia - poczytaj historię tego zgłoszenia.

interduo commented 3 years ago

Tekst może być nieprawidłowym HTML i HTMLPurifier może to efekcie uszkodzić :(

Pokaż przykład.

To może warunkowo jeśli pole zawiera znaczek regexp <*>, dopiero zaprzęgać HTMLPurifiera? Zróbmy test i sprawdźmy na swoich bazach które komórki będą się różnić bo jakoś mam przeczucie, że żadne.

chilek commented 3 years ago

Okazuje się, że informacje o zgłoszeniu wyświetlane w chmurce mają podatność XSS na post przesłany od klienta (przez lms-rtparser.php). Poprawiłem powyższym commitem.

chilek commented 3 years ago

To może warunkowo jeśli pole zawiera znaczek regexp <*>, dopiero zaprzęgać HTMLPurifiera?

Albo nie czytałeś całej historii tego zgłoszenia, albo nie rozumiesz czym jest XSS ;-)

interduo commented 3 years ago

Przy dodawaniu dodatkowych parametrów do eventadd zrobiłem delikatny research na ten temat - jak w innych projektach jest rozwiązana kwestia Sanityzacja danych wejściowych stąd moja propozycja rozwiązania systemowego.

Obecne dane w bazie można w jakiś ładny sposób przeszperać pod kątem XSS.

chilek commented 3 years ago

@interduo to fajnie, że to zrobiłeś - ja również trochę zrobiłem - i to nie parę godzin, a pewnie z parę dni ;-) Mamy już Utils::removeInsecureHtml() po stronie backendu, który w >= 26.x opiera się na HTMLPurifier, a w <= 25.x niestety usuwa tylko elementy <script/>. Więc trochę trzeba sanityzować po stronie backend, a trochę po stronie frontendu. Na pewno wszystkie treści z założenia będące htmlem przy zapisie do bazy trzeba oczyszczać przez removeInsecureHtml().

chilek commented 3 years ago

Przy okazji - edytor wizualny (oparty na TinyMCE) też oczyszcza kod HTML z syfu, ale można go w ogóle pominąć i spreparować złośliwy HTML wysyłając zapytanie POST-em lub GET-em i wtedy i tak po stronie backendu trzeba czyścić wspomnianą już parokrotnie przeze mnie metodą statyczną z Utils.

interduo commented 3 years ago

Powiązane: [Wed Jan 27 14:30:18.626661 2021] [php7:warn] [pid 30522] [client 172.20.3.72:1093] PHP Warning: Directory /var/www/html/lms/vendor/ezyang/htmlpurifier/library/HTMLPurifier/DefinitionCache/Serializer/URI not writable, please chmod to 777 in /var/www/html/lms/vendor/ezyang/htmlpurifier/library/HTMLPurifier/DefinitionCache/Serializer.php on line 297, referer: https://lms/?m=eventedit

Czy ten chmod 777 nie powinien trafić do procedury instalacji lub do skryptu composera?

chilek commented 3 years ago

Trudno powiedzieć - może wystarczy zrobić dodatkowym ustawieniem tej biblioteki wskazanie jej cache-u.

chilek commented 3 years ago

Może to jest to: http://htmlpurifier.org/live/configdoc/plain.html#Cache.SerializerPath ?

chilek commented 3 years ago

Wrzuciłem do master rozwiązanie tego problemu - trzeba potestować. Jeśli sprawdzi się to pójdzie do stable-ów.