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

Decorator UniqueSize doesn't yield unique results #152

Open
ngregoire opened this issue Oct 3, 2024 · 4 comments
Open

Decorator UniqueSize doesn't yield unique results #152

ngregoire opened this issue Oct 3, 2024 · 4 comments

Comments

@ngregoire
Copy link

Tested version: v1.5 as a Burp extension

The documentation states that @UniqueSize should return one instance of responses with a given status/size. I tested it on a few endpoints and always got extra entries (even on a static image).

The script

def queueRequests(target, wordlists):

    # Start of range

    timestamp = 1567555200

    # Use Turbo Intruder's HTTP/2 stack
    engine = RequestEngine(endpoint=target.endpoint, engine=Engine.HTTP2)

    # Emit requests
    for i in range(timestamp, timestamp + 86400):
        engine.queue(target.req, i)

@UniqueSize()
def handleResponse(req, interesting):

    # Log all unique responses
    table.add(req)

The base request:

GET /duck_hunt/game.png?rand=%s HTTP/2
Host: www.arcade.pwn

The results:
image

This bug isn't new and I noticed it before but totally forgot to create an issue.

@albinowax
Copy link
Contributor

@defparam any ideas?

@defparam
Copy link

defparam commented Oct 4, 2024

I'll take a peek today, there is critical path through the decorator implementation that likely isn't thread safe causing these race conditions. I've noticed it too and have been meaning to fix it.

@defparam
Copy link

defparam commented Oct 5, 2024

As I suspected, adding a global lock in the decorator fixes this issue. The issue comes when responses are collected on burst back when starting a test. Each thread competes to get to handleResponse and the decorator attempts to deal with each response one at the time. However in reality atomicity is not promised and I should have added a threading lock to each of these implementations. I don't have a pull request yet because I need more time to test the fix for each of the decorators but here is the fix for @UniqueSize() in lieu of a proper pull request to come (I'll try to get one in this month).

Best,
defparam

import threading

decoratorLock = threading.Lock()

def UniqueSize(instances=1):
    def decorator(func):
        def handleResponse(req, interesting):
            global CodeLength
            decoratorLock.acquire()
            try:
                CodeLength
            except:
                CodeLength = {}

            if "lastreq" in CodeLength:
                currreqs = req.engine.engine.successfulRequests.intValue()
                lastreqs = CodeLength["lastreq"]
                if currreqs < lastreqs:
                    CodeLength = {}
                    CodeLength["lastreq"] = currreqs

            CodeLength["lastreq"] = req.engine.engine.successfulRequests.intValue()

            codelen = str(req.status) + str(req.length)
            if codelen in CodeLength:
                if CodeLength[codelen] >= instances:
                    decoratorLock.release()
                    return
                else:
                    CodeLength[codelen] += 1
            else:
                CodeLength[codelen] = 1
            decoratorLock.release()
            func(req, interesting)
        return handleResponse
    return decorator

@ngregoire
Copy link
Author

I tested your patch and it works fine. Well done! 👍

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

No branches or pull requests

3 participants