phpBB 3.2.6 FORM_TOKEN on login

General discussion of development ideas and the approaches taken in the 3.x branch of phpBB. The current feature release of phpBB 3 is 3.3/Proteus.
Forum rules
Please do not post support questions regarding installing, updating, or upgrading phpBB 3.3.x. If you need support for phpBB 3.3.x please visit the 3.3.x Support Forum on phpbb.com.

If you have questions regarding writing extensions please post in Extension Writers Discussion to receive proper guidance from our staff and community.
Post Reply
User avatar
EA117
Registered User
Posts: 21
Joined: Tue Apr 30, 2019 12:54 pm

phpBB 3.2.6 FORM_TOKEN on login

Post by EA117 »

I see where https://tracker.phpbb.com/browse/PHPBB3-16036 identified that there are additional login forms which still needed the new FORM_TOKEN implementation. If there is to be a second push at getting this FORM_TOKEN implementation out there (meaning a phpBB 3.2.6.1 or whatever), is the fact that "the risk is breaking the ability to login" high enough to warrant some additional "temporary transition-specific mitigation" here? Ideas which come to mind:
  1. Maybe make the new S_FORM_TOKEN_LOGIN template change "implemented, but not actually required yet." i.e. Unlike normal uses of check_form_token(), make the login_box() usage only validate the FORM_TOKEN "if those fields actually exist in the form." And don't make the S_FORM_TOKEN_LOGIN change strictly required by login_box() until 3.2.7 or later, after there has been time for style authors to consume the 3.2.6 changes.
    • The argument could probably also be made that if we're going to neuter the check_form_token() like that during login_box(), the check_form_token() call could just be completely commented out for now, and make it "not required" even for a compatible 3.2.6 style. And simply wait until 3.2.7 or later to finally un-comment the check_form_token() usage in login_box(), after there has been time for style authors to consume the 3.2.6 changes that were "present but not being used yet."
      • Make the new S_FORM_TOKEN_LOGIN template change in 3.2.6, but then have the login forms inject the FORM_TOKEN fields by overloading the existing S_LOGIN_REDIRECT variable that phpBB styles already implement. i.e. Instead of only the redirect form field getting injected through the S_LOGIN_REDIRECT variable, "temporarily" both the redirect form field and the FORM_TOKEN fields would come in through that variable. And don't start actually sending the FORM_TOKEN fields through the new the S_FORM_TOKEN_LOGIN variable until 3.2.7 or later, after there has been time for style authors to consume the 3.2.6 changes.
        • Make presentation of the login_body page test whether S_FORM_TOKEN_LOGIN is referenced in the template about to be used. Force proSilver if its absent. Still leaves all other usages of login_box() broken in a non-compatible style, but at least "login to the board to investigate and fix the issue" isn't 100% broken.

        EDIT: Since the original issue fixed in 3.2.6 is flagged as a security issue, I'll assume the default position here is "we must make the new form token fields mandatory in this release." In which case I like option #3 the best, in which the security fix is delivered even to non-3.2.6-compatible styles "against their will", and without their knowing that they've been "tricked" into including additional hidden form fields.

        And then later, in phpBB 3.2.8 or 3.3.1 or whatever, phpBB finally starts delivering the fields through S_FORM_TOKEN_LOGIN as intended instead of through S_LOGIN_REDIRECT, after the styles have had a chance to catch up. Which won't prevent issues from still happening when it's made mandatory later, but hopefully the set of affected "can't login" users is much less than what it is right now trying to make it mandatory in 3.2.6.

        User avatar
        Marc
        Development Team Leader
        Development Team Leader
        Posts: 185
        Joined: Thu Sep 09, 2010 11:36 am
        Location: Munich, Germany

        Re: phpBB 3.2.6 FORM_TOKEN on login

        Post by Marc »

        Thanks for your input. Yes, as you have already deducted, this was done as a security measure. I do however like your idea for option #3 and am currently testing an implementation of it.

        User avatar
        EA117
        Registered User
        Posts: 21
        Joined: Tue Apr 30, 2019 12:54 pm

        Re: phpBB 3.2.6 FORM_TOKEN on login

        Post by EA117 »

        Thanks for putting in such an efficient implementation of that approach; way less involved than what I had been picturing. Hopefully this does equate to less 3.2.7 upgrade friction, and ultimately fewer impacted users when S_FORUM_TOKEN_LOGIN support finally becomes mandatory again in 3.3.0.

        User avatar
        EA117
        Registered User
        Posts: 21
        Joined: Tue Apr 30, 2019 12:54 pm

        Re: phpBB 3.2.6 FORM_TOKEN on login

        Post by EA117 »

        This discussion on phpbb.com/community connected a couple dots I hadn't put together yet: Technically a phpBB 3.2.5 and earlier style could have offered the ability to login "from any page." i.e. Your template simply needed to decide "do I want to offer a login form here", and it could successfully do so using template-only changes.

        (I was incorrectly thinking that even {S_LOGIN_REDIRECT} was only generated for pages "expected" by phpBB to have a login form, but it was actually available for every page.)

        So now that add_form_token() is required in order to have a successful login form in phpBB 3.2.6 and later in order to address security concerns, might "the right change to make" be to generate {S_FORM_TOKEN_LOGIN} as part of function.php page_header(), so that it's "available on every page" same as {S_LOGIN_REDIRECT} was available?

        To not do so means any style which had implemented login forms "on additional pages" are now broken in phpBB 3.2.6 and later, and can't fix it through changes to the style. They would need PHP code and an extension in order to try and initiate the required add_form_token() on additional pages. It would also mean that the {S_LOGIN_REDIRECT} is now "uselessly included" for every page, when login is no longer possible "from every page."

        edit: https://tracker.phpbb.com/browse/PHPBB3-16054

        User avatar
        david63
        Registered User
        Posts: 355
        Joined: Mon Feb 07, 2005 7:23 am

        Re: phpBB 3.2.6 FORM_TOKEN on login

        Post by david63 »

        EA117 wrote: Mon May 13, 2019 12:18 pm To not do so means any style which had implemented login forms "on additional pages" are now broken in phpBB 3.2.6 and later, and can't fix it through changes to the style.
        That is not necessarily true. The style author can remove the functionality from the style.

        I don't believe that it is the responsibility of phpBB core to attempt to handle any and every feature that style developers, or for that matter extension developers, include in their respective developments, but the responsibility of those developers to adapt their development to work within the scope of the core product.
        David
        Remember: You only know what you know -
        and you do not know what you do not know!

        User avatar
        EA117
        Registered User
        Posts: 21
        Joined: Tue Apr 30, 2019 12:54 pm

        Re: phpBB 3.2.6 FORM_TOKEN on login

        Post by EA117 »

        david63 wrote: Mon May 13, 2019 2:56 pm I don't believe that it is the responsibility of phpBB core to attempt to handle any and every feature that style developers, or for that matter extension developers, include in their respective developments...
        That is a completely fair point and perspective for looking at situations such as this.

        But what also seems clear for this specific scenario is "phpBB's intention", in the sense that "S_LOGIN_REDIRECT was being provided for every page" prior to phpBB 3.2.6, and still is being provided for every page even now in phpBB 3.2.7. Not just "some subset of pages phpBB wants to support login on and style developers should now all adapt and limit their functionality to."

        While there absolutely are situations where "that's just the way it's going to be from now on" is the only feasible answer, the impetus for this particular change -- addressing a security concern for the login form -- did not inherently need or desire "login functionality must now also be prevented from appearing on additional pages in order to realize this security fix."

        The behavior appears to have been "simply overlooked", in that the add_form_key() calls were added to individual login page functions "in the same way add_form_key() is introduced to all other non-login form pages."

        But in the case of login forms, that's not actually the logical pattern, nor how login form templates have been receiving "required login form fields." The actual logical change isn't putting the add_forum_key() call in each individual page function, but simply put it in page_header(). Same as the generation of the S_LOGIN_REDIRECT has been the responsibility of page_header() (and still is), and not something each individual login page function is responsible to generate.

        Obviously I can't speak for the original change author, but there is nothing inherent to this security issue fix which would reasonably expect "and we should also prevent existing style template login pages from working", proSilver or otherwise. Creating this non-compatible situation was just an oversight; not "the way it must be", nor even "the way it should be", in order to provide the desired security fix.

        phpBB already corrected the unintentional loss of backwards compatibility with https://tracker.phpbb.com/browse/PHPBB3-16042, and with this current report I'm simply kicking myself for not realizing and proposing that it should have been addressed differently in order to magnify that scope of backwards compatibility. Such that phpBB is actually addressing the security issue "where phpBB allows login forms to be used", instead of continuing the original security fix's track of "only fix it where proSilver uses login forms."

        Post Reply