Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulnerability using API authorization with SQL Injection #4373

Merged
merged 4 commits into from
Sep 28, 2024

Conversation

nilsteampassnet
Copy link
Owner

Fixing vulnerability regarding API authorize process

A SQL injection has been detected on the API. This injection occurs on the endpoint used to
authenticate users on the API ( /api/index.php/authorize ). It turns out that at code level, two
vulnerable queries are used to authenticate users. These SQL queries are performed in the
getUserAuth function of the /api/Model/AuthModel.php 昀椀le.
As it can be seen, a concatenation is performed in the PHP code with one of the parameters ( login )
controlled by the user. However, it remains di昀케cult to exploit, as the anti XSS 昀椀lters applied to user
input do not allow the request to be manipulated in order to add code later.
However, a second request may allow controlled injection.

The fix consists in removing the old condition of auth with a generic key, which is not allowed any more.
THis part of code has been removed and replaced by an error code.
In front, the API generic keys page has already a warning showing they should not be used any more.

Fixing vulnerability regarding API authorize process
Grouping 2 conditions into 1 unique as suggested
Copy link
Contributor

@corentin-soriano corentin-soriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve on this scope if you want to merge it however there are other SQL queries nested with PHP which are potentially vulnerable.

@nilsteampassnet
Copy link
Owner Author

I approve on this scope if you want to merge it however there are other SQL queries nested with PHP which are potentially vulnerable.

Any example?

@corentin-soriano
Copy link
Contributor

I approve on this scope if you want to merge it however there are other SQL queries nested with PHP which are potentially vulnerable.

Any example?

Line 81

Remplaced existing API database class by MeekroDB library as in Teampass core
Copy link
Contributor

@corentin-soriano corentin-soriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thank you!

api/Model/AuthModel.php Outdated Show resolved Hide resolved
Debug command removal
@nilsteampassnet nilsteampassnet merged commit 5a0744f into master Sep 28, 2024
2 checks passed
@nilsteampassnet nilsteampassnet deleted the vulnerability-api branch September 28, 2024 09:42
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.

2 participants