Skip to content

Added Broken Access Control Module #673

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Added Broken Access Control Module #673

wants to merge 12 commits into from

Conversation

Hamed233
Copy link

  • Added a new Broken Access Control module demonstrating OWASP A01-2021.
  • Implemented multiple security levels (Low, Medium, High, Impossible).
  • Updated README with details about the new module.
  • Modified database schema to support access logs and security monitoring.

@digininja
Copy link
Owner

I like the idea, thanks for submitting. I've not had chance to look at the code yet, but a couple of comments.

Could you remove all the DS_Store files, they shouldn't be in the repo.

As you've changed the database schema, have you done anything so that the changes are automatically applied? If not, it isn't going to work. People with existing installs will update their repo, the new function will break, they will panic and start raising issues. Even with documentation, they won't read it, they will just complain.

@Hamed233
Copy link
Author

I like the idea, thanks for submitting. I've not had chance to look at the code yet, but a couple of comments.

Could you remove all the DS_Store files, they shouldn't be in the repo.

As you've changed the database schema, have you done anything so that the changes are automatically applied? If not, it isn't going to work. People with existing installs will update their repo, the new function will break, they will panic and start raising issues. Even with documentation, they won't read it, they will just complain.

✅ I have removed all .DS_Store files and updated .gitignore to prevent them from being added again.

✅ I have already implemented the necessary changes in setup.php to ensure that new database modifications are applied automatically during installation, so existing users won't face issues.

@digininja
Copy link
Owner

Existing users don't run the setup script, they already have a working system.

@digininja
Copy link
Owner

Can you also test your changes in the Docker container, I don't see why they shouldn't just work, but I've thought that before and later found the whole thing broken.

@Hamed233
Copy link
Author

Existing users don't run the setup script, they already have a working system.

Screenshot 2025-02-21 at 7 43 45 PM

I have implemented a database verification check in index.php to ensure users have the required database components before accessing the Broken Access Control module.
Tested thoroughly—everything is working without issues!

@digininja
Copy link
Owner

digininja commented Feb 21, 2025 via email

@Hamed233
Copy link
Author

I'll give it a test over the weekend. But however thoroughly you think you've tested it, users will find bugs you never expected or will use it in a completely different way and break it.
…
On Fri, 21 Feb 2025, 17:49 Hamed Essam, @.> wrote: Existing users don't run the setup script, they already have a working system. Screenshot.2025-02-21.at.7.43.45.PM.png (view on web) https://github.com/user-attachments/assets/36eb77dc-417a-499c-9b56-19c292d8cc7a I have implemented a database verification check in index.php to ensure users have the required database components before accessing the Broken Access Control module. Tested thoroughly—everything is working without issues! — Reply to this email directly, view it on GitHub <#673 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA4SWNCSYXNK4RI3N7567T2Q5RMHAVCNFSM6AAAAABXTSW44CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZVGE4DQOJWGI . You are receiving this because you commented.Message ID: @.> [image: Hamed233]Hamed233 left a comment (digininja/DVWA#673) <#673 (comment)> Existing users don't run the setup script, they already have a working system. Screenshot.2025-02-21.at.7.43.45.PM.png (view on web) https://github.com/user-attachments/assets/36eb77dc-417a-499c-9b56-19c292d8cc7a I have implemented a database verification check in index.php to ensure users have the required database components before accessing the Broken Access Control module. Tested thoroughly—everything is working without issues! — Reply to this email directly, view it on GitHub <#673 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA4SWNCSYXNK4RI3N7567T2Q5RMHAVCNFSM6AAAAABXTSW44CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZVGE4DQOJWGI . You are receiving this because you commented.Message ID: @.***>

That sounds great! I totally agree—no matter how much we test, real users always find creative ways to break things. 😄

Looking forward to your feedback after testing. Let me know if anything needs improvement!

@digininja
Copy link
Owner

digininja commented Feb 21, 2025 via email

@Hamed233
Copy link
Author

@digininja Tested it bro?

@digininja
Copy link
Owner

Just looking. I've tried to run the setup script and got this:

Fatal error: Uncaught mysqli_sql_exception: Unknown column 'details' in 'security_log' in /var/sites/dvwa/non-secure/htdocs/dvwa/includes/DBMS/MySQL.php:189 Stack trace: #0 /var/sites/dvwa/non-secure/htdocs/dvwa/includes/DBMS/MySQL.php(189): mysqli_query() #1 /var/sites/dvwa/non-secure/htdocs/setup.php(23): include_once('...') #2 {main} thrown in /var/sites/dvwa/non-secure/htdocs/dvwa/includes/DBMS/MySQL.php on line 189

@Hamed233
Copy link
Author

details

Just looking. I've tried to run the setup script and got this:

Fatal error: Uncaught mysqli_sql_exception: Unknown column 'details' in 'security_log' in /var/sites/dvwa/non-secure/htdocs/dvwa/includes/DBMS/MySQL.php:189 Stack trace: #0 /var/sites/dvwa/non-secure/htdocs/dvwa/includes/DBMS/MySQL.php(189): mysqli_query() #1 /var/sites/dvwa/non-secure/htdocs/setup.php(23): include_once('...') #2 {main} thrown in /var/sites/dvwa/non-secure/htdocs/dvwa/includes/DBMS/MySQL.php on line 189

Pull last version and test now

@digininja
Copy link
Owner

I don't understand the point of the low level, there should be some challenge to it but there isn't.

On medium, you need some kind of hint that the permissions are based on the cookie. User's won't read the code, they will just throw attacks at it, and they are unlikely to guess to set a cookie called admin.

Same for high, there is nothing obvious to say what type of attack the user should be trying, you need to give a hint of how to attack it on the page.

Medium and low, if you enter anything other than an integer, you set $html but then exit so it never gets printed.

For the help, you need to give a full walk through on the attack. Have a look at some of the others, there is a strong hint followed by the solution. Check Crypto for the new way to do hints in the help, it is better than the old spoiler technique.

Can you move the menu item to the bottom, that is where people look for the newer modules.

You've implemented a log feature, is there any way to view the log? It could be fun to add XSS into that if there is a way.

@digininja
Copy link
Owner

digininja commented Feb 24, 2025 via email

@Hamed233
Copy link
Author

Hi @digininja,

Let me summarize the improvements I've made to address all the feedback points:

Low Level:

  • Added a simple token-based challenge that's easily bypassed
  • Added clear hints in the HTML comments
  • Fixed the issue with exit() not showing error messages
  • Made the challenge more educational while still being intentionally vulnerable

Medium Level:

  • Improved the cookie-based challenge to be more obvious
  • Added visual feedback showing current role status
  • Added clear hints about checking browser dev tools
  • Changed from a simple 'admin' cookie to a more realistic 'user_role' system

Help Documentation:

  • Completely revamped following the Crypto module style
  • Added collapsible solutions for each level
  • Provided clear, progressive hints
  • Added detailed step-by-step solutions
  • Included information about the logging feature

Logging Features:

  • Implemented logging across all levels
  • Used X-Forwarded-For header when available (hidden vulnerability)
  • Added comprehensive logging details including roles and access attempts

@digininja
Copy link
Owner

Sorry, been busy so just getting round to testing this.

First error, I got this on the medium level:

Fatal error: Uncaught mysqli_sql_exception: Unknown column 'module' in 'field list' in /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/source/medium.php:52 Stack trace: #0 /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/source/medium.php(52): mysqli_query() #1 /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/index.php(80): require_once('...') #2 {main} thrown in /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/source/medium.php on line 52

Re-running the setup script doesn't fix it.

Same for the details field.

The user_id, target_id, and timestamp fields in the table don't have a default value so the insert from line 50 of medium fails. I patched that up in the code but tried again and got this:

Fatal error: Uncaught mysqli_sql_exception: Cannot add or update a child row: a foreign key constraint fails (`dvwa`.`security_log`, CONSTRAINT `security_log_ibfk_1` FOREIGN KEY (`user_id`) REFERENCES `users` (`user_id`)) in /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/source/medium.php:52 Stack trace: #0 /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/source/medium.php(52): mysqli_query() #1 /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/index.php(80): require_once('...') #2 {main} thrown in /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/source/medium.php on line 52

Looks like the same for the other levels as well.

@Hamed233
Copy link
Author

try now!

@digininja
Copy link
Owner

Just realised I never got back to looking at this, really sorry, I'll try to go through it tomorrow.

@digininja
Copy link
Owner

Finally looking at this...

Low level, seems ok.
Medium level, the user_role token is ignored, all that is needed is the user_id token. As this challenge can be solved based on the in browser hints and looking at requests, I think it is easier than the low so swap the two around.
High prints this at the top of the page:

Notice: Only variables should be passed by reference in /var/sites/dvwa/non-secure/htdocs/vulnerabilities/bac/source/high.php on line 9

It also just works and lets me access any profile I want. I've had a quick skim of the source and it is probably because I've logged in as admin. As everyone logs in as admin, limiting non-admins isn't going to work.

Can you also move the left hand menu item to the bottom, that is where people look for new modules so may miss this when it is released.

We need a way to view the log, maybe add a link to it to the security page under the level section. It looks like at the moment if you just dumped out what is written into the log you could get XSS through the IP address by putting JS into the X-Forwarded-For field, that would be a nice additional vuln to keep in.

It is getting closer.

@Hamed233
Copy link
Author

Hamed233 commented Apr 3, 2025

@digininja Done!

@digininja
Copy link
Owner

I'm about to go away for a week so will check on this when I get back. If you've not heard back in a couple of weeks, prod me to remind me.

Repository owner deleted a comment from anshika-panwar-05-code Apr 7, 2025
@Hamed233
Copy link
Author

@digininja Did you check it?

digininja
digininja reviewed Apr 13, 2025
View reviewed changes
config/config.inc.php.dist Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Why have you deleted the default config file?

Copy link
Author

Choose a reason for hiding this comment

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

Screenshot 2025-04-13 at 13 58 03 Still Exist! maybe wasn't pushed!

@digininja
Copy link
Owner

On impossible:

Fatal error: Uncaught mysqli_sql_exception: Unknown column 'action' in 'field list' in /var/sites/dvwa/html/DVWA/vulnerabilities/bac/source/impossible.php:209 Stack trace: #0 /var/sites/dvwa/html/DVWA/vulnerabilities/bac/source/impossible.php(209): mysqli_prepare() #1 /var/sites/dvwa/html/DVWA/vulnerabilities/bac/source/impossible.php(50): logAccessAttempt() #2 /var/sites/dvwa/html/DVWA/vulnerabilities/bac/index.php(100): require_once('...') #3 {main} thrown in /var/sites/dvwa/html/DVWA/vulnerabilities/bac/source/impossible.php on line 209

The help blocks need swapping round after swapping the challenges round.

I don't understand how you expect the high level to be solved, can you update the help to give a full walk through.

@digininja
Copy link
Owner

digininja commented Apr 13, 2025 via email

@Hamed233
Copy link
Author

I added the steps in the help section and adjusted the config file as you requested.

As for the 'impossible' part, that's cache in the DB because I added code that checks whether the table exists or not.

@digininja
Copy link
Owner

digininja commented Apr 13, 2025 via email

@Hamed233
Copy link
Author

Yes, that's what I mean

@digininja
Copy link
Owner

digininja commented Apr 13, 2025 via email

@Hamed233
Copy link
Author

Check Now @digininja

@digininja
Copy link
Owner

In a clean browser, I've just tried the high level and there is no user_id cookie set.

I've just tried setting it as described in the solution and I'm still blocked from accessing user 2

image

Looking at this code, you are checking the session value, not the cookie value

            // "Secure" session-based check (but vulnerable to session fixation)
            if (isset($_SESSION['user_id'])) {
                $session_id = intval($_SESSION['user_id']);

                if ($id == $session_id) {

On medium, should I be able to see my own profile? ID = 1? I can't, I get the error Access denied. Valid token required.

I think something has gone wrong with setting cookies, my fresh browser doesn't have the user_id for high or the user_role for medium. Can you check this.

@Hamed233
Copy link
Author

These changes should resolve all the issues mentioned before:

  • The high level now clearly uses session variables, not cookies
  • The medium level now has clear instructions about using the token parameter
  • The low level now properly sets the user_id cookie
  • All help documentation has been updated to match the actual implementations

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

@digininja digininja digininja left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Hamed233 @digininja