Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#48356 closed feature request (maybelater)

wp_create_nonce(...) and check_ajax_referer(...) fails on the 2nd AJAX call if that is two-action AJAX with AJAX-LOGIN as the first action

Reported by: kestutisit's profile KestutisIT Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch
Focuses: Cc:

Description

So if I generate a new SpecialPluginGlobals['AJAX_SECURITY'] variable with wp_create_nonce(..) and register it as a global JavaScript variable in AssetController.php:

<?php
        wp_localize_script('special-plugin-main', 'SpecialPluginGlobals', array(
            'AJAX_SECURITY' => wp_create_nonce('special-plugin-front-ajax-nonce'),
        ));

And then I Have "Already have an account? Please login" section in the page, where customers does login with it via class method call from SpecialPluginMain.js:

// Dynamic variables
if(typeof SpecialPluginGlobals === "undefined")
{
    // The values here will come from WordPress script localizations,
    // but in case if they wouldn't, we have a backup initializer below
    var SpecialPluginGlobals = {};
}

// NOTE: For object-oriented language experience, this variable name should always match current file name
var FleetManagementMain = {
    globals: FleetManagementGlobals,
    lang: FleetManagementLang,
    vars: FleetManagementVars,

    // <...> Some other JS class methods

    doLogin: function ()
    {
        jQuery.ajax({
            type: 'POST',
            dataType: 'json',
            url: this.globals['LOGIN_AJAX_URL'],
            data:
            {
                'action': 'ajaxlogin', //calls wp_ajax_nopriv_ajaxlogin
                'login_account_id_or_email': jQuery('.special-plugin-search-summary-table form.login-form input.login-account-id-or-email').val(),
                'login_password': jQuery('.special-plugin-search-summary-table form.login-form input.login-password').val(),
                'ajax_security': this.globals['AJAX_SECURITY']
            },
            success: function(data)
            {
                jQuery('.special-plugin-search-summary-table .login-result').text(data.message);
                if (data.loggedIn === true)
                {
                    // NOTE: We set customers dropdown immediately
                    //console.log('Logged in successfully!');

                    // Clear the guest customer lookup section
                    jQuery('.special-plugin-search-summary-table .guest-customer-lookup-section').empty();
                    jQuery('.special-plugin-search-summary-table .login-form').empty();

                    SpecialPluginMain.setCustomersDropdownHTML();
                }
            }
        });
    },

    // <...> Other JS class methods like 'setCustomersDropdownHTML()'
};

Then the system will crash on second Ajax call which tries to pull the list (make an HTML dropdown) of all customers with that Account ID (WordPress user id).

I can sure probably try to send back the new $ajaxNounce after login via JSON, but that is breaking the idea of why why having this AJAX_NOUNCE - to check the source autenthity, and if the script returns it via JS Ajax call then it is a security issue.

And problem here is that, the WordPress changes the NOUNCE after user get's logged in.
This should not be happening first of all, as the purpose of NONCE, first of all is to check the SOURCE AUTHENTICITY that the request really came from the same domain, and is not for password checking - as then it is super easy to break the WordPress security if that is the password mechanism.

So problem is in this method in WordPress core, that it creates different nonces for GUEST and LOGGED_IN user, while there should be a way to create a nonce that would work for both states. As well as the function is self-describing to check the nonce only, and should not do anything else, what would be an unexpected behavior:

<?php
        function wp_create_nonce( $action = -1 ) {
                $user = wp_get_current_user();
                $uid  = (int) $user->ID;
                if ( ! $uid ) {
                        /** This filter is documented in wp-includes/pluggable.php */
                        $uid = apply_filters( 'nonce_user_logged_out', $uid, $action );
                }

                $token = wp_get_session_token();
                $i     = wp_nonce_tick();

                return substr( wp_hash( $i . '|' . $action . '|' . $uid . '|' . $token, 'nonce' ), -12, 10 );
        }

The very bad hack for this issue is demonstrated below (creating a new nonce and sending it via AJAX RESPONSE:

<?php
final class MainController
{
    /**
     * Starts the plug-in main functionality
     */
    public function runOnInit()
    {
         // <..> Other code

         // User hook
         if (!is_user_logged_in())
         {
              // Enable the user with no privileges to run ajax_login() in AJAX
              add_action('wp_ajax_nopriv_ajaxlogin', array($this, 'ajaxLogin'));
         }
    }

    /**
     *
     */
    public function ajaxLogin()
    {
        if($this->canProcess)
        {
            try
            {
                // Assign routing to conf, only if it is not yet assigned
                $conf = $this->conf();
                // Load the language file, only if it is not yet loaded
                $lang = $this->i18n();

                // First check the nonce, if it fails the function will break
                check_ajax_referer('special-plugin-front-ajax-nonce', 'ajax_security');

                // Nonce is checked, get the POST data and sign user on
                $params = array();
                $paramWP_UserIdOrEmail = isset($_POST['login_account_id_or_email']) ? $_POST['login_account_id_or_email'] : 0;
                $userField = is_email($paramWP_UserIdOrEmail) ? 'email' : 'id';
                $params['user_login'] = get_user_by($userField, $paramWP_UserIdOrEmail)->user_login;
                $params['user_password'] = isset($_POST['login_password']) ? $_POST['login_password'] : '';
                $params['remember'] = TRUE;

                $objWPSignOn = wp_signon($params, FALSE);
                if (is_wp_error($objWPSignOn))
                {
                    $jsonParams = array(
                        'loggedIn' => FALSE,
                        'message'=> $lang->escJS('LANG_USER_ACCOUNT_ID_OR_PASSWORD_ERROR_TEXT'),
                    );
                } else
                    {
                    $jsonParams = array(
                        'loggedIn' => TRUE,
                        'message'=> $lang->escJS('LANG_USER_SUCCESSFULLY_LOGGED_IN_TEXT'),
                        'new_ajax_nonce' => wp_create_nonce('special-plugin-front-ajax-nonce'),
                    );
                }

                echo json_encode($jsonParams);
                die();
            } catch (\Exception $e)
            {
                $this->processError(__FUNCTION__, $e->getMessage());
            }
        }
    }

    // <..> Other methods
}

For testing, if needed, the code of all above (a bit older version, and without the AJAX LOGIN part) is available via:
https://wordpress.org/plugins/expandable-faq/
Or
https://github.com/SolidMVC/ExpandableFAQ/tree/master/Controllers

Change History (7)

#1 @KestutisIT
4 years ago

The correct code for demo purpose had to be this:

<?php
var SpecialPluginMain = {
    globals: SpecialPluginGlobals,
    lang: SpecialPluginLang,
    vars: SpecialPluginVars,

#2 follow-up: @ocean90
4 years ago

  • Focuses javascript removed
  • Keywords needs-patch removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Severity changed from major to normal
  • Status changed from new to closed
  • Version 5.2.3 deleted

Hello @KestutisIT, thanks for the report.

Nonces are tied to user sessions and therefore they will be different between non-logged-in users and logged-in users. If you need something to ensure "source authenticity" you have to use your own implementation and don't use nonces.

#3 in reply to: ↑ 2 @KestutisIT
4 years ago

  • Focuses javascript added
  • Keywords 2nd-opinion added
  • Resolution invalid deleted
  • Status changed from closed to reopened

Replying to ocean90:

Hello @KestutisIT, thanks for the report.

Nonces are tied to user sessions and therefore they will be different between non-logged-in users and logged-in users. If you need something to ensure "source authenticity" you have to use your own implementation and don't use nonces.

I cannot quote, as I do not remember on which WordPress page that was written, but as I remember, it was saying, you HAVE to rely on WordPress authentication, sanitation and security methods and features, and do not reinvent your wheel yourself. It is same as including my own jQuery. And probably in the future there would be an issue to pass plugin upload validation to w.org with 'custom made' REST API.
Especially for the nonce, as from 'Operating System for the Open Web' I expect to get nonce as granted to work in modern-web world (where AJAX are included and widely used).
I believe we need to support here an extra parameter for create_nonce that would say what if will return the same nonce for both.
I don't think it is good to create my own nonce method here, as this is not a niche scope - more and more plugins rely on LOGGED-IN state, like BuddyPress, bbPress and that is a large amount of WordPress. And WordPress core says that I MUST CHECK with wp_check_referer. I believe current or later Plugin validator plugin's will reject the plugin if this is not used. I even got to explain when I made sanitation in the model, and not in the controller, and mods of W.org checked in the controller.

So, I'm reopening the ticket, as I strongly believe there has to be done regarding this in WordPress core, as it is first on all related to the basics of security, as as long as WordPress has build-it LOGIN / LOGOUT mechanism, and WP_User is a part of WordPress core, the has to be a way to handle this via WordPress core as well.
I also add '2nd opinion tag'. I also do not understand why you removed Javascript tag. What was the reason for that?

Version 0, edited 4 years ago by KestutisIT (next)

#4 @ocean90
4 years ago

  • Focuses javascript removed
  • Keywords 2nd-opinion removed
  • Resolution set to invalid
  • Status changed from reopened to closed

WordPress already provides you the tools. As you already have demonstrated, sending a new nonce is the way to go, if you want to use nonces. But since you're already using a custom framework I don't see what's wrong with a custom implementation that will fit your needs.

#5 @KestutisIT
4 years ago

  • Focuses javascript added
  • Keywords has-patch added
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version set to 5.2.3

As I asked for 2nd opinion, I please to wait someone else on this topic.
In meanwhile I'm posting A PATCH to WordPress core regarding this.
The problem was introduced in WP 4.0.0 when it started to based it on User Session.
So I made an alternative functions with 'persistent' prefix and based the token on User IP instead. All the rest remained the same. I'm proposing this to add to WordPress core, or to merge it as an 2nd parameter of WordPress core.
In meanwhile I added 'pluggable.php' as the additional file to root folder of plugin to fix WordPress core issues.

The fact that I'm using SolidMVC micro-framework, does not mean that I have to reinvent the security. As if you see where WordPress went with new 'WP_Notify' team they now KNOW THAT, it is bad to do - so I believe the security has to be trusted by WordPress, not by individual methods on every plugin.

I also re-add the 'javascript' keyword because you again did not described what is the reason why you removing it. It is directly related to Javascript. And I even wrote a patch for WordPress don't below. You just need to integrate it. That's it.

The copy of <WP_PLUGIN>/pluggable.php is below:

<?php

if ( ! function_exists( 'wp_get_persistent_token' ) ) :
    /**
     * Function to get the client IP address
     * Purpose - an alternative to wp_get_session_token()
     *
     * @see wp_get_session_token
     * @return string
     */
    function wp_get_persistent_token() {
        if (isset($_SERVER['HTTP_CLIENT_IP'])) {
            $ip_address = $_SERVER['HTTP_CLIENT_IP'];
        } else if(isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
            $ip_address = $_SERVER['HTTP_X_FORWARDED_FOR'];
        } else if(isset($_SERVER['HTTP_X_FORWARDED'])) {
            $ip_address = $_SERVER['HTTP_X_FORWARDED'];
        } else if(isset($_SERVER['HTTP_FORWARDED_FOR'])) {
            $ip_address = $_SERVER['HTTP_FORWARDED_FOR'];
        } else if(isset($_SERVER['HTTP_FORWARDED'])) {
            $ip_address = $_SERVER['HTTP_FORWARDED'];
        } else if(isset($_SERVER['REMOTE_ADDR'])) {
            $ip_address = $_SERVER['REMOTE_ADDR'];
        } else {
            $ip_address = 'UNKNOWN';
        }
        $token = crc32($ip_address);

        return $token;
    }
endif;

if ( ! function_exists( 'check_persistent_ajax_referer' ) ) :
    /**
     * Verifies the Ajax request to prevent processing requests external of the blog.
     *
     * @since 2.0.3
     *
     * @param int|string   $action    Action nonce.
     * @param false|string $query_arg Optional. Key to check for the nonce in `$_REQUEST` (since 2.5). If false,
     *                                `$_REQUEST` values will be evaluated for '_ajax_nonce', and '_wpnonce'
     *                                (in that order). Default false.
     * @param bool         $die       Optional. Whether to die early when the nonce cannot be verified.
     *                                Default true.
     * @return false|int False if the nonce is invalid, 1 if the nonce is valid and generated between
     *                   0-12 hours ago, 2 if the nonce is valid and generated between 12-24 hours ago.
     */
    function check_persistent_ajax_referer( $action = -1, $query_arg = false, $die = true ) {
        if ( -1 == $action ) {
            _doing_it_wrong( __FUNCTION__, __( 'You should specify a nonce action to be verified by using the first parameter.' ), '4.7' );
        }

        $nonce = '';

        if ( $query_arg && isset( $_REQUEST[ $query_arg ] ) ) {
            $nonce = $_REQUEST[ $query_arg ];
        } elseif ( isset( $_REQUEST['_ajax_nonce'] ) ) {
            $nonce = $_REQUEST['_ajax_nonce'];
        } elseif ( isset( $_REQUEST['_wpnonce'] ) ) {
            $nonce = $_REQUEST['_wpnonce'];
        }

        $result = wp_verify_persistent_nonce( $nonce, $action );

        /**
         * Fires once the Ajax request has been validated or not.
         *
         * @since 2.1.0
         *
         * @param string    $action The Ajax nonce action.
         * @param false|int $result False if the nonce is invalid, 1 if the nonce is valid and generated between
         *                          0-12 hours ago, 2 if the nonce is valid and generated between 12-24 hours ago.
         */
        do_action( 'check_persistent_ajax_referer', $action, $result );

        if ( $die && false === $result ) {
            if ( wp_doing_ajax() ) {
                wp_die( -1, 403 );
            } else {
                die( '-1' );
            }
        }

        return $result;
    }
endif;

if ( ! function_exists( 'wp_verify_persistent_nonce' ) ) :
    /**
     * Verify that correct nonce was used with time limit.
     *
     * The user is given an amount of time to use the token, so therefore, since the
     * USER_IP and $action remain the same, the independent variable is the time.
     *
     * @note For persistent nonce, user is not involved
     *
     * @since 2.0.3
     *
     * @param string     $nonce  Nonce that was used in the form to verify
     * @param string|int $action Should give context to what is taking place and be the same when nonce was created.
     * @return false|int False if the nonce is invalid, 1 if the nonce is valid and generated between
     *                   0-12 hours ago, 2 if the nonce is valid and generated between 12-24 hours ago.
     */
    function wp_verify_persistent_nonce( $nonce, $action = -1 ) {
        $nonce = (string) $nonce;

        if ( empty( $nonce ) ) {
            return false;
        }

        $token = wp_get_persistent_token();
        $i     = wp_nonce_tick();

        // Nonce generated 0-12 hours ago
        $expected = substr( wp_hash( $i . '|' . $action . '|' . $token, 'nonce' ), -12, 10 );
        if ( hash_equals( $expected, $nonce ) ) {
            return 1;
        }

        // Nonce generated 12-24 hours ago
        $expected = substr( wp_hash( ( $i - 1 ) . '|' . $action . '|' . $token, 'nonce' ), -12, 10 );
        if ( hash_equals( $expected, $nonce ) ) {
            return 2;
        }

        /**
         * Fires when persistent nonce verification fails.
         * @note $uid is not included here
         *
         * @since 4.4.0
         *
         * @param string     $nonce  The invalid nonce.
         * @param string|int $action The nonce action.
         * @param WP_User    $user   The current user object.
         * @param string     $token  The user's session token.
         */
        do_action( 'wp_verify_persistent_nonce_failed', $nonce, $action, $token );

        // Invalid nonce
        return false;
    }
endif;

if ( ! function_exists( 'wp_create_persistent_nonce' ) ) :
    /**
     * Creates a cryptographic token tied to a specific action, user, user session,
     * and window of time.
     *
     * @note For persistent nonce, user is not involved
     *
     * @since 2.0.3
     * @since 4.0.0 Session tokens were integrated with nonce creation
     *
     * @param string|int $action Scalar value to add context to the nonce.
     * @return string The token.
     */
    function wp_create_persistent_nonce( $action = -1 ) {
        $token = wp_get_persistent_token();
        $i     = wp_nonce_tick();

        return substr( wp_hash( $i . '|' . $action . '|' . $token, 'nonce' ), -12, 10 );
    }
endif;

Last edited 4 years ago by KestutisIT (previous) (diff)

#6 @ocean90
4 years ago

  • Focuses javascript removed
  • Resolution set to maybelater
  • Status changed from reopened to closed
  • Type changed from defect (bug) to feature request
  • Version 5.2.3 deleted

Thanks for the showing how an implementation of such a feature can look like. Though, I'm going to close this as maybelater for now. It can be reconsidered once core has a need for this or the demand for that functionality is high enough that warrants it.

#7 @SergeyBiryukov
4 years ago

  • Component changed from General to Users
Note: See TracTickets for help on using tickets.