Bezpieczny kod Trzy łatwe kawałki

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

Większe wpadki

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ć.

Nie, no to się nie zdarza...

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

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

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

Utrudnij bardziej

$file_name = '2015-06-12-'. md5('2015-06-12#'. $salt) .'.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

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

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

$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

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

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

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

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

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;

$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.?

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

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