Skip to content

MFA: ignore non html requests#38492

Merged
roland-d merged 2 commits intojoomla:4.2-devfrom
Fedik:mfa-non-html
Aug 17, 2022
Merged

MFA: ignore non html requests#38492
roland-d merged 2 commits intojoomla:4.2-devfrom
Fedik:mfa-non-html

Conversation

@Fedik
Copy link
Copy Markdown
Member

@Fedik Fedik commented Aug 17, 2022

Pull Request for Issue #38476 and #38436 .

Summary of Changes

MFA redirect should not happen on non HTML request

Testing Instructions

Please follow #38436 and #38476

@bayareajenn
Copy link
Copy Markdown

I have tested this item ✅ successfully on 1fd81f6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38492.

1 similar comment
@viocassel
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on 1fd81f6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38492.

@richard67
Copy link
Copy Markdown
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38492.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 17, 2022
@roland-d roland-d merged commit 7ebca92 into joomla:4.2-dev Aug 17, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 17, 2022
@roland-d
Copy link
Copy Markdown
Contributor

Thanks everybody

@fancyFranci fancyFranci added this to the Joomla 4.2.1 milestone Aug 17, 2022
@Fedik Fedik deleted the mfa-non-html branch August 17, 2022 20:53
@nikosdion
Copy link
Copy Markdown
Contributor

From a security perspective, this is wrong.

I am an attacker. I steal someone's username and password. I log into the site. I am in the captive page. Now I can run any non-HTML context with the full privileges of the user whose credentials I stole and nobody will stop me.

First of all, this is a massive security hole for GET requests because I can retrieve any data the user has access to. If I have stolen the credentials of an Administrator or Super User it's very likely I have access to a component's JSON or CSV view which gives me access to personally identifiable information. Congratulations, your get several thousands to millions of Euros in fines thanks to GDPR.

Second, it now becomes TRIVIAL for me to nerf your site as my POST requests can proceed uninhibited... as long as I send format=bozo to the POST request. Whine whine we have the anti-CSRF token whine whine. Bollocks. The captive page contains the anti-CSRF token and any time I want to fetch a new one I just have to GET index.php with no arguments. If I've stollen the credentials of a Publisher or above I can deface your site at the very least.

I HAD ALREADY EXPLAINED ALL OF THESE IN MAY, THE FIRST TIME @Fedik HAD THIS DISASTROUS IDEA. So what did you do here? You just undid all the good that captive Multi-factor Authentication brought to Joomla.

@roland-d You need to revert it. Moreover, you and @fancyFranci ARE SUPPOSED TO ASK THE CODE OWNER, THAT'S ME, BEFORE MERGING ANY PR REGARDING THE WEBAUTHN AND THE MFA FEATURES. I am the code owner of these features for a reason!

roland-d added a commit that referenced this pull request Aug 18, 2022
roland-d added a commit that referenced this pull request Aug 18, 2022
@roland-d
Copy link
Copy Markdown
Contributor

@nikosdion Thank you for catching this. I have reverted this now and we can check later how it should be handled as it does show an error when users click on the button.

As for contacting the code owner, I understand that but I never thought about contacting a code owner because I was not triggered by anything. Yes, I know you contributed this code but I do not know that for every feature in Joomla. Having some kind of notification to contact the code owner would be helpful. Perhaps we should add you to the codeowners file if that helps, I do not know.

@roland-d roland-d removed this from the Joomla! 4.2.1 milestone Aug 18, 2022
@richard67
Copy link
Copy Markdown
Member

@roland-d Shouldn't we re-open the 2 issues #38476 and #38436 as long as we don't have the solution?

@roland-d
Copy link
Copy Markdown
Contributor

@richard67 Good point, we should only re-open 38476 as the fix was for that.

@nikosdion
Copy link
Copy Markdown
Contributor

@roland-d I am always going through the commits before a new release because I am weird like that :p

Regarding code ownership, I think there are two problems. One is GitHub and the other is the institutional knowledge gap which is a chronic Joomla pain.

I had been added in the .github/CODEOWNERS file for the WebAuthn plugin and the MFA. However, since I am a lowly Collaborator and not a full Member of the repo, GitHub would NOT auto-assign me as a reviewer for PRs touching my code. Therefore this change never made it to the repo. Even if you add me now, it will be for naught (and I think I was told it was creating problems, dunno?)

So, I have a question. Can I get into the CMS maintenance team? At this point I am pretty much doing the work anyway so if it means I can actually get to be notified for PRs touching security-sensitive code I have contributed so much the better. I'd prefer if there was a way to do that without having to carry a badge, albeit GitHub won't play ball. Well, in for a dime, in for a dollar.

@richard67 I had already said how we can address that problem:

Another way to do it — best practice for 3PDs actually — is for the plugin to check if we're in a captive page and not execute. I am leaving this note not as something applicable to this PR but as something that a 3PD might stumble on. The rabbit hole will lead them to this comment here and they will figure out what to do next ;)

If you assign #38436 and #38476 to me I can make it happen, tomorrow, after a morning swimming session.

@richard67
Copy link
Copy Markdown
Member

@nikosdion Assigned #38476 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants