Bezpieczny kod Trzy łatwe kawałki

Krzysiek Dróżdż
krzysiek@wpmagus.pl
WPmagus.pl

Dlaczego?

I to nic nie zmienia :(

Błędy są częste

Znacznie częstsze niż myślisz!

Większe wpadki

  • WordPress (XXS, SSRF)
  • Twenty Fifteen (XSS)
  • Gravity Forms (Upload, SQL Injection, XSS)
  • FreshMail Newsletter (SQL Injection)
  • TineMCE Advanced (CSRF)
  • WooCommerce (SQL Injection, XSS)
  • Pods (XSS, CSRF, SQL Injection)
  • RevSlider (Upload, LFD)
  • All In One WP Security (SQL Injection, XSS)
  • WordFence (XSS)
  • WordPress SEO by Yoast (SQL Injection, XSS)
  • itd.

Logi/backupy dla każdego

Problem

Wtyczki umieszczają różne pliki na serwerze.

Każdy, kto zna daną wtyczkę, wie, gdzie tych plików szukać.

Jeśli ścieżkę pliku da się przewidzieć i katalog nie jest zabezpieczony - plik da się pobrać.

Co z tym zrobić?

Każ zgadywać

Gdyby serwer indeksował zawartość katalogu, wstawiamy pusty plik index.html

Zagadka: Dlaczego index.html a nie index.php?

Utrudnij zgadywanie

Zamiast

$file_name = '2015-06-12.log';

użyj

$file_name = '2015-06-12-'. md5('2015-06-12') .'.log';
2015-06-12-cf27319294af17711a8fbf42222f9e76.log

Byłoby OK, ale dalej można zgadnąć! ;)

Utrudnij bardziej

No to spróbuj zgadnąć teraz ;)

$file_name = '2015-06-12-'. md5('2015-06-12#'. $salt) .'.log';
2015-06-12-407db12300f163971d0072c24693ecce.log

… a zasolić możesz np. NONCE_SALT

… lub po prostu użyj wp_hash('2015-06-12')

Zablokuj dostęp

Dodaj jeszcze plik .htaccess blokujący dostęp do plików logów

deny from all
Zawartość pliku .htaccess

Formularz do czegokolwiek

Problem

Brak podstawowych standardów obsługi znaków specjalnych…

… umożliwia wykonanie własnych zapytań do bazy danych.

(W tym jej usunięcie)

Co z tym zrobić?

Teoretycznie proste

Zamiast

$sql = "SELECT ... WHERE ... col = {$_GET['s']}";
$rows = $wpdb->get_results( $sql );

Użyj

$sql = "SELECT ... WHERE col = %s";
$rows = $wpdb->get_results( $wpdb->prepare($sql, $_GET['s']) );

Spójrzmy w kod

ale w praktyce bywa trudniej :(

$sql = "SELECT * FROM {$wpdb->prefix}malicious_files WHERE (1=1) ";
if ( ! current_user_can('manage_options') ) {
    $current_user_id = get_current_user_id();
    $sql .= " AND user_id={$current_user_id} ";
}
if ( isset($_GET['mal_f']) ) {
    $sql .= " AND name='{$_GET['mal_f']}' ";
}
if ( isset($_GET['mal_d']) && $_GET['mal_d'] ) {
    $sql .= " AND description LIKE '{$_GET['mal_d']}%' ";
}

Ale niewiele trudniej

Na szczęście prepare może pracować z częściowymi zapytaniami

$sql = "SELECT * FROM {$wpdb->prefix}malicious_files WHERE (1=1) ";
if ( ! current_user_can('manage_options') ) {
    $current_user_id = get_current_user_id();
    $sql .= $wpdb->prepare(" AND user_id = %d ", $current_user_id);
}
if ( isset($_GET['mal_f']) ) {
    $sql .= $wpdb->prepare(" AND name = %s ", $_GET['mal_f']);
}
...

I jeszcze LIKE

Spójrzmy

$sql .= " AND description LIKE '{$_GET['mal_d']}%' ";

na prosty rozum będzie tak

$sql .= $wpdb->prepare(" AND description LIKE %s ", $_GET['mal_d'].'%');

ale to nie jest dobre, bo powinno być tak

$sql .= $wpdb->prepare(" AND description LIKE %s ",
            $wpdb->esc_like($_GET['mal_d']) .'%');

Wszystko dla wszystkich

Problem

To, że przycisk jest w panelu administracyjnym, nie znaczy, że nikt inny nie wywoła odpowiedniego requestu.

To, że w linku wysyłany jest odpowiedni ID, nie znaczy, że nie można go zmienić.

Wyobraź sobie akcję usuwania rekordu w postaci:
?my_action=delete&id=12

Zagadka: A jeśli użytkownik zmieni ID?

Co z tym zrobić?

Jeśli dana akcja jest zarezerwowana tylko dla danej grupy użytkowników, to za każdym razem należy sprawdzać wszystkie warunki.

Tylko dla właściciela

Co tu jest nie tak?

case 'usun':
    $id = $_REQUEST['malid'];
    $wpdb->query( $wpdb->prepare(
        "DELETE FROM {$wpdb->prefix}malicious_files WHERE id = %d", $id) );
    wp_redirect( site_url('/panel/?mess=deleted') );
    break;

Tak będzie lepiej…

$wpdb->query( $wpdb->prepare(
        "DELETE FROM {$wpdb->prefix}malicious_files WHERE id = %d AND user_id = %d",
        $id, get_current_user_id()
    ) );

A uprawnienia?

Zagadka: A co, jeśli użytkonik przestał być administratorem/redaktorem/itp.?

Ale to też można sprawdzić

if ( current_user_can('edit_posts') ) { ... }

Uwaga! Ale nie tak:

if ( current_user_can('contributor') ) { ... }

Pełna lista capabilities: https://codex.wordpress.org/Roles_and_Capabilities

To nie wszystko…

Zagadka: A co, jeśli użytkownik jest zalogowany, a ktoś podsunie mu odpowiedni link do kliknięcia?

To już zupełnie inna historia… ;)

Dzięki za uwagę!

Krzysiek Dróżdż
krzysiek@wpmagus.pl
WPmagus.pl