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

Fixing credentials leak #756

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fixing credentials leak #756

wants to merge 5 commits into from

Conversation

aliowka
Copy link

@aliowka aliowka commented May 25, 2018

When chaining upstream proxy to BMP through HTTPS connection, the upstream proxy credentials should be sent only with CONNECT request. Otherwise, if the upstream proxy does not provide man-in-the-midle, it will be unable to dismiss Proxy-Authorization header, so the header will eventually leak to the end point site, which provides serious security flaw.
The site htat reports the Prox-Authorization header: https://www.piliapp.com/what-is-my/http-request-headers/
Other sites might not report it: I already opened an issue to httpbin.org

To fix this, I have added 2 more conditions on adding the credentials header:

1. The request should be CONNECT
2. Otherwise the uri should not start with '/', which means we are in plain HTTP mode.
In HTTP mode we should always pass credentials with each request.

I hope this helps.

aliowka-fornova added 5 commits May 23, 2018 19:22
@aliowka
Copy link
Author

aliowka commented Oct 24, 2018

Hi @jekh do you plan to accept this any time soon? Do you have any objections or questions that I can help with? I have no problem to run with my own fork of BMP but the fix is critical IMHO. Or may be it's only me here who cares about my proxy username and password to not be given away to each site I visit?
We all busy, totally understood.

@ericbeland
Copy link

ericbeland commented Apr 26, 2019

@aliowka, We have an actively maintained fork of the BrowserMob Proxy, renamed to the BrowserUp Proxy with Java 11 support and modern dependencies. Feel free to open a PR there. We may eventually move this in anyway as it seems useful.
Cheers!
Eric

@aliowka
Copy link
Author

aliowka commented Apr 28, 2019

Thanks @ericbeland will do.

@ericbeland
Copy link

We merged this fix into the BrowserUp Proxy today. Cheers!

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.

None yet

2 participants