From 6748ce8a947e38e5b411ca6729967aa47b3fae9f Mon Sep 17 00:00:00 2001 From: ozh Date: Sun, 5 Feb 2017 16:04:50 +0100 Subject: [PATCH] Introduce yourls_sanitize_url_safe() Remove anti CRLF checks from yourls_sanitize_url() and yourls_esc_url() because some legit URLs have %0A or %0D We're just stripping those, using new function yourls_sanitize_url_safe(), for internal redirection when target location isn't hardcoded. --- admin/index.php | 2 +- includes/functions-auth.php | 2 +- includes/functions-formatting.php | 35 ++++++++++++++++++++++++++++--- includes/functions.php | 2 +- 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/admin/index.php b/admin/index.php index 2c06474..ed798b9 100644 --- a/admin/index.php +++ b/admin/index.php @@ -52,7 +52,7 @@ if( !empty( $search ) && !empty( $_GET['search_in'] ) ) { break; } $search_sentence = yourls_s( 'Searching for %1$s in %2$s.', yourls_esc_html( $search ), yourls_esc_html( $search_in_text ) ); - $search_url = yourls_sanitize_url( "&search=$search&search_in=$search_in" ); + $search_url = yourls_sanitize_url_safe( "&search=$search&search_in=$search_in" ); $search_text = $search; $search = str_replace( '*', '%', '*' . yourls_escape( $search ) . '*' ); if( $search_in == 'all' ) { diff --git a/includes/functions-auth.php b/includes/functions-auth.php index a410845..744edd0 100644 --- a/includes/functions-auth.php +++ b/includes/functions-auth.php @@ -81,7 +81,7 @@ function yourls_is_valid_user() { // Login form : redirect to requested URL to avoid re-submitting the login form on page reload if( isset( $_REQUEST['username'] ) && isset( $_REQUEST['password'] ) && isset( $_SERVER['REQUEST_URI'] ) ) { $url = $_SERVER['REQUEST_URI']; - yourls_redirect( $url ); + yourls_redirect( yourls_sanitize_url_safe($url) ); } } diff --git a/includes/functions-formatting.php b/includes/functions-formatting.php index 3b68e07..7d38510 100644 --- a/includes/functions-formatting.php +++ b/includes/functions-formatting.php @@ -92,7 +92,9 @@ function yourls_sanitize_title( $unsafe_title, $fallback = '' ) { } /** - * A few sanity checks on the URL. Used for redirection or DB. For display purpose, see yourls_esc_url() + * A few sanity checks on the URL. Used for redirection or DB. + * For redirection when you don't trust the URL ($_SERVER variable, query string), see yourls_sanitize_url_safe() + * For display purpose, see yourls_esc_url() * * @param string $unsafe_url unsafe URL * @param array $protocols Optional allowed protocols, default to global $yourls_allowedprotocols @@ -103,6 +105,25 @@ function yourls_sanitize_url( $unsafe_url, $protocols = array() ) { return yourls_apply_filter( 'sanitize_url', $url, $unsafe_url ); } +/** + * A few sanity checks on the URL, including CRLF. Used for redirection when URL to be sanitized is critical and cannot be trusted. + * + * Use when critical URL comes from user input or environment variable. In such a case, this function will sanitize + * it like yourls_sanitize_url() but will also remove %0A and %0D to prevent CRLF injection. + * Still, some legit URLs contain %0A or %0D (see issue 2056, and for extra fun 1694, 1707, 2030, and maybe others) + * so we're not using this function unless it's used for internal redirection when the target location isn't + * hardcoded, to avoid XSS via CRLF + * + * @since 1.7.2 + * @param string $unsafe_url unsafe URL + * @param array $protocols Optional allowed protocols, default to global $yourls_allowedprotocols + * @return string Safe URL + */ +function yourls_sanitize_url_safe( $unsafe_url, $protocols = array() ) { + $url = yourls_esc_url( $unsafe_url, 'safe', $protocols ); + return yourls_apply_filter( 'sanitize_url_safe', $url, $unsafe_url ); +} + /** * Perform a replacement while a string is found, eg $subject = '%0%0%0DDD', $search ='%0D' -> $result ='' * @@ -479,6 +500,9 @@ function yourls_esc_attr( $text ) { * A number of characters are removed from the URL. If the URL is for displaying * (the default behaviour) ampersands are also replaced. * + * This function by default "escapes" URL for display purpose (param $context = 'display') but can + * take extra steps in URL sanitization. See yourls_sanitize_url() and yourls_sanitize_url_safe() + * * @since 1.6 * * @param string $url The URL to be cleaned. @@ -512,8 +536,13 @@ function yourls_esc_url( $url, $context = 'display', $protocols = array() ) { $url = preg_replace( '|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\[\]\\x80-\\xff]|i', '', $url ); // Previous regexp in YOURLS was '|[^a-z0-9-~+_.?\[\]\^#=!&;,/:%@$\|*`\'<>"()\\x80-\\xff\{\}]|i' // TODO: check if that was it too destructive - $strip = array( '%0d', '%0a', '%0D', '%0A' ); - $url = yourls_deep_replace( $strip, $url ); + + // If $context is 'safe', an extra step is taken to make sure no CRLF injection is possible. + // To be used when $url can be forged by evil user (eg it's from a $_SERVER variable, a query string, etc..) + if ( 'safe' == $context ) { + $strip = array( '%0d', '%0a', '%0D', '%0A' ); + $url = yourls_deep_replace( $strip, $url ); + } // Replace ampersands and single quotes only when displaying. if ( 'display' == $context ) { diff --git a/includes/functions.php b/includes/functions.php index a2b04be..e416361 100644 --- a/includes/functions.php +++ b/includes/functions.php @@ -832,7 +832,7 @@ function yourls_log_redirect( $keyword ) { $now = date( 'Y-m-d H:i:s' ); $keyword = yourls_escape( yourls_sanitize_string( $keyword ) ); - $referrer = ( isset( $_SERVER['HTTP_REFERER'] ) ? yourls_escape( yourls_sanitize_url( $_SERVER['HTTP_REFERER'] ) ) : 'direct' ); + $referrer = ( isset( $_SERVER['HTTP_REFERER'] ) ? yourls_escape( yourls_sanitize_url_safe( $_SERVER['HTTP_REFERER'] ) ) : 'direct' ); $ua = yourls_escape( yourls_get_user_agent() ); $ip = yourls_escape( yourls_get_IP() ); $location = yourls_escape( yourls_geo_ip_to_countrycode( $ip ) ); -- 2.42.0