LPP overwrites comments created by reshaper

[0xm4v3rick]

Description: When I have logger++ enabled, reshaper burp plugin is not able to add comment based on the created rules. Adding video that should clarify the issue. Happens when LPP is enabled before reshaper in burp Extensions tab. Currently I have to start LPP way before reshaper as a workaround. I created an issue with reshaper and here is what was discussed https://github.com/synfron/ReshaperForBurp/issues/27

Steps To Reproduce:

• Create a rule in reshaper to add comment based on presence of parameter.
• Observe that the comments vanish when LPP is enabled. More details in the video.

Expected behavior: Comments added by other tools should persist instead of being overwritten by LPP. Any option/switch to do that would be helpful. A more friendlier way of using comments so that other plugins can also use this option and not get overwritten by LPP

Screenshots: Video attached.

Version: 3.19.2 from bApp store

https://user-images.githubusercontent.com/29249263/185376837-5f88da45-dca2-4470-975e-0b4a68e5c3aa.mp4

[CoreyD97]

Hey @0xm4v3rick

Thanks for opening this issue. I'm not able to look into this for a few weeks as I'm on vacation, though it is interesting that you're experiencing this behavior.

Unfortunately the use of comments to track requests and responses is necessary to ensure that pairs are matched up correctly. However when making this design choice I also made sure that the extension does this in a friendly way to work with other plugins. Specifically, L++ should append a comment to requests rather than overwriting the value, and finally remove the comment after the response has been matched. The comment used here is also designed to be in a form that is very unlikely to be used by other extensions to prevent conflicts. Up to now, I haven't seen any conflict with how the extension handles comments (except L++ not correctly displaying comments in the table or allowing setting of new comments).

I'll look into this when I get back, and hopefully will be able to solve this issue for you.

[0xm4v3rick]

@CoreyD97 No worries, I can wait. Have a awesome and healthy vacation!!

[CoreyD97]

Sorry for the delay here. I started looking into this but couldn't find the cause. As mentioned previously, the extension actually appends the identifier to the comment, so shouldn't overwrite it. Similarly, when removing the identifier, it only removes the part of the value that matches the L++ identifier. This is something that was implemented from the start, and has worked perfectly for a couple of years, so it is odd that it has started recently.

However, Burp did start making some changes to the API around the time you reported this, and a few other issues showed up so it is possible that something changed on the underlying API. I've also recently ported the extension to the new Montoya API, and am working with PortSwigger to get a better way of tracking requests implemented.

I'll leave this issue open for now, and hopefully I can find a better solution in the meantime.

[tsc-awardle]

A couple of observations here:

The order of events in the video clip seems strange:

1. HTTP history shows request with comment my comment (empty Status and Length fields).
2. Comment is overwritten with LPP identifier at the same time the response completes (Status and Length field populated)
3. LPP identifier is eventually removed

I would expect the LPP token to be present during step 1, and I don't understand how step 3 happens. The response is already final at this point, so I wouldn't expect another callback to any of the LogProcessor.java methods at this point.

I have a couple of theories:

1. LPP is actually setting the identifier before reshaper, and reshaper is overwriting the initial LPP identifier.
2. Since reshaper is modifying the request, could it be causing a mismatch of the logEntry identifier?

On a related note, I noticed that LPP stopped logging responses for Repeater traffic. The requests show up in LPP window without a blank comment, which eventually changes to "Timed Out" after a minute or so via the AbandonedRequestCleanupRunnable

I'll see if I can find some time to step through the callbacks with a debugger and find out where things are going wrong.

[tsc-awardle]

I tested the following:

• Reshaper rule: Event Direction = Request, LPP: disabled
  • Comment shows up in proxy history during request, but is erased upon response.
  • No other extensions enabled that would mess with the comments, so this would be in the scope of the Reshaper plugin
• Reshaper rule: _Event Direction = Response, LPP: disabled
  • Comment shows up as expected in proxy history
• Reshaper rule: Event Direction = Response, LPP enabled and loaded BEFORE Reshaper (higher in the extensions list)
  • Comment shows up as expected in proxy history. LPP works as expected
• Reshaper rule: Event Direction = Response, LPP enabled and loaded AFTER Reshaper (lower in the extensions list)
  • Comment shows up as expected in proxy history. but LPP only shows incomplete requests

Based on this behavior, it looks like Burp callbacks occur in the reverse order of the extension order. So extensions at the bottom of the list get called first, and it works its way to the top.

@0xm4v3rick, how is your Reshaper rule configured? And can you try re-ordering the the extensions to see if that makes a difference?

[CoreyD97]

Thanks for your testing here @tsc-awardle, I really appreciate it. This issue was originally created before this commit, and this commit isn't part of the Logger++ release on the BApp store. I wonder if these changes would have fixed this issue?

Originally, Logger++ relied on tagging requests in their comment, but to avoid incompatibilities with other extensions like in this case, that commit changed the extension to only use that method where necessary. With this change, proxy responses are temporarily tagged during processHttpMessage and is removed during processProxyMessage, both of which happen only when the response is received.

However, the behavior you observed with the first extension being processed last seems contrary to what we've typically understood about Burp's handling of extensions. Maybe something changed with recent versions? This image from @floyd-fuh shows the process as we have previously understood it.

[tsc-awardle]

Yes, I was testing with the most recent master branch. Though I loaded up the bapp store version and it still seems to work fine as long LPP is loaded after Reshaper, and the Reshaper rule is configured to edit the response in stead of the request (Reshaper request comments do not persist, regardless of LPP)

That is a super helpful image! I think it's still a valid diagram, as Burp is probably still calling request callbacks in ascending order, but then we see that response callbacks are called in reverse. This explains why LPP needs to be the very last extension, otherwise Reshaper clobbers the comment before it reaches LPP.

Also, yes there have been some significant changes to Burp's internals over the last year. I think at least one of my extensions breaks in some way after each update🙃. Unfortunately, these are mostly opaque so solving bugs takes some trial and error.

Given the age of this bug, and the fact that I couldn't get reshaper request comments working at all, it's entirely possible that a recent Burp update broke the ability to persist comments on incomplete requests. Or perhaps I have setting configured somewhere that's causing the behavior.

~My repeater requests are still showing up as incomplete in LPP when using the master branch, but they actually work just fine in the bapp store version. Maybe repeater requests also need special treatment like the proxy?~

EDIT: Opening in a new issue and submitting PR for repeater problem.

[0xm4v3rick]

@CoreyD97 @tsc-awardle I just tested this and the reshaper rules to add the comments seems to be working now - irrespective of the LPP position in the extension list. Few days back reshaper comments were not even working even though LPP was not installed. Looks like new burp versions have changed a few things or its something else? ¯_(ツ)_/¯